1419780 - nsIconProtocolHandler parses a URI twice unnecessarily

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Valentin Gosu [:valentin] (he/him)]

https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/image/decoders/icon/nsIconProtocolHandler.cpp#71,74,80-81

There is no need to call SetSpec twice. It is better to make nsMozIconURI conditionally implement nsINestedURI.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1419780 - nsIconProtocolHandler parses a URI twice unnecessarily

Review commit: https://reviewboard.mozilla.org/r/202040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202040/

Comment 2

[:Gijs (out for now; he/him)]

Comment on attachment 8930955 [details]
Bug 1419780 - nsIconProtocolHandler parses a URI twice unnecessarily

https://reviewboard.mozilla.org/r/202040/#review207528

While this looks OK to me, I'm not a peer for image/decoders/ stuff, and this style of XPCOM/C++ change is also not my forte, so it'd be good if :tn or someone else more familiar with this code could also review it.

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8930955 [details]
Bug 1419780 - nsIconProtocolHandler parses a URI twice unnecessarily

https://reviewboard.mozilla.org/r/202040/#review207632

::: image/decoders/icon/nsIconURI.cpp:753
(Diff revision 1)
>    if (iconURL) {
>      iconURL.forget(aURI);
>    } else {
>      *aURI = nullptr;
>    }
>    return NS_OK;

According to the nsINestedURI header if we return null then we shouldn't return success. We probably don't ever hit that case though because QIing to nsINestedURI is conditional on mIconURL.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> Comment on attachment 8930955 [details]
> According to the nsINestedURI header if we return null then we shouldn't
> return success. We probably don't ever hit that case though because QIing to
> nsINestedURI is conditional on mIconURL.

Good point. Thanks for the review.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8930955 [details]
Bug 1419780 - nsIconProtocolHandler parses a URI twice unnecessarily

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202040/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c18f19fe1788
nsIconProtocolHandler parses a URI twice unnecessarily r=Gijs,tnikkel

Comment 7

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/c18f19fe1788