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!
Created attachment 252154 [details] Patch
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?
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.
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.
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)");
http://trac.webkit.org/changeset/183677 I'll change it to use ASCIILiteral soon.
(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?
(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.