Bug 59143

Summary: Need populate touch-icon url to FrameLoaderClient
Product: WebKit Reporter: michaelbai
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, ap, beidson, buildbot, commit-queue, ddkilzer, dglazkov, eric, gustavo.noronha, gustavo, steveblock, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Initial implementation
ddkilzer: review-
Using TOUCH_ICON_LOADING to enable feature, Disabled by default.
none
Sync
none
Fix style
none
Fix chromium build
none
Remove the chromium implementation, make patch a little smaller and easy to review.
ddkilzer: review-
Address all comments
none
Fix style issue
ddkilzer: review-
Address the comments
ddkilzer: review+, ddkilzer: commit-queue-
Try to fix the build error
none
Address the comment
none
Try to fix the build error again
none
Try to fix the build error again none

Description michaelbai 2011-04-21 15:02:00 PDT
This is a feature request

More and more websites were using below tag to provide different icon.

 <link rel="apple-touch-icon" href="somepath/image.png" />
 <link rel="apple-touch-icon-precomposed" href="somepath/image.png" />

To get touch icon url in FrameLoaderClient, we need handle above tags and populate the urls to the FrameLoaderClient, so client could handle them as needed.

I had a patch for this feature.
Comment 1 michaelbai 2011-04-26 08:55:07 PDT
Created attachment 91114 [details]
Initial implementation
Comment 2 David Kilzer (:ddkilzer) 2011-04-26 09:51:31 PDT
Comment on attachment 91114 [details]
Initial implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=91114&action=review

r- to correct issues noted above.

Also, this change seems like it will load all icons on all page loads in all browsers.  This isn't correct since Desktop browsers (at least Safari) is not going to want to try to load the "touch" icon variants on every page load.  MobileSafari on iOS currently only loads the "touch" icons when the user saves the URL (or web app) to the Home screen, not on every page load.  You need a way to enable/disable certain icons loading (and have "touch" icons off by default) before you land this.

Also, please make sure WebKit still compiles with ENABLE(ICON_DATABASE) disabled.  Thanks!

> Source/WebCore/dom/Document.cpp:355
> +        default:
> +            return -1;

There should not be a default case here.  Having one defeats the compiler from warning you when you don't have a case defined.  This return statement should be moved outside of the switch statement.

This should also be an (inline) method in IconURL.{h|cpp} so that you can share code between Document.cpp and DocumentLoader.cpp.

This method should really return size_t (which is used for array indexes), and then default to Favicon if 'type' is unknown and ASSERT_NOT_REACHED() before returning a default value.

> Source/WebCore/dom/Document.cpp:4384
> +    if (index != -1)
> +        return m_iconURLs[index];
> +    else
> +        return IconURL();

Should use early return here:

    if (index == -1)
        return IconURL();
    return m_iconURLs[index];

> Source/WebCore/dom/Document.h:1338
> +    IconURL m_iconURLs[3];

"3" should not be hard-coded here.  You should use the IconType enum to define this, or add some kind of compiler assertion, or define a const value in IconURL.h that is next to the IconType enum.

> Source/WebCore/loader/DocumentLoader.cpp:92
> +static int convertIconTypeToIndex(IconType type) {
> +    switch(type) {
> +        case Favicon:
> +            return 0;
> +        case TouchPrecomposedIcon:
> +            return 1;
> +        case TouchIcon:
> +            return 2;
> +        default:
> +            return -1;
> +    }
> +}

Duplicate code.  Please use the method added to IconURL.h.

> Source/WebCore/loader/DocumentLoader.cpp:684
> +    if (index != -1) {
> +        return m_iconURLs[index];
> +    } else {
> +        return IconURL();
> +    }

Please use early return.  Unnecessary curly braces.  (Please run check-webkit-style on your changed files.)

> Source/WebCore/loader/DocumentLoader.h:304
> +        IconURL m_iconURLs[3];

"3" should be a constant defined in IconURL.h (as noted for Document.cpp).

> Source/WebCore/loader/FrameLoader.cpp:525
> -    url.setPath("/favicon.ico");
> -    return url;
> +    if (iconType == Favicon) {
> +        url.setPath("/TouchPrecomposedIcon.ico");

Why did this change from "/favicon.ico" to "/TouchPrecomposedIcon.ico"?  I don't think this is correct.
Comment 3 michaelbai 2011-04-26 10:25:17 PDT
Hi David,

Thanks very much for the quick response.

I have some issues want to be discussed before I make another patch.

a. Is it OK that using 'SUPPORT_TOUCH_ICON' to disable the touch icon loading?

b. Please refer to my inline reply for convertIconTypeToIndex.


Tao

(In reply to comment #2)
> (From update of attachment 91114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91114&action=review
> 
> r- to correct issues noted above.
> 
> Also, this change seems like it will load all icons on all page loads in all browsers.  This isn't correct since Desktop browsers (at least Safari) is not going to want to try to load the "touch" icon variants on every page load.  MobileSafari on iOS currently only loads the "touch" icons when the user saves the URL (or web app) to the Home screen, not on every page load.  You need a way to enable/disable certain icons loading (and have "touch" icons off by default) before you land this.
> 
> Also, please make sure WebKit still compiles with ENABLE(ICON_DATABASE) disabled.  Thanks!
> 
> > Source/WebCore/dom/Document.cpp:355
> > +        default:
> > +            return -1;
> 
> There should not be a default case here.  Having one defeats the compiler from warning you when you don't have a case defined.  This return statement should be moved outside of the switch statement.
> 
> This should also be an (inline) method in IconURL.{h|cpp} so that you can share code between Document.cpp and DocumentLoader.cpp.
> 
> This method should really return size_t (which is used for array indexes), and then default to Favicon if 'type' is unknown and ASSERT_NOT_REACHED() before returning a default value.
> 
> > Source/WebCore/dom/Document.cpp:4384
> > +    if (index != -1)
> > +        return m_iconURLs[index];
> > +    else
> > +        return IconURL();
> 
> Should use early return here:
> 
>     if (index == -1)
>         return IconURL();
>     return m_iconURLs[index];
> 
> > Source/WebCore/dom/Document.h:1338
> > +    IconURL m_iconURLs[3];
> 
> "3" should not be hard-coded here.  You should use the IconType enum to define this, or add some kind of compiler assertion, or define a const value in IconURL.h that is next to the IconType enum.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:92
> > +static int convertIconTypeToIndex(IconType type) {
> > +    switch(type) {
> > +        case Favicon:
> > +            return 0;
> > +        case TouchPrecomposedIcon:
> > +            return 1;
> > +        case TouchIcon:
> > +            return 2;
> > +        default:
> > +            return -1;
> > +    }
> > +}
> 
> Duplicate code.  Please use the method added to IconURL.h.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:684
> > +    if (index != -1) {
> > +        return m_iconURLs[index];
> > +    } else {
> > +        return IconURL();
> > +    }
> 
> Please use early return.  Unnecessary curly braces.  (Please run check-webkit-style on your changed files.)
> 
> > Source/WebCore/loader/DocumentLoader.h:304
> > +        IconURL m_iconURLs[3];
> 
> "3" should be a constant defined in IconURL.h (as noted for Document.cpp).
> 
> > Source/WebCore/loader/FrameLoader.cpp:525
> > -    url.setPath("/favicon.ico");
> > -    return url;
> > +    if (iconType == Favicon) {
> > +        url.setPath("/TouchPrecomposedIcon.ico");
> 
> Why did this change from "/favicon.ico" to "/TouchPrecomposedIcon.ico"?  I don't think this is correct.
Comment 4 michaelbai 2011-04-26 10:28:31 PDT
The inline comment didn't show up, copied it here.

Hi David,

Thanks very much for the quick response.

I have some issues want to be discussed before I make another patch.

a. Is it OK that using 'SUPPORT_TOUCH_ICON' to disable the touch icon loading?

b. Please refer to my inline reply for convertIconTypeToIndex.


Tao

> 
> > Source/WebCore/dom/Document.cpp:355
> > +        default:
> > +            return -1;
> 
> There should not be a default case here.  Having one defeats the compiler from warning you when you don't have a case defined.  This return statement should be moved outside of the switch statement.
> 
> This should also be an (inline) method in IconURL.{h|cpp} so that you can share code between Document.cpp and DocumentLoader.cpp.
> 
> This method should really return size_t (which is used for array indexes), and then default to Favicon if 'type' is unknown and ASSERT_NOT_REACHED() before returning a default value.
> 

The index of a array is the implementation specific thing, and this code might not need to be shared. To put this method in IconURL looks a little weird, isn't it? Anyway, I am not familiar with Webkit convention, If you think it is fine, I am also OK with it. Please let me know.

Does ASSERT_NOT_REACHED work in debug build? Shall we return 0 (Favicon) in release build if 'type' is invalid?
Comment 5 David Kilzer (:ddkilzer) 2011-04-26 11:27:28 PDT
(In reply to comment #4)
> The inline comment didn't show up, copied it here.
> 
> Hi David,
> 
> Thanks very much for the quick response.
> 
> I have some issues want to be discussed before I make another patch.
> 
> a. Is it OK that using 'SUPPORT_TOUCH_ICON' to disable the touch icon loading?

It's not so much about support as it is whether you want to load the touch icons during a page load.  Maybe ENABLE(TOUCH_ICON_LOADING)?

> b. Please refer to my inline reply for convertIconTypeToIndex.
> 
> 
> Tao
> 
> > 
> > > Source/WebCore/dom/Document.cpp:355
> > > +        default:
> > > +            return -1;
> > 
> > There should not be a default case here.  Having one defeats the compiler from warning you when you don't have a case defined.  This return statement should be moved outside of the switch statement.
> > 
> > This should also be an (inline) method in IconURL.{h|cpp} so that you can share code between Document.cpp and DocumentLoader.cpp.
> > 
> > This method should really return size_t (which is used for array indexes), and then default to Favicon if 'type' is unknown and ASSERT_NOT_REACHED() before returning a default value.
> > 
> 
> The index of a array is the implementation specific thing, and this code might not need to be shared. To put this method in IconURL looks a little weird, isn't it? Anyway, I am not familiar with Webkit convention, If you think it is fine, I am also OK with it. Please let me know.
> 
> Does ASSERT_NOT_REACHED work in debug build? Shall we return 0 (Favicon) in release build if 'type' is invalid?

ASSERT_NOT_REACHED() will crash a Debug build, but do nothing on Release builds.  Returning Favicon sounds good (since that was the behavior before adding the touch icon support).
Comment 6 michaelbai 2011-04-26 17:27:44 PDT
Created attachment 91193 [details]
Using TOUCH_ICON_LOADING to enable feature, Disabled by default.
Comment 7 michaelbai 2011-04-27 08:41:23 PDT
Comment on attachment 91193 [details]
Using TOUCH_ICON_LOADING to enable feature, Disabled by default.

Compilation passed with ICONDATABASE disabled.
Comment 8 michaelbai 2011-04-27 13:28:22 PDT
Hi David,

Please help to review it again.
Comment 9 michaelbai 2011-04-28 09:31:20 PDT
Created attachment 91503 [details]
Sync
Comment 10 WebKit Review Bot 2011-04-28 09:35:08 PDT
Attachment 91503 [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

Last 3072 characters of output:
(  [whitespace/parens] [5]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:59:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:58:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:75:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:76:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:81:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:84:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:85:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/IconURL.h:69:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/loader/FrameLoader.h:437:  The parameter name "iconType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/FrameLoader.h:438:  The parameter name "iconType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/FrameLoader.cpp:476:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/loader/FrameLoader.cpp:499:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/loader/FrameLoader.cpp:514:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/loader/FrameLoader.cpp:526:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 38 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 michaelbai 2011-04-28 10:12:04 PDT
Created attachment 91510 [details]
Fix style
Comment 12 WebKit Review Bot 2011-04-28 10:46:51 PDT
Attachment 91510 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8513823
Comment 13 michaelbai 2011-04-28 11:01:56 PDT
Created attachment 91518 [details]
Fix chromium build
Comment 14 michaelbai 2011-05-03 09:09:57 PDT
Created attachment 92082 [details]
Remove the chromium implementation, make patch a little smaller and easy to review.
Comment 15 David Kilzer (:ddkilzer) 2011-05-03 10:57:50 PDT
Comment on attachment 92082 [details]
Remove the chromium implementation, make patch a little smaller and easy to review.

View in context: https://bugs.webkit.org/attachment.cgi?id=92082&action=review

r- to fix the FeatureDefines.xcconfig issue, remove the conflict markers, renaming/sharing getIconIndex() in IconURL.h, fix indentation issues and fix "didGot" variable names.  I think one more patch should do it!

> Source/WebCore/ChangeLog:7154
> +>>>>>>> .r84917

Please remove the conflict marker.

> Source/WebCore/Configurations/FeatureDefines.xcconfig:120
> +ENABLE_TOUCH_ICON_LOADING = ;

Changes to FeatureDefines.xcconfig need to be copied to all four locations:

JavaScriptCore/Configurations/FeatureDefines.xcconfig
WebCore/Configurations/FeatureDefines.xcconfig
WebKit/mac/Configurations/FeatureDefines.xcconfig
WebKit2/Configurations/FeatureDefines.xcconfig

> Source/WebCore/dom/Document.cpp:341
> +static size_t getIconIndex(IconType type)

Nit: I would call this toIconIndex() instead of getIconIndex() since it's a conversion method, not a true "getter".

> Source/WebCore/dom/Document.cpp:347
> +        break;

Nit: You could use early returns here as well, but it's not necessary.

> Source/WebCore/dom/Document.cpp:4400
> +    if (!iconURL(iconType).m_iconURL.isValid())

Why did this change from an isEmpty() check to isValid() check?

> Source/WebCore/loader/DocumentLoader.cpp:82
> +static size_t getIconIndex(IconType type)

Nit: This should also be named toIconIndex().

This should go in IconURL.h since it's much closer to IconType and ICON_COUNT, and then you can share the implementation in DocumentLoader.cpp and Document.cpp.  There's no need to duplicate this code.

I wouldn't worry about the name of the file being "IconURL.h" unless you can come up with a better one.

> Source/WebCore/loader/DocumentLoader.cpp:697
> +            m_iconURLs[getIconIndex(url.m_iconType)] = url;

Weird indentation here.  Please fix.

> Source/WebCore/loader/FrameLoader.cpp:479
> +            iconURLs.append(getDefaultIconURL(Favicon));

Weird indentation here.  Please fix.

> Source/WebCore/loader/FrameLoader.cpp:482
> +    bool didGotPrecomposedIcon = false;

"didGot" should be changed to "have" in didGotPrecomposedIcon and didGotTouchIcon.

> Source/WebCore/loader/FrameLoader.cpp:499
> +bool FrameLoader::fillIconURL(IconType iconType, Vector<IconURL>* iconURLs)

Nit: addIconURL() seems better than fillIconUURL(), but the current name is fine.

> Source/WebKit/chromium/ChangeLog:637
> +>>>>>>> .r84917

Please remove the conflict marker.

> Source/WebKit/mac/ChangeLog:378
> +>>>>>>> .r84917

Please remove the conflict marker.
Comment 16 michaelbai 2011-05-03 12:18:33 PDT
Created attachment 92102 [details]
Address all comments
Comment 17 WebKit Review Bot 2011-05-03 12:21:50 PDT
Attachment 92102 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/Configurations/Featu..." exit_code: 1

Source/WebCore/dom/IconURL.h:75:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 michaelbai 2011-05-03 13:01:31 PDT
Created attachment 92110 [details]
Fix style issue
Comment 19 David Kilzer (:ddkilzer) 2011-05-03 13:25:02 PDT
Comment on attachment 92110 [details]
Fix style issue

View in context: https://bugs.webkit.org/attachment.cgi?id=92110&action=review

r- to update/add ChangeLog entries, fix FeatureDefines.xcconfig sort order (just ENABLE_TOUCH_ICON_LOADING), move toIconIndex out of struct IconURL and into the WebCore namespace, and use Vector<IconURL, ICON_COUNT> to limit the capacity of the vector when it's first created.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:120
> +ENABLE_TOUCH_ICON_LOADING = ;

Need an entry in JavaScriptCore/ChangeLog for the change to JavaScriptCore/Configurations/FeatureDefines.xcconfig.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:136
> +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_WEBGL) $(ENABLE_3D_RENDERING) $(ENABLE_ANIMATION_API) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATALIST) $(ENABLE_DATA_TRANSFER_ITEMS) $(ENABLE_DETAILS) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PAGE_VISIBILITY_API) $(ENABLE_PROGRESS_TAG) $(ENABLE_REGISTER_PROTOCOL_HANDLER) $(ENABLE_QUOTA) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_VIDEO_TRACK) $(ENABLE_MEDIA_STATISTICS) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT) $(ENABLE_TOUCH_ICON_LOADING);

Please add $(ENABLE_TOUCH_ICON_LOADING) alphabetically after $(ENABLE_SVG_USE) like (most) of the rest of the items.

> Source/WebCore/dom/IconURL.h:68
> +    static size_t toIconIndex(IconType type)

Nit:  You didn't have to add the method to struct IconURL--you could have added it to the WebCore namespace as a stand-alone method.  That's what we do for similar conversion methods.  I see now why you didn't think it made sense to add this method "to IconURL".

> Source/WebCore/loader/FrameLoader.cpp:477
> +    Vector<IconURL> iconURLs;

I believe you could use "Vector<IconURL, ICON_COUNT>" here to hint that you only need a capacity of ICON_COUNT allocated.

> Source/WebCore/loader/FrameLoader.h:241
> +    Vector<IconURL> iconURLs(int iconTypes);

I believe you could use "Vector<IconURL, ICON_COUNT>" here to hint that you only need a capacity of ICON_COUNT allocated.

> Source/WebKit2/ChangeLog:14
> +        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
> +        (WebKit::WebFrameLoaderClient::dispatchDidChangeIcons):
> +        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:

Need to update the WebKit2/ChangeLog to list the changes to Configurations/FeatureDefines.xcconfig.

> Source/WebKit/mac/ChangeLog:13
> +        * WebCoreSupport/WebFrameLoaderClient.h:
> +        * WebCoreSupport/WebFrameLoaderClient.mm:
> +        (WebFrameLoaderClient::dispatchDidChangeIcons):

Need to update WebKit/mac/ChangeLog to list the changes to Configurations/FeatureDefines.xcconfig.
Comment 20 michaelbai 2011-05-03 15:34:43 PDT
Created attachment 92149 [details]
Address the comments
Comment 21 Early Warning System Bot 2011-05-03 15:59:45 PDT
Attachment 92149 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8534501
Comment 22 David Kilzer (:ddkilzer) 2011-05-03 16:11:44 PDT
Comment on attachment 92149 [details]
Address the comments

View in context: https://bugs.webkit.org/attachment.cgi?id=92149&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        Respect the interface change in FrameLoaderClient.

This comment doesn't make any sense.  Just remove it.

> Source/WebCore/dom/IconURL.h:67
> +typedef Vector<IconURL, ICON_COUNT> IconURLs;

Nice!
Comment 23 David Kilzer (:ddkilzer) 2011-05-03 16:12:06 PDT
(In reply to comment #22)
> (From update of attachment 92149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92149&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Respect the interface change in FrameLoaderClient.
> 
> This comment doesn't make any sense.  Just remove it.
> 
> > Source/WebCore/dom/IconURL.h:67
> > +typedef Vector<IconURL, ICON_COUNT> IconURLs;
> 
> Nice!

r=me assuming you fix the build failures.
Comment 24 michaelbai 2011-05-03 16:46:19 PDT
Created attachment 92166 [details]
Try to fix the build error
Comment 25 michaelbai 2011-05-03 16:57:20 PDT
Created attachment 92170 [details]
Address the comment
Comment 26 David Kilzer (:ddkilzer) 2011-05-03 17:09:12 PDT
(In reply to comment #25)
> Created an attachment (id=92170) [details]
> Address the comment

I'll let the ews bots chew on this and check in on it later.
Comment 27 Early Warning System Bot 2011-05-03 17:21:09 PDT
Attachment 92170 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8546016
Comment 28 Build Bot 2011-05-03 22:31:57 PDT
Attachment 92170 [details] did not build on win:
Build output: http://queues.webkit.org/results/8557177
Comment 29 Collabora GTK+ EWS bot 2011-05-04 04:47:09 PDT
Attachment 92170 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8557488
Comment 30 michaelbai 2011-05-04 08:50:51 PDT
Created attachment 92250 [details]
Try to fix the build error again
Comment 31 michaelbai 2011-05-04 08:57:20 PDT
Created attachment 92252 [details]
Try to fix the build error again
Comment 32 Eric Seidel (no email) 2011-05-04 09:51:55 PDT
Comment on attachment 92252 [details]
Try to fix the build error again

Rejecting attachment 92252 [details] from commit-queue.

michaelbai@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 33 Collabora GTK+ EWS bot 2011-05-04 13:51:36 PDT
Attachment 92252 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8556672
Comment 34 WebKit Commit Bot 2011-05-04 14:00:24 PDT
The commit-queue encountered the following flaky tests while processing attachment 92252 [details]:

fast/history/history-subframe-with-name.html bug 51039 (author: mihaip@chromium.org)
The commit-queue is continuing to process your patch.
Comment 35 WebKit Commit Bot 2011-05-04 14:03:06 PDT
Comment on attachment 92252 [details]
Try to fix the build error again

Clearing flags on attachment: 92252

Committed r85785: <http://trac.webkit.org/changeset/85785>
Comment 36 WebKit Commit Bot 2011-05-04 14:03:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 David Kilzer (:ddkilzer) 2011-05-04 14:30:00 PDT
(In reply to comment #33)
> Attachment 92252 [details] did not build on gtk:
> Build output: http://queues.webkit.org/results/8556672

See also:
http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/13405

autoreconf: Entering directory `.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal -I Source/autotools 
autoreconf: configure.ac: tracing
autoreconf: running: libtoolize --copy
libtoolize: Consider adding `-I Source/autotools' to ACLOCAL_AMFLAGS in Makefile.am.
autoreconf: running: /usr/bin/autoconf --include=Source/autotools
autoreconf: running: /usr/bin/autoheader --include=Source/autotools
autoreconf: running: automake --add-missing --copy --no-force
Source/WebCore/GNUmakefile.am:266: ENABLE_TOUCH_ICON_LOADING does not appear in AM_CONDITIONAL
GNUmakefile.am:193:   `Source/WebCore/GNUmakefile.am' included from here
autoreconf: automake failed with exit status: 1
Failed to setup build environment using 'autotools'!
Comment 38 WebKit Review Bot 2011-05-04 15:07:51 PDT
http://trac.webkit.org/changeset/85785 might have broken GTK Linux 32-bit Release