[webkit-reviews] review denied: [Bug 59143] Need populate touch-icon url to FrameLoaderClient : [Attachment 92110] Fix style issue

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied
michaelbai at chromium.org's request for review:
Bug 59143: Need populate touch-icon url to FrameLoaderClient
https://bugs.webkit.org/show_bug.cgi?id=59143

Attachment 92110: Fix style issue
https://bugs.webkit.org/attachment.cgi?id=92110&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.