Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(164)

Issue 29323561: Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by Eric
Modified:
4 years, 2 months ago
Reviewers:
sergei, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass. Change type of 'm_webBrowser2' from 'CComQIPtr' to 'CComPtr' and also make it private. Remove 'GetBrowser()', which was not called outside this class. Its synchronization behavior is also no longer needed. Remove its synchronization member 's_criticalSectionBrowser'. Rewrite all occurrences of 'GetBrowser()' with direct references to 'm_webBrowser2'. Add 'IsRootBrowser()', which encapsulates otherwise- duplicated code (and reads better). Dead code. Remove 'FinalRelease()', which is no longer needed. Remove 'FinalConstruct()', which was providing nothing over its base class implementation. Remove 'GetTabHWND()', which was never called. Remove 'MainThreadProc()', 'GetMainThreadHandle()', and 'IsMainThreadDone()', none of which were even defined.

Patch Set 1 #

Total comments: 14

Patch Set 2 : fixed assignment of m_webBrowser2 in SetSite #

Patch Set 3 : change type of m_webBrowser2 #

Total comments: 22

Patch Set 4 : address comments #

Total comments: 4

Patch Set 5 : initialization; shorten comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -166 lines) Patch
M src/plugin/PluginClass.h View 1 2 3 4 5 chunks +7 lines, -16 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 2 3 4 14 chunks +55 lines, -150 lines 0 comments Download

Messages

Total messages: 18
Eric
https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp#newcode320 src/plugin/PluginClass.cpp:320: IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite); Moving this statement outside the try-block ensures it's ...
4 years, 6 months ago (2015-08-13 16:58:41 UTC) #1
sergei
It seems better to split it at least into removing of dead code and the ...
4 years, 4 months ago (2015-10-01 16:15:52 UTC) #2
Oleksandr
https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp#newcode220 src/plugin/PluginClass.cpp:220: */ On 2015/10/01 16:15:51, sergei wrote: > I'm sorry, ...
4 years, 4 months ago (2015-10-05 10:44:47 UTC) #3
sergei
https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp#newcode220 src/plugin/PluginClass.cpp:220: */ On 2015/10/05 10:44:47, Oleksandr wrote: > On 2015/10/01 ...
4 years, 4 months ago (2015-10-05 11:00:57 UTC) #4
Eric
New patch set also renames IsRootPageBrowser() to IsRootBrowser(). https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginClass.cpp#newcode140 src/plugin/PluginClass.cpp:140: CComPtr<IWebBrowser> ...
4 years, 3 months ago (2015-11-18 13:57:31 UTC) #5
sergei
On 2015/11/18 13:57:31, Eric wrote: > New patch set also renames IsRootPageBrowser() to IsRootBrowser(). > ...
4 years, 3 months ago (2015-11-18 14:23:57 UTC) #6
Eric
4 years, 3 months ago (2015-11-18 18:17:31 UTC) #7
sergei
https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp#oldcode335 src/plugin/PluginClass.cpp:335: m_webBrowser2.Release(); It's still good to call `m_webBrowser2.Release();` here at ...
4 years, 2 months ago (2015-11-30 15:52:09 UTC) #8
Eric
New patch set incorporates a rebase also. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp#oldcode335 src/plugin/PluginClass.cpp:335: m_webBrowser2.Release(); On ...
4 years, 2 months ago (2015-11-30 16:31:51 UTC) #9
Oleksandr
Overall a welcome change. One note I'd like to mention though is that FinalConstruct was ...
4 years, 2 months ago (2015-12-03 11:51:59 UTC) #10
Eric
New patch set. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp#newcode125 src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; Initialized it to zero, ...
4 years, 2 months ago (2015-12-03 14:24:55 UTC) #11
Oleksandr
The patch looks good, however I think the change is too substantial for a Noissue ...
4 years, 2 months ago (2015-12-04 11:19:09 UTC) #12
Eric
On 2015/12/04 11:19:09, Oleksandr wrote: > The patch looks good, however I think the change ...
4 years, 2 months ago (2015-12-04 14:17:18 UTC) #13
sergei
https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp#newcode93 src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) On 2015/11/30 16:31:51, Eric wrote: > On ...
4 years, 2 months ago (2015-12-07 10:49:09 UTC) #14
Eric
I've addressed Oleksandr's comment; there's now an issue for this change set. The most recent ...
4 years, 2 months ago (2015-12-07 14:04:13 UTC) #15
Oleksandr
Less code less bugs, I think. LGTM.
4 years, 2 months ago (2015-12-07 15:23:40 UTC) #16
sergei
LGMT https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginClass.cpp#newcode93 src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) On 2015/12/07 14:04:12, Eric wrote: > ...
4 years, 2 months ago (2015-12-08 09:42:00 UTC) #17
sergei
4 years, 2 months ago (2015-12-08 11:00:04 UTC) #18
On 2015/12/08 09:42:00, sergei wrote:
> LGMT
LGTM*
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5