Bug 59143

Created attachment 91114 [details] Initial implementation

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.

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.

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?

(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).

Created attachment 91193 [details] Using TOUCH_ICON_LOADING to enable feature, Disabled by default.

Comment on attachment 91193 [details] Using TOUCH_ICON_LOADING to enable feature, Disabled by default. Compilation passed with ICONDATABASE disabled.

Hi David, Please help to review it again.

Created attachment 91503 [details] Sync

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.

Created attachment 91510 [details] Fix style

Attachment 91510 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8513823

Created attachment 91518 [details] Fix chromium build

Created attachment 92082 [details] Remove the chromium implementation, make patch a little smaller and easy to review.

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.

Created attachment 92102 [details] Address all comments

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.

Created attachment 92110 [details] Fix style issue

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.

Created attachment 92149 [details] Address the comments

Attachment 92149 [details] did not build on qt: Build output: http://queues.webkit.org/results/8534501

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!

(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.

Created attachment 92166 [details] Try to fix the build error

Created attachment 92170 [details] Address the comment

(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.

Attachment 92170 [details] did not build on qt: Build output: http://queues.webkit.org/results/8546016

Attachment 92170 [details] did not build on win: Build output: http://queues.webkit.org/results/8557177

Attachment 92170 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8557488

Created attachment 92250 [details] Try to fix the build error again

Created attachment 92252 [details] Try to fix the build error again

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.

Attachment 92252 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8556672

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 on attachment 92252 [details] Try to fix the build error again Clearing flags on attachment: 92252 Committed r85785: <http://trac.webkit.org/changeset/85785>

All reviewed patches have been landed. Closing bug.

(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'!

http://trac.webkit.org/changeset/85785 might have broken GTK Linux 32-bit Release