Bug 234234 – Dead code in LinkIconCollector.cpp

• Component: DOM
• Status: RESOLVED FIXED

[karl]

This seems wrong.
https://github.com/WebKit/WebKit/blob/102a6d5be26fa5eba220703239dda452a0940d0b/Source/WebCore/html/LinkIconCollector.cpp#L53-L57

    // Apple Touch icons always come first.
    if (a.type == LinkIconType::Favicon && b.type != LinkIconType::Favicon)
        return 1;
    if (a.type == LinkIconType::Favicon && b.type != LinkIconType::Favicon)
        return -1;

should probably be

    // Apple Touch icons always come first.
    if (a.type == LinkIconType::Favicon && b.type != LinkIconType::Favicon)
        return 1;
    if (b.type == LinkIconType::Favicon && a.type != LinkIconType::Favicon)
        return -1;

[karl]

Created attachment 447084 [details]
Patch

[Alex Christensen]

This may be right, but can it be tested?  How did you find this?
Also, we don't need to redo formatting in the change log with this.

[karl]

Alex,

> How did you find this?

I was in the process of reading the code because I had just opened 
https://bugzilla.mozilla.org/show_bug.cgi?id=1745680

And I was trying to understand how WebKit, Blink and Gecko were processing html link for icons. So I just casually reading the code when I stumbled upon this dead code. 

> can it be tested?

It can be probably tested, being my first ever patch to WebKit. I don't see any tests for compareIcons() 

The code was added 5+ years ago without tests.
https://github.com/WebKit/WebKit/commit/0148de572687b1981331e70d1135408ae8cac4f2#diff-f6e9e7480dfa766ab4c1f0b36526f8faf97018240ba256b2217236e479c43919R50-R56

If you have recommendations on where these tests should go, I can try.

> Also, we don't need to redo formatting in the change log with this.

Yes, my VSCode instance has probably reformatted the Changelog. I need to fix this. Is there a URL to a recommended vscode setup for the WebKit project. 

Thanks.

[karl]

Created attachment 447104 [details]
Patch

[karl]

Fixed the Changelog file.


PS: for someone reading this in the future.

WebKit/.vscode/settings.json

{
    "files.trimTrailingWhitespace": false,
    "files.trimFinalNewlines": false,
}

will do the trick.

[youenn fablet]

Comment on attachment 447104 [details]
Patch

Is there a way to write a test?

[Radar WebKit Bug Importer]

<rdar://problem/86718656>

[EWS]

Committed r287262 (245418@main): <https://commits.webkit.org/245418@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447104 [details].

[Darin Adler]

Comment on attachment 447104 [details]
Patch

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

> Source/WebCore/html/LinkIconCollector.cpp:56
> +    if (b.type == LinkIconType::Favicon && a.type != LinkIconType::Favicon)

Aside from the really good point that we need a test, I should mention that I have a style I personally prefer for this kind of function that makes it easier to get the logic right and harder for mistakes to hide. This is the style I would use if writing a new function:

    bool aIsAppleTouchIcon = a.type == <whatever>;
    bool bIsAppleTouchIcon = b.type == <whatever>;
    if (aIsAppleTouchIcon != bIsAppleTouchIcon)
        return aIsAppleTouchIcon < bIsAppleTouchIcon ? 1 : -1;

    auto aSize = iconSize(a);
    auto bSize = iconSize(b);
    if (aSize != bSize)
        return aSize < bSize ? 1 : -1;

    bool aIsPrecomposed = a.type == LinkIconType::TouchPrecomposedIcon;
    bool bIsPrecomposed = b.type == LinkIconType::TouchPrecomposedIcon;
    if (aIsPrecomposed != bIsPrecomposed)
        return aIsPrecomposed < bIsPrecomposed ? 1 : -1;

    return 0;

The only tricky part is making sure the "-1" and "1" are correct, which is best verified using tests.