Bug 53749 – Templatize KURLGooglePrivate::init

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Cosmin Truta]

Currently, there are two backend initializers with identical source implementations. They can easily be replaced by a single template function.

[Cosmin Truta]

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.

[WebKit Review Bot]

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.

[Cosmin Truta]

Filed bug 53755 against check-webkit-style.

[David Levin]

(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.

[Cosmin Truta]

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.

[Brett Wilson (Google)]

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.

[Adam Barth]

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

[Cosmin Truta]

(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.

[David Levin]

Comment on attachment 81258 [details]
Patch

r- as Adam mentioned re separating out the massive style change.

[Cosmin Truta]

(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.

[Cosmin Truta]

Created attachment 82085 [details]
Patch

[Adam Barth]

Comment on attachment 82085 [details]
Patch

Ah.  Looks great.

[Adam Barth]

> 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.

[Cosmin Truta]

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.

[WebKit Review Bot]

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.

[Cosmin Truta]

Created attachment 82100 [details]
Reworked patch

Oops, clicked on the wrong file name to attach, by mistake.

[Adam Barth]

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.

[Cosmin Truta]

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.

[Cosmin Truta]

Created attachment 82104 [details]
Patch

Oops, another typo (this one was introduced by me). It's getting late...

[WebKit Commit Bot]

Comment on attachment 82104 [details]
Patch

Clearing flags on attachment: 82104

Committed r78319: <http://trac.webkit.org/changeset/78319>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.