1291283 - In TryToStartImageLoadOnValue, use URLValueData::GetLocalURLFlag() to identify local-ref URI

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

After bug 652991 land, we can simply use URLValueData::mLocalURLFlag flag to figure out whether a URL is a local-ref fragment.

Comment 1

[u459114]

Attachment: Bug 1291283 - Use URLValueData::GetLocalURLFlag() to identify local-ref URI.

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

Comment 2

[u459114]

hi dholbert,
With local-ref flag support in URLValueData[1], we can make code here simpler.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.h#124

Comment 3

[Daniel Holbert [:dholbert]]

https://reviewboard.mozilla.org/r/69304/#review66408

::: layout/style/nsCSSDataBlock.cpp
(Diff revision 1)
> -    // For example,
> +    if (urlValue->GetLocalURLFlag()) {
> -    //   mask-image: url(#mask_id); // refer to a SVG mask element, whose id is
> -    //                              // "mask_id", in the current document.
> -    // For such 'mask-image' values (pointing to an in-document element),
> -    // there is no need to trigger image download.
> -    if (aProperty == eCSSProperty_mask_image) {

Shouldn't we still keep this check (and the explanatory comment inside of it), to limit this special case to "mask-image"?

For other properties, e.g. background-image, I don't think we want to take this early-return...  I'd expect we should still try to download whatevertheURLis#foo and render that as an image. (right?)  (I think that's what happens right now.)

Comment 4

[u459114]

https://reviewboard.mozilla.org/r/69304/#review66408

> Shouldn't we still keep this check (and the explanatory comment inside of it), to limit this special case to "mask-image"?
> 
> For other properties, e.g. background-image, I don't think we want to take this early-return...  I'd expect we should still try to download whatevertheURLis#foo and render that as an image. (right?)  (I think that's what happens right now.)

1.
I think we may skip it since this new added if statement already cover the removed one and has better performance(calling URLValueData::GetLocalURLFlag() is O(1))

2.
URLValueData::GetLocalURLFlag() returns true only if the first non-"C0 controls + space" character is '#'[1].
So url(xxx#foo) is not a local-ref url, while url(#foo) is.

We will still download url(whatever#foo), but not for url(#foo). Even for bg-image, I thing we should still apply the same logic: no need to download a local resource(such as url(#foo)). Since that resouce is already embedded in the current document.

[1] http://searchfox.org/mozilla-central/source/layout/style/nsCSSValue.cpp#32

Comment 5

[u459114]

Comment on attachment 8777857 [details]
Bug 1291283 - Use URLValueData::GetLocalURLFlag() to identify local-ref URI.

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

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #4)
> 2.
> URLValueData::GetLocalURLFlag() returns true only if the first non-"C0
> controls + space" character is '#'[1].
> So url(xxx#foo) is not a local-ref url, while url(#foo) is.

Sorry, yeah -- I was talking about "background-image: url(#foo)", with "#" as the first character. (which should make us try to download http://the-page-url-goes-here#foo, i.e. the actual whole page, and see if it's possible to display as a background image.)

It's maybe unlikely that much (any?) web content actually depends on that behavior, but I think that's what's supposed to happen, for properties that simply take an image URL and don't understand things like local references.

> Since that resouce is already embedded in the current document.

background-image:url(#foo) doesn't point to a resource within the document in the same way that mask-image does, as I understand it...  (It's not interpreted that way, at least.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8777857 [details]
Bug 1291283 - Use URLValueData::GetLocalURLFlag() to identify local-ref URI.

https://reviewboard.mozilla.org/r/69304/#review66496

r=me with the following:

::: layout/style/nsCSSDataBlock.cpp:73
(Diff revision 2)
>      //                              // "mask_id", in the current document.
>      // For such 'mask-image' values (pointing to an in-document element),
>      // there is no need to trigger image download.
>      if (aProperty == eCSSProperty_mask_image) {
> -      nsIURI* docURI = aDocument->GetDocumentURI();
> +      // Filter out all fragment URLs.
> +      // Since nsCSSValue::GetURLStructValue runs much faster then

typo: s/then/than/

::: layout/style/nsCSSDataBlock.cpp:81
(Diff revision 2)
> +      URLValue* urlValue = aValue.GetURLStructValue();
> +      if (urlValue->GetLocalURLFlag()) {
> +        return;
> +      }
> +
> +      // Even though urlValue is not a fragment URL, it might still refers to

typo: s/refers/refer/

::: layout/style/nsCSSDataBlock.cpp:84
(Diff revision 2)
> +      }
> +
> +      // Even though urlValue is not a fragment URL, it might still refers to
> +      // an internal resource.
> +      // For example, aDocument base URL is "http://foo/index.html" and
> +      // intentionally defines mask-image by url(http://foo/index.html#mask)

"defines mask-image by" doesn't make sense to me.

I think this wants to say "references a mask-image at"?  Or perhaps "defines a mask-image at".

Comment 8

[u459114]

Comment on attachment 8777857 [details]
Bug 1291283 - Use URLValueData::GetLocalURLFlag() to identify local-ref URI.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2eafd00d4379
Use URLValueData::GetLocalURLFlag() to identify local-ref URI. r=dholbert

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2eafd00d4379