Bug 53749 - Templatize KURLGooglePrivate::init
Summary: Templatize KURLGooglePrivate::init
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Enhancement
Assignee: Cosmin Truta
URL:
Keywords:
Depends on: 54228
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-03 19:32 PST by Cosmin Truta
Modified: 2011-02-11 00:14 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.81 KB, patch)
2011-02-03 19:46 PST, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (10.41 KB, patch)
2011-02-04 11:47 PST, Cosmin Truta
levin: review-
Details | Formatted Diff | Diff
Patch (3.80 KB, patch)
2011-02-10 18:43 PST, Cosmin Truta
abarth: review+
Details | Formatted Diff | Diff
Reworked patch (468 bytes, patch)
2011-02-10 21:35 PST, Cosmin Truta
no flags Details | Formatted Diff | Diff
Reworked patch (6.99 KB, patch)
2011-02-10 21:46 PST, Cosmin Truta
abarth: review-
Details | Formatted Diff | Diff
Patch (5.78 KB, patch)
2011-02-10 22:21 PST, Cosmin Truta
abarth: review+
abarth: commit-queue+
Details | Formatted Diff | Diff
Patch (5.78 KB, patch)
2011-02-10 22:25 PST, Cosmin Truta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2011-02-03 19:32:47 PST
Currently, there are two backend initializers with identical source implementations. They can easily be replaced by a single template function.
Comment 1 Cosmin Truta 2011-02-03 19:46:04 PST
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.
Comment 2 WebKit Review Bot 2011-02-03 20:33:00 PST
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.
Comment 3 Cosmin Truta 2011-02-03 21:11:33 PST
Filed bug 53755 against check-webkit-style.
Comment 4 David Levin 2011-02-04 09:45:11 PST
(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.
Comment 5 Cosmin Truta 2011-02-04 11:47:11 PST
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.
Comment 6 Brett Wilson (Google) 2011-02-09 22:57:17 PST
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 7 Adam Barth 2011-02-10 01:30:10 PST
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
Comment 8 Cosmin Truta 2011-02-10 13:58:38 PST
(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 9 David Levin 2011-02-10 14:38:40 PST
Comment on attachment 81258 [details]
Patch

r- as Adam mentioned re separating out the massive style change.
Comment 10 Cosmin Truta 2011-02-10 14:45:23 PST
(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.
Comment 11 Cosmin Truta 2011-02-10 18:43:42 PST
Created attachment 82085 [details]
Patch
Comment 12 Adam Barth 2011-02-10 20:48:01 PST
Comment on attachment 82085 [details]
Patch

Ah.  Looks great.
Comment 13 Adam Barth 2011-02-10 20:49:18 PST
> 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.
Comment 14 Cosmin Truta 2011-02-10 21:35:08 PST
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.
Comment 15 WebKit Review Bot 2011-02-10 21:37:00 PST
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.
Comment 16 Cosmin Truta 2011-02-10 21:46:32 PST
Created attachment 82100 [details]
Reworked patch

Oops, clicked on the wrong file name to attach, by mistake.
Comment 17 Adam Barth 2011-02-10 22:06:25 PST
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.
Comment 18 Cosmin Truta 2011-02-10 22:21:23 PST
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.
Comment 19 Cosmin Truta 2011-02-10 22:25:47 PST
Created attachment 82104 [details]
Patch

Oops, another typo (this one was introduced by me). It's getting late...
Comment 20 WebKit Commit Bot 2011-02-11 00:14:24 PST
Comment on attachment 82104 [details]
Patch

Clearing flags on attachment: 82104

Committed r78319: <http://trac.webkit.org/changeset/78319>
Comment 21 WebKit Commit Bot 2011-02-11 00:14:29 PST
All reviewed patches have been landed.  Closing bug.