[Webkit-unassigned] [Bug 234234] Dead code in LinkIconCollector.cpp

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=234234

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com

--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 447104
  --> https://bugs.webkit.org/attachment.cgi?id=447104
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211220/fd4f90ff/attachment.htm>