Bug 234234 - Dead code in LinkIconCollector.cpp
Summary: Dead code in LinkIconCollector.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-13 04:12 PST by karl
Modified: 2021-12-20 10:02 PST (History)
11 users (show)

See Also:


Attachments
Patch (201.75 KB, patch)
2021-12-13 16:37 PST, karl
no flags Details | Formatted Diff | Diff
Patch (1.39 KB, patch)
2021-12-13 20:36 PST, karl
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description karl 2021-12-13 04:12:25 PST
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;
Comment 1 karl 2021-12-13 16:37:24 PST
Created attachment 447084 [details]
Patch
Comment 2 Alex Christensen 2021-12-13 16:53:07 PST
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.
Comment 3 karl 2021-12-13 17:10:58 PST
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.
Comment 4 karl 2021-12-13 20:36:49 PST
Created attachment 447104 [details]
Patch
Comment 5 karl 2021-12-13 20:38:16 PST
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.
Comment 6 youenn fablet 2021-12-14 01:19:17 PST
Comment on attachment 447104 [details]
Patch

Is there a way to write a test?
Comment 7 Radar WebKit Bug Importer 2021-12-20 04:13:16 PST
<rdar://problem/86718656>
Comment 8 EWS 2021-12-20 08:24:30 PST
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].
Comment 9 Darin Adler 2021-12-20 10:02:10 PST
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.