Bug 60247

Created attachment 92469 [details] Initial implementation

Comment on attachment 92469 [details] Initial implementation View in context: https://bugs.webkit.org/attachment.cgi?id=92469&action=review Here's a few comments. Since this involves the chromium api though Darin should really review it. He may also say things that change what I suggested a little bit in some places so I won't mark this r- so he'll look it over. > Source/WebKit/chromium/ChangeLog:8 > + * WebKit.gyp: Typically you should include a small note for each function or file about why a change was done there (or minimally what was done). > Source/WebKit/chromium/public/WebIconURL.h:40 > + Favicon = 1, Inconsistent casing seems like it should be FavIcon. Also why not make this "= 1 << 0" for consistency as well. > Source/WebKit/chromium/public/WebIconURL.h:57 > + WebIconType m_iconType; Typical WebKit style is to make a class with private member variables and m_ prefixes or a struct with public member variables and no m_ prefix. > Source/WebKit/chromium/src/WebFrameImpl.cpp:533 > + webIconURLs[i] = (WebIconURL(iconURLs[i].m_iconURL, WebIconTypeUtilities::ToWebIconType(iconURLs[i].m_iconType))); Why have the outer parens? > Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:50 > + default: I would recommend not having a default and handling every case explicitly so that the compiler will catch missing enums in this switch if any are added. In addition, just return the type directly from the case and then after the switch statement have an ASSERT_NOT_REACHED(); > Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:69 > + default: Ditto. > Source/WebKit/chromium/src/WebIconTypeUtilities.h:39 > +class WebIconTypeUtilities { Is Utilities the common name for this in type of functionality in this directory? It looks like a Converter more than Utilities. Also, I wonder if we can make the value align exactly and then do the conversion using a cast. The fragileness of this approach would be overcome by using COMPILE_ASSERTs. > Source/WebKit/chromium/src/WebIconTypeUtilities.h:40 > + public: No indent on public: > Source/WebKit/chromium/src/WebIconTypeUtilities.h:41 > + static WebCore::IconType ToIconType(WebIconType); Naming doesn't follow WebKit style -- should be toIconType. > Source/WebKit/chromium/src/WebIconTypeUtilities.h:42 > + static WebIconType ToWebIconType(WebCore::IconType); Ditto. > Source/WebKit/chromium/src/WebIconTypeUtilities.h:43 > + static int ToIconTypes(int); Ditto. Also there should be a variable name here. The variable name is only left out when it is obviously redundant with the information already on the line. This is not the case for the input "int".

(In reply to comment #2) > (From update of attachment 92469 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92469&action=review > > Here's a few comments. Since this involves the chromium api though Darin should really review it. He may also say things that change what I suggested a little bit in some places so I won't mark this r- so he'll look it over. > > > Source/WebKit/chromium/ChangeLog:8 > > + * WebKit.gyp: > > Typically you should include a small note for each function or file about why a change was done there (or minimally what was done). > Done > > Source/WebKit/chromium/public/WebIconURL.h:40 > > + Favicon = 1, > > Inconsistent casing seems like it should be FavIcon. > > Also why not make this "= 1 << 0" for consistency as well. > I think Favicon is a term now. It is also used in IconType.h and chromium. > > Source/WebKit/chromium/public/WebIconURL.h:57 > > + WebIconType m_iconType; > > Typical WebKit style is to make a class with private member variables and m_ prefixes or a struct with public member variables and no m_ prefix. > Done > > Source/WebKit/chromium/src/WebFrameImpl.cpp:533 > > + webIconURLs[i] = (WebIconURL(iconURLs[i].m_iconURL, WebIconTypeUtilities::ToWebIconType(iconURLs[i].m_iconType))); > > Why have the outer parens? > Done > > Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:50 > > + default: > > I would recommend not having a default and handling every case explicitly so that the compiler will catch missing enums in this switch if any are added. > > In addition, just return the type directly from the case and then after the switch statement have an ASSERT_NOT_REACHED(); > Done > > Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:69 > > + default: > > Ditto. > Done > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:39 > > +class WebIconTypeUtilities { > > Is Utilities the common name for this in type of functionality in this directory? It looks like a Converter more than Utilities. > > Also, I wonder if we can make the value align exactly and then do the conversion using a cast. > > The fragileness of this approach would be overcome by using COMPILE_ASSERTs. > I would prefer use the switch ... case, it prevents the missing match of icon type definition. It might also the reason we have WebXXX leayer. > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:40 > > + public: > > No indent on public: > Done > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:41 > > + static WebCore::IconType ToIconType(WebIconType); > > Naming doesn't follow WebKit style -- should be toIconType. > > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:42 > > + static WebIconType ToWebIconType(WebCore::IconType); > > Ditto. > Done > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:43 > > + static int ToIconTypes(int); > > Ditto. Also there should be a variable name here. The variable name is only left out when it is obviously redundant with the information already on the line. This is not the case for the input "int". Done, How do I suppress the style check error, I used to have the variable name here.

Created attachment 92594 [details] Address all comments

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 92469 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=92469&action=review > > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:43 > > > + static int ToIconTypes(int); > > > > Ditto. Also there should be a variable name here. The variable name is only left out when it is obviously redundant with the information already on the line. This is not the case for the input "int". > > Done, How do I suppress the style check error, I used to have the variable name here. No style error :) (The style checker looks for name overlapping with the type given. That doesn't happen here. It also does some special checks for "set" functions but that isn't the case here either.) You likely got style errors on the other lines where the variable name was redundant with the type.

(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 92469 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=92469&action=review > > > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:43 > > > > + static int ToIconTypes(int); > > > > > > Ditto. Also there should be a variable name here. The variable name is only left out when it is obviously redundant with the information already on the line. This is not the case for the input "int". > > > > Done, How do I suppress the style check error, I used to have the variable name here. > > No style error :) (The style checker looks for name overlapping with the type given. That doesn't happen here. It also does some special checks for "set" functions but that isn't the case here either.) > > You likely got style errors on the other lines where the variable name was redundant with the type. You are right. It passed the check. I got the style error in different line.

Comment on attachment 92594 [details] Address all comments View in context: https://bugs.webkit.org/attachment.cgi?id=92594&action=review > Source/WebKit/chromium/public/WebFrame.h:135 > + virtual WebVector<WebIconURL> favIconURL(int) const = 0; what is this magic int parameter? > Source/WebKit/chromium/public/WebIconURL.h:38 > +enum WebIconType { please create a separate header file for each toplevel type: class, struct or enum. please also follow the naming conventions for enums. enum Foo { FooA, FooB, ... }; > Source/WebKit/chromium/public/WebIconURL.h:57 > + WebIconType iconType; do these fields need to be mutable? maybe RIIA would be sufficient? > Source/WebKit/chromium/src/WebIconTypeUtilities.h:39 > +class WebIconTypeUtilities { utilities classes tend to become dumping grounds. can you instead give this a more specific name? it seems to be all about type conversion. have you considered making the api types have exactly the same values as the webcore ones? that could simplify conversions. see AssertMatchingEnums.cpp.

Created attachment 92643 [details] Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM.

(In reply to comment #7) > (From update of attachment 92594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92594&action=review > > > Source/WebKit/chromium/public/WebFrame.h:135 > > + virtual WebVector<WebIconURL> favIconURL(int) const = 0; > > what is this magic int parameter? > It is the combination of IconTypes, added a comment > > Source/WebKit/chromium/public/WebIconURL.h:38 > > +enum WebIconType { > > please create a separate header file for each toplevel type: class, struct or enum. > > please also follow the naming conventions for enums. enum Foo { FooA, FooB, ... }; > I moved the WebIconType into WebIconURL as it will only used by WebIconURL. I didn't add the prefix as to access the emun value has to be used WebIconURL::. Hope it is fine. > > Source/WebKit/chromium/public/WebIconURL.h:57 > > + WebIconType iconType; > > do these fields need to be mutable? maybe RIIA would be sufficient? > Sorry, I don't know RIIA stand for, I guess you want to use const WebIconTyp iconType. If so, I can not assign the WebIconURL to WebVector in WebFrameImpl::favIcon(int). > > Source/WebKit/chromium/src/WebIconTypeUtilities.h:39 > > +class WebIconTypeUtilities { > > utilities classes tend to become dumping grounds. can you instead give this a more specific name? it seems to be all about type conversion. > > have you considered making the api types have exactly the same values as the webcore ones? that could simplify conversions. see AssertMatchingEnums.cpp. Removed WebIconTypeUtilities, Used AssertMatchingEnums

Created attachment 92644 [details] Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM.

Comment on attachment 92644 [details] Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM. Attachment 92644 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8610110

Created attachment 92657 [details] Fix build

Created attachment 92804 [details] Sync, Update change log, fix the style issus

Comment on attachment 92804 [details] Sync, Update change log, fix the style issus View in context: https://bugs.webkit.org/attachment.cgi?id=92804&action=review Mostly following up Darin's comments. > Source/WebKit/chromium/public/WebIconURL.h:43 > + TouchPrecomposedIcon = 1 << 2 Even though this is inside of the struct, I look at a number of other classes in WebKit/chromium/public and they prefix the name with the enum, so it would be WebIconTypeInvalid, etc. > Source/WebKit/chromium/public/WebIconURL.h:57 > + WebIconType iconType; I suspect Darin meant RAII and I believe he was suggesting making these member variables private (and making this a class essentially with m_ variables and accesssors for the values where needed), so the only place that the object become immutable (except for the copy operator, which is used in WebFrameImpl::favIconURL). > Source/WebKit/chromium/src/WebFrameImpl.cpp:529 > + WTF::Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes); I doubt that you need the WTF:: prefix.

Created attachment 92819 [details] Address the comments

(In reply to comment #15) > Created an attachment (id=92819) [details] > Address the comments Since the enum type is now prefixed with WebIconType, why post fix it with icon? (which makes it more verbose and clumsy). I was trying to indicate this with my comment in which I constructed one of the names: WebIconTypeInvalid

Created attachment 92823 [details] Address the comments I left the favicon as WebIconTypeFavicon as favicon is a term.

Comment on attachment 92823 [details] Address the comments View in context: https://bugs.webkit.org/attachment.cgi?id=92823&action=review > Source/WebKit/chromium/public/WebIconURL.h:38 > +struct WebIconURL { Last thing (Sorry for not noticing in the last revision), but this should be a class.

Created attachment 92836 [details] Address the comment Done, thanks very much

Comment on attachment 92836 [details] Address the comment Clearing flags on attachment: 92836 Committed r86091: <http://trac.webkit.org/changeset/86091>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/86091 might have broken Chromium Win Release

It looks like this line has problems: WTF::Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes); Specifically: FrameLoader::iconURLs doesn't seem to exist. Also I think you can get rid of WTF:: from this line.

Comment on attachment 92836 [details] Address the comment View in context: https://bugs.webkit.org/attachment.cgi?id=92836&action=review > Source/WebKit/chromium/public/WebFrame.h:135 > + virtual WebVector<WebIconURL> favIconURL(int iconTypes) const = 0; you should explain what this mysterious int parameter is. it is not obviously a bit-mask of Type enum values. you should say so! > Source/WebKit/chromium/public/WebIconURL.h:40 > + enum WebIconType { because this enum is nested inside a class, it does not need the redundant WebIcon prefix. if you look around at other headers, you will see that this should just be enum Type { TypeInvalid = 0, ... }; > Source/WebKit/chromium/src/WebFrameImpl.cpp:529 > + WTF::Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes); no WTF:: in .cpp files

Created attachment 92961 [details] Address the comments and Rollback previous favIconURL() definition.

Attachment 92961 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/src/WebFrameImpl.cpp:526: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 92962 [details] Fix style

Created attachment 93022 [details] Fix style

Comment on attachment 93022 [details] Fix style View in context: https://bugs.webkit.org/attachment.cgi?id=93022&action=review > Source/WebKit/chromium/public/WebFrame.h:137 > + // the document loaded in this frame. The iconTypes could be a bit-mask of Sorry for all the back-n-forth on this. Being out on vacation has meant that I haven't had the time to give this the focus I should. Thanks for putting up with all the cycles. nit: "The iconTypes [is] a bit-mask of WebIconURL::Type values, used to select from the available set of icon URLs." nit: Since this method returns possibly many WebIconURL objects, please pluralize the method name. It should be favIconURLs instead of favIconURL. nit: Since this method returns an array of WebIconURL and not WebFavIconURL, the method should not have the "Fav" part in its name. I think it should just be iconURLs. > Source/WebKit/chromium/public/WebFrameClient.h:206 > + virtual void didChangeIcons(WebFrame*, WebIconURL::Type) { } I think this method name could be improved. Since it is only reporting the change of a single icon type, it should not be pluralized. It should just be didChangeIcon. > Source/WebKit/chromium/public/WebIconURL.h:67 > + you can define a constructor here that takes a WebCore::IconURL object. wrap the constructor in #if WEBKIT_IMPLEMENTATION so that it will not be visible to consumers of the API (we don't want them to see WebCore stuff). with the above constructor, it is now possible to implicitly convert WTF::Vector<WebCore::IconURL> to WebVector<WebIconURL>. Magic! > Source/WebKit/chromium/src/WebFrameImpl.cpp:531 > +WebVector<WebIconURL> WebFrameImpl::favIconURL(int webIconTypes) const nit: parameter name should just be iconTypes. > Source/WebKit/chromium/src/WebFrameImpl.cpp:537 > + Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes); with the constructor on WebIconURL that i mentioned, you can condense this down to: return frameLoader->iconURLs(iconTypes);

Created attachment 93062 [details] Address the comment

Comment on attachment 93062 [details] Address the comment Attachment 93062 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8689133

Created attachment 93122 [details] Fix build

Created attachment 93147 [details] Revert the method in WebFrameClient.h to make the transient easy.

Comment on attachment 93147 [details] Revert the method in WebFrameClient.h to make the transient easy. View in context: https://bugs.webkit.org/attachment.cgi?id=93147&action=review > Source/WebKit/chromium/WebKit.gyp:188 > + 'public/WebIconType.h', wrong file? > Source/WebKit/chromium/public/WebFrame.h:138 > + // WebIconURL::Type values, sed to select from the available set of icon "sed" -> used? > Source/WebKit/chromium/public/WebIconURL.h:62 > + WebIconURL(const WebCore::IconURL& iconURL) nit: we usually place any WEBKIT_IMPLEMENTATION section as the very end of the public section. take a look at other header files to familiarize yourself with conventions. > Source/WebKit/chromium/src/WebFrameImpl.cpp:537 > + Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes); were you not able to replace all of this code with a simple "return frameLoader->iconURLs(...);" ^^^ that was the point of defining the constructor

Created attachment 93180 [details] Address the comment

(In reply to comment #34) > (From update of attachment 93147 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93147&action=review > > > Source/WebKit/chromium/WebKit.gyp:188 > > + 'public/WebIconType.h', > > wrong file? > Done. > > Source/WebKit/chromium/public/WebFrame.h:138 > > + // WebIconURL::Type values, sed to select from the available set of icon > > "sed" -> used? > Done. > > Source/WebKit/chromium/public/WebIconURL.h:62 > > + WebIconURL(const WebCore::IconURL& iconURL) > > nit: we usually place any WEBKIT_IMPLEMENTATION section as the very end of the public section. take a look at other header files to familiarize yourself with conventions. > Sorry, I am anxious to get this check in. > > Source/WebKit/chromium/src/WebFrameImpl.cpp:537 > > + Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes); > > were you not able to replace all of this code with a simple "return frameLoader->iconURLs(...);" > > ^^^ that was the point of defining the constructor Not aware of the magic here.

Comment on attachment 93180 [details] Address the comment View in context: https://bugs.webkit.org/attachment.cgi?id=93180&action=review > Source/WebKit/chromium/WebKit.gyp:188 > + 'public/WebIconType.h', did you maybe upload the wrong version of the patch?

Created attachment 93193 [details] Correct patch

I am so sorry, I uploaded wrong patch. Just a kind reminder, please help the review chromium patch, before put this in the commit queue, otherwise the build will be failed. (In reply to comment #37) > (From update of attachment 93180 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93180&action=review > > > Source/WebKit/chromium/WebKit.gyp:188 > > + 'public/WebIconType.h', > > did you maybe upload the wrong version of the patch?

Comment on attachment 93193 [details] Correct patch View in context: https://bugs.webkit.org/attachment.cgi?id=93193&action=review > Source/WebKit/chromium/ChangeLog:9 > + Added a parameter to favIconURL() to specify the type of icon need to this is a bit stale. it should also mention the method renaming. > Source/WebKit/chromium/public/WebFrame.h:133 > + // This method is deprecated and will be removed soon. nit: we usually format comments like this for easy discovery later. // DEPRECATED: Use iconIRLs instead. > Source/WebKit/chromium/public/WebFrameClient.h:205 > + // This method is deprecated and will be removed soon. same nit... // DEPRECATED: Implement didChangeIcon instead. > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:764 > + // Keep the API work in the transient. nit: this a good place to insert FIXME, again for easy discovery later. > Source/WebKit/chromium/src/WebFrameImpl.cpp:531 > +WebVector<WebIconURL> WebFrameImpl::iconURLs(int webIconTypes) const nit: param should just be called iconTypes as it is declared in the header.

Created attachment 93298 [details] Final?

Comment on attachment 93298 [details] Final? View in context: https://bugs.webkit.org/attachment.cgi?id=93298&action=review > Source/WebKit/chromium/public/WebFrame.h:140 > + virtual WebVector<WebIconURL> iconURLs(int iconTypes) const = 0; your chromium patch, which forks a copy of WebIconURL, would be unnecessary if you made this return an empty vector.

Created attachment 93457 [details] Implement iconURLs to make transient easy

Attachment 93457 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebFrame.h:140: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 93461 [details] Fix style

Comment on attachment 93461 [details] Fix style Attachment 93461 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8693222

Created attachment 93474 [details] Fix build

The commit-queue encountered the following flaky tests while processing attachment 93474 [details]: http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 93474 [details] Fix build Clearing flags on attachment: 93474 Committed r86452: <http://trac.webkit.org/changeset/86452>

All reviewed patches have been landed. Closing bug.