Bug 143306 – Streamline icon-related code, mostly unused

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Adler]

Streamline icon-related code, mostly unused

[Darin Adler]

Created attachment 249980 [details]
Patch

[Darin Adler]

Created attachment 249986 [details]
Patch

[Build Bot]

Comment on attachment 249986 [details]
Patch

Attachment 249986 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6456446019436544

New failing tests:
fast/dom/icon-url-list.html
fast/dom/icon-url-list-apple-touch.html

[Build Bot]

Created attachment 249990 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249986 [details]
Patch

Attachment 249986 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5134296171937792

New failing tests:
fast/dom/icon-url-list.html
fast/dom/icon-url-list-apple-touch.html

[Build Bot]

Created attachment 249996 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 249986 [details]
Patch

Oops, looks like I uploaded a partial patch *without* the changes to LayoutTests. That’s why tests are failing.

[Darin Adler]

Created attachment 250031 [details]
Patch

[Darin Adler]

Ready to review now. All the tests are passing now.

[Antti Koivisto]

Comment on attachment 250031 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        time, with some small exceptoins, icon logic is in the web browsers, not in WebKit.

typo "exceptoins"

> Source/WebCore/dom/IconURL.h:38
>  enum IconType {

enum class would be nice

> Source/WebCore/html/LinkRelAttribute.h:41
> +    bool isStyleSheet { false };

I have been wondering if we should use { false } or { }. So far I have stayed with the former though it is redundant.

[Antti Koivisto]

Comment on attachment 250031 [details]
Patch

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

> Source/WebCore/loader/icon/IconController.cpp:55
> +enum LinkElementSelector { AllLinkElements, LinkElementsWithMIMETypes };

enum class SelectLinkElements { All, WithMIMEType }?

[Darin Adler]

Comment on attachment 250031 [details]
Patch

OK. I’ll make some changes tonight before I land this.

[Darin Adler]

Comment on attachment 250031 [details]
Patch

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

>> Source/WebCore/dom/IconURL.h:38
>>  enum IconType {
> 
> enum class would be nice

This was a little inconvenient to do quickly. I’m going to wait on that for a later patch.

[Darin Adler]

Committed r182351: <http://trac.webkit.org/changeset/182351>

[Gyuyoung Kim]

(In reply to comment #14)
> Committed r182351: <http://trac.webkit.org/changeset/182351>

It looks all ports build have broken since r182351. I upload a fix to Bug 143408.