Bug 59143 - Need populate touch-icon url to FrameLoaderClient
: Need populate touch-icon url to FrameLoaderClient
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-04-21 15:02 PST by
Modified: 2011-05-04 15:07 PST (History)


Attachments
Initial implementation (59.06 KB, patch)
2011-04-26 08:55 PST, michaelbai@chromium.org
ddkilzer: review-
Review Patch | Details | Formatted Diff | Diff
Using TOUCH_ICON_LOADING to enable feature, Disabled by default. (66.12 KB, patch)
2011-04-26 17:27 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Sync (66.08 KB, patch)
2011-04-28 09:31 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Fix style (65.80 KB, patch)
2011-04-28 10:12 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Fix chromium build (65.81 KB, patch)
2011-04-28 11:01 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Remove the chromium implementation, make patch a little smaller and easy to review. (51.05 KB, patch)
2011-05-03 09:09 PST, michaelbai@chromium.org
ddkilzer: review-
Review Patch | Details | Formatted Diff | Diff
Address all comments (58.14 KB, patch)
2011-05-03 12:18 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Fix style issue (58.13 KB, patch)
2011-05-03 13:01 PST, michaelbai@chromium.org
ddkilzer: review-
Review Patch | Details | Formatted Diff | Diff
Address the comments (62.21 KB, patch)
2011-05-03 15:34 PST, michaelbai@chromium.org
ddkilzer: review+
ddkilzer: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Try to fix the build error (65.36 KB, patch)
2011-05-03 16:46 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Address the comment (65.30 KB, patch)
2011-05-03 16:57 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Try to fix the build error again (66.14 KB, patch)
2011-05-04 08:50 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Try to fix the build error again (66.33 KB, patch)
2011-05-04 08:57 PST, michaelbai@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-21 15:02:00 PST
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 From 2011-04-26 08:55:07 PST -------
Created an attachment (id=91114) [details]
Initial implementation
------- Comment #2 From 2011-04-26 09:51:31 PST -------
(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 #3 From 2011-04-26 10:25:17 PST -------
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] [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 From 2011-04-26 10:28:31 PST -------
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 From 2011-04-26 11:27:28 PST -------
(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 From 2011-04-26 17:27:44 PST -------
Created an attachment (id=91193) [details]
Using TOUCH_ICON_LOADING to enable feature, Disabled by default.
------- Comment #7 From 2011-04-27 08:41:23 PST -------
(From update of attachment 91193 [details])
Compilation passed with ICONDATABASE disabled.
------- Comment #8 From 2011-04-27 13:28:22 PST -------
Hi David,

Please help to review it again.
------- Comment #9 From 2011-04-28 09:31:20 PST -------
Created an attachment (id=91503) [details]
Sync
------- Comment #10 From 2011-04-28 09:35:08 PST -------
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 From 2011-04-28 10:12:04 PST -------
Created an attachment (id=91510) [details]
Fix style
------- Comment #12 From 2011-04-28 10:46:51 PST -------
Attachment 91510 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8513823
------- Comment #13 From 2011-04-28 11:01:56 PST -------
Created an attachment (id=91518) [details]
Fix chromium build
------- Comment #14 From 2011-05-03 09:09:57 PST -------
Created an attachment (id=92082) [details]
Remove the chromium implementation, make patch a little smaller and easy to review.
------- Comment #15 From 2011-05-03 10:57:50 PST -------
(From update of attachment 92082 [details])
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 From 2011-05-03 12:18:33 PST -------
Created an attachment (id=92102) [details]
Address all comments
------- Comment #17 From 2011-05-03 12:21:50 PST -------
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 From 2011-05-03 13:01:31 PST -------
Created an attachment (id=92110) [details]
Fix style issue
------- Comment #19 From 2011-05-03 13:25:02 PST -------
(From update of attachment 92110 [details])
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 From 2011-05-03 15:34:43 PST -------
Created an attachment (id=92149) [details]
Address the comments
------- Comment #21 From 2011-05-03 15:59:45 PST -------
Attachment 92149 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8534501
------- Comment #22 From 2011-05-03 16:11:44 PST -------
(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!
------- Comment #23 From 2011-05-03 16:12:06 PST -------
(In reply to comment #22)
> (From update of attachment 92149 [details] [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 From 2011-05-03 16:46:19 PST -------
Created an attachment (id=92166) [details]
Try to fix the build error
------- Comment #25 From 2011-05-03 16:57:20 PST -------
Created an attachment (id=92170) [details]
Address the comment
------- Comment #26 From 2011-05-03 17:09:12 PST -------
(In reply to comment #25)
> Created an attachment (id=92170) [details] [details]
> Address the comment

I'll let the ews bots chew on this and check in on it later.
------- Comment #27 From 2011-05-03 17:21:09 PST -------
Attachment 92170 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8546016
------- Comment #28 From 2011-05-03 22:31:57 PST -------
Attachment 92170 [details] did not build on win:
Build output: http://queues.webkit.org/results/8557177
------- Comment #29 From 2011-05-04 04:47:09 PST -------
Attachment 92170 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8557488
------- Comment #30 From 2011-05-04 08:50:51 PST -------
Created an attachment (id=92250) [details]
Try to fix the build error again
------- Comment #31 From 2011-05-04 08:57:20 PST -------
Created an attachment (id=92252) [details]
Try to fix the build error again
------- Comment #32 From 2011-05-04 09:51:55 PST -------
(From update of attachment 92252 [details])
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 From 2011-05-04 13:51:36 PST -------
Attachment 92252 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8556672
------- Comment #34 From 2011-05-04 14:00:24 PST -------
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 From 2011-05-04 14:03:06 PST -------
(From update of attachment 92252 [details])
Clearing flags on attachment: 92252

Committed r85785: <http://trac.webkit.org/changeset/85785>
------- Comment #36 From 2011-05-04 14:03:15 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #37 From 2011-05-04 14:30:00 PST -------
(In reply to comment #33)
> Attachment 92252 [details] [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 From 2011-05-04 15:07:51 PST -------
http://trac.webkit.org/changeset/85785 might have broken GTK Linux 32-bit Release