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

Issue 29332440: TEST CODE - Replace frame/iframe cache sets with boolean (Closed)

Created:
Dec. 7, 2015, 5:29 p.m. by Eric
Modified:
Dec. 14, 2015, 1:44 p.m.
Reviewers:
sergei, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

TEST CODE - Replace frame/iframe cache sets with boolean Remove 'CPluginDomTraverserBase::m_documentName', which was never explicitly initialized. It was effectively a constant whose value was the empty string. Replace set 'm_cacheDocumentHasIframes' with boolean 'cacheHasIframes' and likewise for frames. The only value ever inserted into these sets was 'm_documentName', that is, the empty string, so these sets only existed in one of two states: (1) empty (replaced by the value 'false'), and (2) a set containing exactly one element, the empty string (replaced by the value 'true'). Initialize the booleans to true upon construction. Replace the 'insert()' operation with an assignment of 'true'. Replace the 'clear()' operation with an assignment of 'false'. Replace the 'find()' operation with the value of the replacement boolean value. -------- Depends upon the following review: https://codereview.adblockplus.org/29332057/

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -11 lines) Patch
M src/plugin/PluginDomTraverserBase.h View 7 chunks +11 lines, -11 lines 0 comments Download

Messages

Total messages: 3
Eric
I've made the original subject "TEST CODE" because I am slightly incredulous that this actually ...
Dec. 7, 2015, 5:36 p.m. (2015-12-07 17:36:13 UTC) #1
Oleksandr
We shouldn't remove the m_documentName. The fact that it wasn't use is a bug, but ...
Dec. 14, 2015, 10:09 a.m. (2015-12-14 10:09:09 UTC) #2
Eric
Dec. 14, 2015, 1:44 p.m. (2015-12-14 13:44:13 UTC) #3
On 2015/12/14 10:09:09, Oleksandr wrote:
> We shouldn't remove the m_documentName. The fact that it wasn't use is a bug,
> but there already a proposed fix here:
> https://codereview.adblockplus.org/29331669/. It is used for the Matches call
> from the traverser.

OK.

I have to say, though, that those cache sets still look kind of suspicious to
me. Let me repeat my suggestion to split up that review so we can commit the
documentUrl parts quickly. I think I'd like to revisit them once they might
actually do something. I didn't touch the synchronization around the cache sets
in this review, but it's not clear to me that they're necessary.

Closing this review as "defect corrected in another way".

Powered by Google App Engine
This is Rietveld