Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29333107: Issue #1652, #3456 - Replace 's_asyncWebBrowser2' with thread-safe facilities

Dec. 29, 2015, 5:41 p.m. by Eric
July 27, 2016, 10:26 p.m.
sergei, Oleksandr
Felix Dahlke


Issue #1652, #3456 - Replace 's_asyncWebBrowser2' with thread-safe facilities Remove 's_asyncWebBrowser2' and 'GetAsyncBrowser()'. Replace them with a new class 'AnyCurrentSiteBrowser', which encapsulates an 'IWebBrowser2*' site pointer and ensures that it remains a current site during its life span. Update all the functions that had used 'GetAsyncBrowser()'. Remove 's_instances'. Replace it with a new class 'SyncSet' with its own mutex. A separate mutex is required to properly implement 'AnyCurrentSiteBrowser'. Rewrite explicit loops through 's_instances' with the functional member 'LinearSearch()'. Clarify the code in 'ShowStatusBar()', flattening its control flow and improving its debug messages. In 'FirstRunThread()', add exception handler around body. Fix missing call to 'CoUninitialize()'. Fix missing test for null browser. Add function 'NavigateNewTab()', extracting duplicate code that had appeared in three different places. Change 'IsStatusBarEnabled()' to anonymous namespace. Renamed it to be more descriptive.

Patch Set 1 : #

Total comments: 41

Patch Set 2 : address comments #

Total comments: 9

Patch Set 3 : rebase only #

Unified diffs Side-by-side diffs Delta from patch set Stats (+399 lines, -197 lines) Patch
M src/plugin/Instances.h View 1 2 2 chunks +91 lines, -2 lines 0 comments Download
M src/plugin/PluginClass.h View 1 2 6 chunks +7 lines, -8 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 2 13 chunks +246 lines, -187 lines 0 comments Download
M test/plugin/InstancesTest.cpp View 1 2 2 chunks +55 lines, -0 lines 0 comments Download


Total messages: 10
Dec. 29, 2015, 5:56 p.m. (2015-12-29 17:56:00 UTC) #1
It'a good idea with 'AnyCurrentSiteBrowser', however it would be better to split the changes into ...
Jan. 4, 2016, 3:34 p.m. (2016-01-04 15:34:32 UTC) #2
On 2016/01/04 15:34:32, sergei wrote: > It'a good idea with 'AnyCurrentSiteBrowser', however it would be ...
Jan. 4, 2016, 5:49 p.m. (2016-01-04 17:49:21 UTC) #3
Patch set 2 addresses Sergei's comments that I accepted.
Jan. 4, 2016, 5:52 p.m. (2016-01-04 17:52:19 UTC) #4
I'm waiting for the opinion of others
Jan. 11, 2016, 10:37 a.m. (2016-01-11 10:37:29 UTC) #5
On 2016/01/11 10:37:29, sergei wrote: > I'm waiting for the opinion of others If you ...
Jan. 29, 2016, 2:21 p.m. (2016-01-29 14:21:39 UTC) #6
On 2016/01/29 14:21:39, Eric wrote: > On 2016/01/11 10:37:29, sergei wrote: > > I'm waiting ...
Jan. 29, 2016, 3:28 p.m. (2016-01-29 15:28:56 UTC) #7
None of the comments require code changes. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h#newcode25 src/plugin/Instances.h:25: * Template ...
Jan. 29, 2016, 4:16 p.m. (2016-01-29 16:16:13 UTC) #8
No comments regarding the AnyCurrentSiteBrowser - that part is fine by me. The comments I ...
Feb. 1, 2016, 6:03 p.m. (2016-02-01 18:03:15 UTC) #9
Feb. 3, 2016, 2:57 p.m. (2016-02-03 14:57:28 UTC) #10
On 2016/02/01 18:03:15, Oleksandr wrote:
> No comments regarding the AnyCurrentSiteBrowser - that part is fine by me. The
> comments I do have, I guess will be considered "bikeshedding", but in this
> I think these are important.

Bikeshedding is not raising a small concern the first time. It's spending too
much time on a small concern.

Your main concern seems to be with apparently-unrelated changes. This change set
has two kinds of changes: implementing class 'AnyCurrentSiteBrowser' and using
it. When I was working on the second of these two, I rewrote some code at the
point of use to make sure that it was being used correctly, since that was
rather more difficult with the old code.

These changes could be split off, but it would be a fair bit of work to make two
change sets, test the new one to make sure it neither breaks the build nor is
defective, and all for committing two change sets separately in a history that
we are very unlikely to revisit. We did not revisit changes that broke the build
(two of them, both involving ATL), so it seems of low value to split these
changes up.

File src/plugin/Instances.h (right):

src/plugin/Instances.h:25: * Template class for a synchronized set of BHO
On 2016/02/01 18:03:13, Oleksandr wrote:
> I think Sergei was merely talking about the comment here, not the
> implementation. 

I don't believe so. He has said more that once that he would prefer it not be a
template class. If it's not a template class, you get inferior unit testing.

> Why say it is a class for BHO instances when it can be so much
> more.

Because we're never going to use it for anything more.

This class is specifically targeted at the behavior of IE and the way it calls
'SetSite()' on BHO instances. Its behaviors aren't so weird that it couldn't be
used elsewhere, but it was written to deal with BHO semantics, and it's the only
way we'll use it. I see no downside in documenting the class with a
less-than-fully general comment that states very specifically the environment
the class is written for.

> We are using it here as well https://codereview.adblockplus.org/29330403/
> for example.

In fact we are _not_ using it there. The present class is 'SyncSet'. The one
there is 'SyncMap'. The use different underlying containers. What they do have
in common is similar behaviors specialized to be containers of BHO objects and
managed through 'SetSite()' as called by IE.

src/plugin/Instances.h:52: bool InsertIfAbsent(T p)
On 2016/02/01 18:03:13, Oleksandr wrote:
> I don't have a strong opinion on this, but I agree that we should treat this
> class as a generic one, not BHO specific one.

Answered above.

File src/plugin/Instances.h (right):

src/plugin/Instances.h:1: /*
On 2016/02/01 18:03:14, Oleksandr wrote:
> Nit: This file is also being added in
> https://codereview.adblockplus.org/29330403/. I'm guessing this review builds
> top of that one?

No. I made sure they were not interdependent. The idea, though, is that both
'SyncSet' and 'SyncMap' end up in the same source file, since they are related
classes that each deal with containers of BHO instances.

File src/plugin/PluginClass.cpp (right):

src/plugin/PluginClass.cpp:396: bool IsStatusBarDisabledInRegistry()
On 2016/02/01 18:03:14, Oleksandr wrote:
> I see no reason why IsStatusBarEnabled is refactored in this change set. It is
> unrelated, IMO.

It's not entirely unrelated. These changes tag along with the changes of type of
the various browser variables from 'IWebBrowser2' or 'CComPtr<IWebBrowser2' to
'AnyCurrentSiteBrowser'. Strictly speaking, they could go into a separate change
set, but it would be a "noissue" commit.

I don't see enough value in creating two change sets here to go through the
effort of splitting the change set up. It might be worth changing the commit
message to reflect that there are "noissue" changes tagging along here to
related parts of the code.

src/plugin/PluginClass.cpp:464: if (!isVisible)
On 2016/02/01 18:03:14, Oleksandr wrote:
> Same here. Why is this done in the change set with the message "Replace
> 's_asyncWebBrowser2' with thread-safe facilities"? I don't think this is
> at all.

See above.

src/plugin/PluginClass.cpp:975: NavigateNewTab(browser, FirstRunPageFileUrl());
On 2016/02/01 18:03:14, Oleksandr wrote:
> I agree this looks better with NavigateNewTab separated, but this is not
> relevant to the present change at all. 

When I went through and changed types to use 'AnyCurrentSiteBrowser', I went
ahead and improved the code in its vicinity.

As I've said elsewhere, it could be split into a "noissue" commit, but I don't
see a lot of value in that at this point.

Powered by Google App Engine
This is Rietveld