Summary: | Dead code in LinkIconCollector.cpp | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | karl <karl+webkit> | ||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, bdakin, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, thorton, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=157435 | ||||||||
Attachments: |
|
Description
karl
2021-12-13 04:12:25 PST
Created attachment 447084 [details]
Patch
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. 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. Created attachment 447104 [details]
Patch
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 on attachment 447104 [details]
Patch
Is there a way to write a test?
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 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. |