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

Issue 29334386: Issue #2230 - Refactor filter life cycle and use

Jan. 22, 2016, 5:57 p.m. by Eric
May 17, 2016, 9:38 p.m.
sergei, Oleksandr
Felix Dahlke


Issue #2230 - Refactor filter life cycle and use Remove "CPluginTab::m_traverser" and replace it with an object constructed at the point of traversal. Remove "CPluginTraverserBase::ClearCache()". Because traverser object are now used only once, there's no reason to reset their state. Removed "isMainDoc" argument to 'TraverseDocument()', whose only purpose was to guard a call to 'ClearCache()'. Remove 'CAdblockPlusClient::IsElementHidden()'. It's only behavior over the filter function of the same name was some unnecessary locking. Change constructor argument for traverser classes from 'CPluginTab' to 'CPluginFilter'. These classes were only accessing the filter value. Renamed the externally-visible version of 'TraverseDocument()' to 'TraverseDocumentRoot()'. Remove 'TraverseHeader()' and 'TraverseSubdocument()', which were never called. Remove 'm_isHeaderTraversed', now not assigned to anything after initialization. Remove traverser members 'm_cacheDomElementCount' and 'm_pBrowser', neither of which was referenced. -------- Other reviews depend on this one: Load filters on "download begin" event https://codereview.adblockplus.org/29334397/

Patch Set 1 #

Patch Set 2 : rebase only #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -121 lines) Patch
M src/plugin/AdblockPlusClient.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 chunk +0 lines, -11 lines 0 comments Download
M src/plugin/AdblockPlusDomTraverser.h View 2 chunks +1 line, -4 lines 0 comments Download
M src/plugin/AdblockPlusDomTraverser.cpp View 3 chunks +3 lines, -4 lines 0 comments Download
M src/plugin/PluginDomTraverserBase.h View 11 chunks +24 lines, -80 lines 0 comments Download
M src/plugin/PluginFilter.h View 1 chunk +1 line, -10 lines 0 comments Download
M src/plugin/PluginFilter.cpp View 1 1 chunk +1 line, -5 lines 0 comments Download
M src/plugin/PluginTabBase.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/PluginTabBase.cpp View 3 chunks +2 lines, -5 lines 0 comments Download


Total messages: 6
This is the first of two change sets fixing #2230. This one only contains changes ...
Jan. 22, 2016, 6:17 p.m. (2016-01-22 18:17:18 UTC) #1
Working on CSS injection I have observed that some local changes are already partially made ...
Feb. 26, 2016, 1:24 p.m. (2016-02-26 13:24:58 UTC) #2
I disagree that we can just remove cachiing here. It is very important on XHR-heavy ...
May 2, 2016, 12:48 p.m. (2016-05-02 12:48:45 UTC) #3
Patch set 2 is rebase only. I'm not sure there are actual changes; if not, ...
May 17, 2016, 7:55 p.m. (2016-05-17 19:55:52 UTC) #4
On 2016/05/02 12:48:45, Oleksandr wrote: > I disagree that we can just remove caching here. ...
May 17, 2016, 9:16 p.m. (2016-05-17 21:16:16 UTC) #5
May 17, 2016, 9:38 p.m. (2016-05-17 21:38:09 UTC) #6
On 2016/02/26 13:24:58, sergei wrote:
> Could you please split it into several
> reviews, so I can base CSS injection on them?

I'm not sure the splitting you're asking for is possible. Many of the splits
you're asking about are for changes that look simple in the review tool, but are
not correct taken in isolation. I have to think through this again to know
what's possible in this regard.

> 5.[...] In addition it seems can address #264.

Making the cache a separate class will assist in that.

> 6.[depends on #4] change the constructor argument of `CPluginDomTraverser` and
> `CPluginDomTraverserBase` to `CPluginFilter*`. Actually, I would rather choose
> `std::shared_ptr<CPluginFilter>` here instead of raw pointer because it can
> happen that `CPluginFilter` is modified by initializer thread.

That's not a good solution. It pushes a lifetime dependency into could that has
no need to know about how the filter is initialized.

There are two possible behaviors if we need to use the filter and it's not
available. If we want to block while filter initialization completes, we can do
that. If we want to not block, we can simply skip filtering altogether. I've
been assuming the correct behavior is to filter, since that's what the plugin
does. That means we need to block somehow. It's better to perform blocking
separately and then just use a plain pointer.

The current code has a completely crazy was of doing that blocking. The detached
initialization class I wrote in another review provides a much better way with
good locality of reference.

(There's actually another possible behavior. With a task queue, let the queue
wake up a blocking task after filter initialization completes. I consider this
approach beyond the scope of incremental progress at this point. Too much to
clean up first.)

Powered by Google App Engine
This is Rietveld