RESOLVED FIXED 60247
Handle the touch icon in WebKit/Chromium
https://bugs.webkit.org/show_bug.cgi?id=60247
Summary Handle the touch icon in WebKit/Chromium
michaelbai
Reported 2011-05-04 22:25:37 PDT
Follow the patch https://bugs.webkit.org/show_bug.cgi?id=59143, handle the touch icon in WebKit/chromium
Attachments
Initial implementation (15.36 KB, patch)
2011-05-05 14:34 PDT, michaelbai
no flags
Address all comments (15.56 KB, patch)
2011-05-06 09:57 PDT, michaelbai
fishd: review-
Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM. (11.17 KB, application/octet-stream)
2011-05-06 14:40 PDT, michaelbai
no flags
Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM. (11.17 KB, patch)
2011-05-06 14:47 PDT, michaelbai
webkit.review.bot: commit-queue-
Fix build (10.16 KB, patch)
2011-05-06 16:37 PDT, michaelbai
no flags
Sync, Update change log, fix the style issus (10.68 KB, patch)
2011-05-09 10:42 PDT, michaelbai
levin: review-
Address the comments (10.94 KB, patch)
2011-05-09 11:57 PDT, michaelbai
no flags
Address the comments (10.92 KB, patch)
2011-05-09 12:14 PDT, michaelbai
levin: review-
Address the comment (10.91 KB, patch)
2011-05-09 13:08 PDT, michaelbai
fishd: review-
Address the comments and Rollback previous favIconURL() definition. (11.33 KB, patch)
2011-05-10 09:00 PDT, michaelbai
no flags
Fix style (11.32 KB, patch)
2011-05-10 09:06 PDT, michaelbai
no flags
Fix style (11.27 KB, patch)
2011-05-10 15:11 PDT, michaelbai
fishd: review-
Address the comment (11.60 KB, patch)
2011-05-10 19:12 PDT, michaelbai
webkit.review.bot: commit-queue-
Fix build (11.60 KB, patch)
2011-05-11 08:37 PDT, michaelbai
no flags
Revert the method in WebFrameClient.h to make the transient easy. (11.93 KB, patch)
2011-05-11 11:10 PDT, michaelbai
fishd: review-
Address the comment (11.93 KB, patch)
2011-05-11 13:59 PDT, michaelbai
fishd: review-
Correct patch (11.85 KB, patch)
2011-05-11 15:16 PDT, michaelbai
fishd: review+
fishd: commit-queue-
Final? (12.16 KB, patch)
2011-05-12 10:17 PDT, michaelbai
no flags
Implement iconURLs to make transient easy (deleted)
2011-05-13 09:02 PDT, michaelbai
no flags
Fix style (12.32 KB, patch)
2011-05-13 09:14 PDT, michaelbai
webkit.review.bot: commit-queue-
Fix build (12.41 KB, patch)
2011-05-13 10:11 PDT, michaelbai
no flags
michaelbai
Comment 1 2011-05-05 14:34:19 PDT
Created attachment 92469 [details] Initial implementation
David Levin
Comment 2 2011-05-05 18:09:50 PDT
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".
michaelbai
Comment 3 2011-05-06 09:57:15 PDT
(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.
michaelbai
Comment 4 2011-05-06 09:57:51 PDT
Created attachment 92594 [details] Address all comments
David Levin
Comment 5 2011-05-06 10:10:18 PDT
(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.
michaelbai
Comment 6 2011-05-06 10:44:18 PDT
(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.
Darin Fisher (:fishd, Google)
Comment 7 2011-05-06 11:58:37 PDT
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.
michaelbai
Comment 8 2011-05-06 14:40:57 PDT
Created attachment 92643 [details] Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM.
michaelbai
Comment 9 2011-05-06 14:46:03 PDT
(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
michaelbai
Comment 10 2011-05-06 14:47:44 PDT
Created attachment 92644 [details] Removed the utitilies class, used COMPILE_ASSERT_MATCHING_ENUM.
WebKit Review Bot
Comment 11 2011-05-06 15:32:45 PDT
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
michaelbai
Comment 12 2011-05-06 16:37:26 PDT
Created attachment 92657 [details] Fix build
michaelbai
Comment 13 2011-05-09 10:42:13 PDT
Created attachment 92804 [details] Sync, Update change log, fix the style issus
David Levin
Comment 14 2011-05-09 10:59:40 PDT
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.
michaelbai
Comment 15 2011-05-09 11:57:02 PDT
Created attachment 92819 [details] Address the comments
David Levin
Comment 16 2011-05-09 12:02:48 PDT
(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
michaelbai
Comment 17 2011-05-09 12:14:19 PDT
Created attachment 92823 [details] Address the comments I left the favicon as WebIconTypeFavicon as favicon is a term.
David Levin
Comment 18 2011-05-09 12:49:20 PDT
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.
michaelbai
Comment 19 2011-05-09 13:08:39 PDT
Created attachment 92836 [details] Address the comment Done, thanks very much
WebKit Commit Bot
Comment 20 2011-05-09 15:04:45 PDT
Comment on attachment 92836 [details] Address the comment Clearing flags on attachment: 92836 Committed r86091: <http://trac.webkit.org/changeset/86091>
WebKit Commit Bot
Comment 21 2011-05-09 15:04:51 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 22 2011-05-09 15:21:50 PDT
http://trac.webkit.org/changeset/86091 might have broken Chromium Win Release
David Levin
Comment 23 2011-05-09 15:30:39 PDT
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.
Darin Fisher (:fishd, Google)
Comment 24 2011-05-10 08:23:22 PDT
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
michaelbai
Comment 25 2011-05-10 09:00:57 PDT
Created attachment 92961 [details] Address the comments and Rollback previous favIconURL() definition.
WebKit Review Bot
Comment 26 2011-05-10 09:03:29 PDT
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.
michaelbai
Comment 27 2011-05-10 09:06:12 PDT
Created attachment 92962 [details] Fix style
michaelbai
Comment 28 2011-05-10 15:11:25 PDT
Created attachment 93022 [details] Fix style
Darin Fisher (:fishd, Google)
Comment 29 2011-05-10 18:34:19 PDT
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);
michaelbai
Comment 30 2011-05-10 19:12:47 PDT
Created attachment 93062 [details] Address the comment
WebKit Review Bot
Comment 31 2011-05-10 20:03:46 PDT
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
michaelbai
Comment 32 2011-05-11 08:37:12 PDT
Created attachment 93122 [details] Fix build
michaelbai
Comment 33 2011-05-11 11:10:08 PDT
Created attachment 93147 [details] Revert the method in WebFrameClient.h to make the transient easy.
Darin Fisher (:fishd, Google)
Comment 34 2011-05-11 13:25:46 PDT
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
michaelbai
Comment 35 2011-05-11 13:59:53 PDT
Created attachment 93180 [details] Address the comment
michaelbai
Comment 36 2011-05-11 14:04:08 PDT
(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.
Darin Fisher (:fishd, Google)
Comment 37 2011-05-11 15:10:52 PDT
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?
michaelbai
Comment 38 2011-05-11 15:16:23 PDT
Created attachment 93193 [details] Correct patch
michaelbai
Comment 39 2011-05-11 15:18:13 PDT
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?
Darin Fisher (:fishd, Google)
Comment 40 2011-05-12 09:50:26 PDT
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.
michaelbai
Comment 41 2011-05-12 10:17:01 PDT
Darin Fisher (:fishd, Google)
Comment 42 2011-05-12 17:08:19 PDT
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.
michaelbai
Comment 43 2011-05-13 09:02:33 PDT
Created attachment 93457 [details] Implement iconURLs to make transient easy
WebKit Review Bot
Comment 44 2011-05-13 09:05:45 PDT
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.
michaelbai
Comment 45 2011-05-13 09:14:09 PDT
Created attachment 93461 [details] Fix style
WebKit Review Bot
Comment 46 2011-05-13 10:01:24 PDT
Comment on attachment 93461 [details] Fix style Attachment 93461 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8693222
michaelbai
Comment 47 2011-05-13 10:11:50 PDT
Created attachment 93474 [details] Fix build
WebKit Commit Bot
Comment 48 2011-05-13 12:27:41 PDT
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.
WebKit Commit Bot
Comment 49 2011-05-13 12:29:05 PDT
Comment on attachment 93474 [details] Fix build Clearing flags on attachment: 93474 Committed r86452: <http://trac.webkit.org/changeset/86452>
WebKit Commit Bot
Comment 50 2011-05-13 12:29:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.