Bug 144491 – [Content Extensions] Add CombinedURLFilters debugging code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

Changing CombinedURLFilters is hard because I can't see what's going on.  I add some debugging code, and I change std::pair to a struct with names of its members.  Much better!

[Alex Christensen]

Created attachment 252154 [details]
Patch

[Daniel Bates]

Comment on attachment 252154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252154&action=review

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:40
> +    std::unique_ptr<PrefixTreeVertex> child;

I take it the name child is more descriptive than vertex in this context?

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:54
> +    size_t size = sizeof(PrefixTreeNode)

Is this correct? Where is PrefixTreeNode defined?

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:75
>      

This is OK as-is. I would remove this empty line as I don't feel that it improves the readability of this function.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:77
> +    for (unsigned i = 0; i < depth * 2; ++i)
> +        builder.append(' ');

This is OK as-is. You are underutilizing the index variable i. We only use the variable depth for this loop and can take advantage of it being passed using copy-by-value to write this loop as:

while (depth--)
    builder.append("  ");

Notice I append a string of two space characters on each iteration.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:84
> +    

This is OK as-is. I would remove this empty line as I don't feel that it improves the readability of this function.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:99
> +        

Please remove the leading whitespace in this line.

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:262
> +                    const Term& term = edge.term;
> +                    unsigned newSubtreeStart = term.generateGraph(nfa, prefixEnd, edge.child->finalActions);

Can we inline the value of the local variable term on this line or make the name of the local variable more descriptive?

> Source/WebCore/contentextensions/Term.h:214
> +inline String quantifierToString(AtomQuantifier quantifier)

Is it necessary for this to return a String? It seems sufficient to return char.

> Source/WebCore/contentextensions/Term.h:228
> +inline String Term::toString() const

Is it necessary that this be inline?

[Daniel Bates]

Comment on attachment 252154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252154&action=review

> Source/WebCore/ChangeLog:25
> +        * contentextensions/CombinedURLFilters.cpp:
> +        (WebCore::ContentExtensions::recursiveMemoryUsed):
> +        (WebCore::ContentExtensions::CombinedURLFilters::memoryUsed):
> +        (WebCore::ContentExtensions::prefixTreeVertexToString):
> +        (WebCore::ContentExtensions::recursivePrint):
> +        (WebCore::ContentExtensions::CombinedURLFilters::print):
> +        (WebCore::ContentExtensions::CombinedURLFilters::addPattern):
> +        (WebCore::ContentExtensions::generateNFAForSubtree):
> +        (WebCore::ContentExtensions::CombinedURLFilters::processNFAs):
> +        * contentextensions/CombinedURLFilters.h:
> +        * contentextensions/NFA.cpp:
> +        (WebCore::ContentExtensions::NFA::memoryUsed):
> +        * contentextensions/NFA.h:
> +        * contentextensions/Term.h:
> +        (WebCore::ContentExtensions::quantifierToString):
> +        (WebCore::ContentExtensions::Term::toString):

For your consideration, I suggest that we add some per-function remarks to this change log entry. At the very least, I suggest adding the remark "Added." to functions that were added by this patch.

[Alex Christensen]

Comment on attachment 252154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252154&action=review

Thanks! Addressing comments:

>> Source/WebCore/contentextensions/CombinedURLFilters.cpp:54
>> +    size_t size = sizeof(PrefixTreeNode)
> 
> Is this correct? Where is PrefixTreeNode defined?

Should be PrefixTreeVertex

>> Source/WebCore/contentextensions/Term.h:214
>> +inline String quantifierToString(AtomQuantifier quantifier)
> 
> Is it necessary for this to return a String? It seems sufficient to return char.

Yes.  AtomQuantifier::One returns the empty string.  There is no empty char.

>> Source/WebCore/contentextensions/Term.h:228
>> +inline String Term::toString() const
> 
> Is it necessary that this be inline?

Yes, it's a function defined in a header.

[Daniel Bates]

Comment on attachment 252154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252154&action=review

> Source/WebCore/contentextensions/Term.h:232
> +        return "(Empty)";

We should use ASCIILiteral() to avoid computing the length of the string since this is string literal that only contains ASCII characters 

return ASCIILiteral("(Empty)");

> Source/WebCore/contentextensions/Term.h:234
> +        return "(Deleted)";

Similarly, we should write this as:

return ASCIILiteral("(Deleted)");

[Alex Christensen]

http://trac.webkit.org/changeset/183677

I'll change it to use ASCIILiteral soon.

[Daniel Bates]

(In reply to comment #4)
> >> Source/WebCore/contentextensions/Term.h:214
> >> +inline String quantifierToString(AtomQuantifier quantifier)
> > 
> > Is it necessary for this to return a String? It seems sufficient to return char.
> 
> Yes.  AtomQuantifier::One returns the empty string.  There is no empty char.

You're right!

> 
> >> Source/WebCore/contentextensions/Term.h:228
> >> +inline String Term::toString() const
> > 
> > Is it necessary that this be inline?
> 
> Yes, it's a function defined in a header.

Obviously. What I meant to ask is: can we move this function to an implementation file, say, create file Term.cpp, so that we do not inline it?

[Alex Christensen]

(In reply to comment #7)
> Obviously. What I meant to ask is: can we move this function to an
> implementation file, say, create file Term.cpp, so that we do not inline it?
Yes.  Term.cpp and DFANode.cpp need to be made.  See the fix me in DFA.cpp.