Currently, there are two backend initializers with identical source implementations. They can easily be replaced by a single template function.
Created attachment 81175 [details] Patch There is a bogus failure in the style checker. It complains "Use 0 instead of NULL" [readability/null], although the word "NULL" appears in a comment only.
Attachment 81175 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/KURLGooglePrivate.h:59: Use 0 instead of NULL. [readability/null] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Filed bug 53755 against check-webkit-style.
(In reply to comment #1) > Created an attachment (id=81175) [details] > Patch > > There is a bogus failure in the style checker. It complains "Use 0 instead of NULL" [readability/null], although the word "NULL" appears in a comment only. This is by design and not a bogus failure.
Created attachment 81258 [details] Patch (In reply to comment #4) > This is by design and not a bogus failure. Ok. I am resubmitting the patch, with the comments updated. check-webkit-style is now passing. I replaced "NULL" with "0", "'\0'" or "null" as appropriate. I kept using the (dictionary word) "null" because it makes sense to refer to the "null string", and also because it has semantic meaning in the underlying classes (e.g. CString::isNull). In fact, before this change, the spellings "null" and "NULL" were used interchangeably.
I'm not a super template instantiation expert. Normally you'd need to define the function in the header so all the callers for the different types see the same thing. That seems to not be necessary in this case because there are only two possible instantiations, and they're both instantiated from the .cpp file where the actual implementation is. Maybe a real WebKit reviewer has an idea for if there is a better method more commonly used in WebKit for this kind of thing. But this looks good to me.
Comment on attachment 81258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81258&action=review Can you post the style cleanups in a separate patch? It's difficult to see what the actual code change is in this patch. > Source/WebCore/platform/KURLGoogle.cpp:66 > - // The encoding parameter may be NULL, but in this case the object must not > + // The encoding parameter may be null, but in this case the object must not NULL => 0
(In reply to comment #7) > Can you post the style cleanups in a separate patch? It's difficult to see what the actual code change is in this patch. Done. It's bug 54228. I saw that you noticed it already. > > Source/WebCore/platform/KURLGoogle.cpp:66 > > - // The encoding parameter may be NULL, but in this case the object must not > > + // The encoding parameter may be null, but in this case the object must not > > NULL => 0 Also done. I removed null from everywhere except for comments that concern URLs and strings.
Comment on attachment 81258 [details] Patch r- as Adam mentioned re separating out the massive style change.
(In reply to comment #9) > r- as Adam mentioned re separating out the massive style change. I will prepare another patch as soon as Adam (or someone else) reviews the style fix in bug 54228.
Created attachment 82085 [details] Patch
Comment on attachment 82085 [details] Patch Ah. Looks great.
> Maybe a real WebKit reviewer has an idea for if there is a better method more commonly used in WebKit for this kind of thing. But this looks good to me. We usually put the body of the templated function in the header, but if this works, its fine. We can always move the function later if we need to.
Created attachment 82099 [details] Reworked patch (In reply to comment #13) > We usually put the body of the templated function in the header, but if this works, its fine. We can always move the function later if we need to. Thank you for the r+, Adam, but now I'm thinking: Would it not be better to clearly indicate that the template is only a helper, not to be called from the outside (hence no template body in the header), and name it the same way in which googleurl internal helpers are named? I renamed the template helper to doInit, and I added a comment in KURLGooglePrivate.h about its internal-only use. Should it ever need to be used from outside, then it can be renamed back to init, and moved to the header. I also fixed another bunch of comment typos along the way.
Attachment 82099 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 82100 [details] Reworked patch Oops, clicked on the wrong file name to attach, by mistake.
Comment on attachment 82100 [details] Reworked patch doInit isn't the correct name for this function. WebKit functions never start with the word "do". It's almost always redundant. I liked the previous patch better.
Created attachment 82103 [details] Patch (In reply to comment #17) > doInit isn't the correct name for this function. WebKit functions never start with the word "do". It's almost always redundant. I liked the previous patch better. I understand. I'm attaching the old patch, but with the comment fixes in.
Created attachment 82104 [details] Patch Oops, another typo (this one was introduced by me). It's getting late...
Comment on attachment 82104 [details] Patch Clearing flags on attachment: 82104 Committed r78319: <http://trac.webkit.org/changeset/78319>
All reviewed patches have been landed. Closing bug.