Bug 213335 – Image `crossorigin` mutations should be considered "relevant mutations"

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rob Buis]

See discussion in https://bugs.webkit.org/show_bug.cgi?id=209970. The test for this is html/semantics/embedded-content/the-img-element/relevant-mutations.html.

[Rob Buis]

Created attachment 402285 [details]
Patch

[Rob Buis]

Created attachment 402380 [details]
Patch

[Rob Buis]

Created attachment 402396 [details]
Patch

[Rob Buis]

Created attachment 402419 [details]
Patch

[Rob Buis]

Created attachment 402428 [details]
Patch

[Darin Adler]

Comment on attachment 402428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402428&action=review

review+ assuming this will be revised to *not* use "!=" to compare string literals

> Source/WebCore/html/HTMLImageElement.cpp:169
> +        if (child.get() == this)

Should not need to call "get" here. RefPtr should have the appropriate operator== overload.

> Source/WebCore/html/HTMLImageElement.cpp:256
> +        auto oldCrossoriginState = oldValue.isNull() ? "empty" : (equalIgnoringASCIICase(oldValue, "use-credentials") ? "use-credentials" : "anonymous");
> +        auto newCrossoriginState = newValue.isNull() ? "empty" : (equalIgnoringASCIICase(newValue, "use-credentials") ? "use-credentials" : "anonymous");
> +        if (oldCrossoriginState != newCrossoriginState)

This looks dangerous. It compares two "const char*" strings using !=, which doesn’t compare string contents, instead it compares string pointers, which are not guaranteed to be equal. I suggest we use an enumeration with three values here instead of strings. Could also avoid writing the code twice by using a function or lambda. Might call it parseCrossOriginState.

> Source/WebCore/html/HTMLImageElement.h:31
> +#include "ImageLoader.h"

Really unfortunate that we need to add this new include just for the enumeration type. Can we consider a different pattern so we can avoid that? Is there some kind of forward declaration we can do? Maybe make it a non-member enumeration just to avoid this. I worry about headers including others headers having a bad affect on our compile time, cumulatively long term.

> Source/WebCore/html/HTMLSourceElement.cpp:88
> -        if (is<HTMLPictureElement>(*parent))
> -            downcast<HTMLPictureElement>(*parent).sourcesChanged();
> +        if (is<HTMLPictureElement>(*parent)) {
> +            m_validSourceElement = true;
> +            for (const Node* node = previousSibling(); node; node = node->previousSibling()) {
> +                if (is<HTMLImageElement>(*node))
> +                    m_validSourceElement = false;
> +            }
> +            if (m_validSourceElement)
> +                downcast<HTMLPictureElement>(*parent).sourcesChanged();
> +        }

I would be happier with this code if m_validSourceElement was guaranteed to be false when the element is not in a picture element, rather than being uninitialized (or left set the way it was last time it was the child of a picture element).

Also seems like we could consider removing the is<HTMLPictureElement> checks, because m_validSourceElement would guarantee that it’s a picture element.

> Source/WebCore/html/HTMLSourceElement.h:65
> +    bool m_validSourceElement { false };

This sounds like a variable which would contain an element, not one that is a boolean predicate. Maybe m_isValid? Something else? Is the term "valid" really the correct one for this? We should use the same term that the HTML specification uses for it, ideally.

[Rob Buis]

Created attachment 402448 [details]
Patch

[Rob Buis]

Comment on attachment 402428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402428&action=review

>> Source/WebCore/html/HTMLImageElement.cpp:169
>> +        if (child.get() == this)
> 
> Should not need to call "get" here. RefPtr should have the appropriate operator== overload.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:256
>> +        if (oldCrossoriginState != newCrossoriginState)
> 
> This looks dangerous. It compares two "const char*" strings using !=, which doesn’t compare string contents, instead it compares string pointers, which are not guaranteed to be equal. I suggest we use an enumeration with three values here instead of strings. Could also avoid writing the code twice by using a function or lambda. Might call it parseCrossOriginState.

Right, much better! The "empty" solution was a bit hack and random as well. Done.

>> Source/WebCore/html/HTMLImageElement.h:31
>> +#include "ImageLoader.h"
> 
> Really unfortunate that we need to add this new include just for the enumeration type. Can we consider a different pattern so we can avoid that? Is there some kind of forward declaration we can do? Maybe make it a non-member enumeration just to avoid this. I worry about headers including others headers having a bad affect on our compile time, cumulatively long term.

I think possibly the only thing we can do is convert enums to enum classes, which can be forward declared. Maybe we need a bug for that, a bit similar to bug(s) to introduce modern c++ features?

>> Source/WebCore/html/HTMLSourceElement.cpp:88
>> +        }
> 
> I would be happier with this code if m_validSourceElement was guaranteed to be false when the element is not in a picture element, rather than being uninitialized (or left set the way it was last time it was the child of a picture element).
> 
> Also seems like we could consider removing the is<HTMLPictureElement> checks, because m_validSourceElement would guarantee that it’s a picture element.

I removed the is<HTMLPictureElement> checks and do not allow m_validSourceElement to be uninitialized.

>> Source/WebCore/html/HTMLSourceElement.h:65
>> +    bool m_validSourceElement { false };
> 
> This sounds like a variable which would contain an element, not one that is a boolean predicate. Maybe m_isValid? Something else? Is the term "valid" really the correct one for this? We should use the same term that the HTML specification uses for it, ideally.

It is hard to come up with the perfect name here. I think I have a better name now and added a comment why it is needed.

[Rob Buis]

Created attachment 402458 [details]
Patch

[Rob Buis]

Comment on attachment 402428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402428&action=review

>>> Source/WebCore/html/HTMLImageElement.h:31
>>> +#include "ImageLoader.h"
>> 
>> Really unfortunate that we need to add this new include just for the enumeration type. Can we consider a different pattern so we can avoid that? Is there some kind of forward declaration we can do? Maybe make it a non-member enumeration just to avoid this. I worry about headers including others headers having a bad affect on our compile time, cumulatively long term.
> 
> I think possibly the only thing we can do is convert enums to enum classes, which can be forward declared. Maybe we need a bug for that, a bit similar to bug(s) to introduce modern c++ features?

Ok, RelevantMutation was already an enum class, so it just had to be moved out of ImageLoader class, and now we can avoid the include.

[EWS]

Committed r263345: <https://trac.webkit.org/changeset/263345>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402458 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64590953>

[Darin Adler]

Comment on attachment 402458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402458&action=review

> Source/WebCore/html/HTMLImageElement.cpp:169
> +        if (child == this)

I looked again, and I think this is dead code. The loop condition already checked child != this so I don’t see how child == this here could ever be true.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 402476 [details]
Patch

[Rob Buis]

Comment on attachment 402458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402458&action=review

>> Source/WebCore/html/HTMLImageElement.cpp:169
>> +        if (child == this)
> 
> I looked again, and I think this is dead code. The loop condition already checked child != this so I don’t see how child == this here could ever be true.

Nice catch, I thought it was just a normal children iteration loop. This should go green...

[EWS]

Committed r263350: <https://trac.webkit.org/changeset/263350>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402476 [details].

[Simon Fraser (smfr)]

This broke srcset in <picture> in iframes, like:

<picture>
    <source srcset="" media="(min-width: 1069px) and (-webkit-min-device-pixel-ratio: 1.5), (min-width: 1069px) and (min-resolution: 1.5dppx), (min-width: 1069px) and (min-resolution: 144dpi)">
    <source srcset="" media="(min-width: 1069px)"><source srcset="..." media="(min-width: 735px) and (max-width: 1068px) and (-webkit-min-device-pixel-ratio: 1.5), (min-width: 735px) and (max-width: 1068px) and (min-resolution: 1.5dppx), (min-width: 735px) and (max-width: 1068px) and (min-resolution: 144dpi)">
    <source srcset="" media="(min-width: 735px) and (max-width: 1068px)">
    <source srcset="" media="(max-width: 734px) and (-webkit-min-device-pixel-ratio: 1.5), (max-width: 734px) and (min-resolution: 1.5dppx), (max-width: 734px) and (min-resolution: 144dpi)">
    <source srcset="" media="(max-width: 734px)">
    <img width="400" height="250" class="jsx-528434275 picture__image">
</picture>

Is that intended?

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #18)
> This broke srcset in <picture> in iframes, like:
> 
> <picture>
>     <source srcset="" media="(min-width: 1069px) and
> (-webkit-min-device-pixel-ratio: 1.5), (min-width: 1069px) and
> (min-resolution: 1.5dppx), (min-width: 1069px) and (min-resolution: 144dpi)">
>     <source srcset="" media="(min-width: 1069px)"><source srcset="..."
> media="(min-width: 735px) and (max-width: 1068px) and
> (-webkit-min-device-pixel-ratio: 1.5), (min-width: 735px) and (max-width:
> 1068px) and (min-resolution: 1.5dppx), (min-width: 735px) and (max-width:
> 1068px) and (min-resolution: 144dpi)">
>     <source srcset="" media="(min-width: 735px) and (max-width: 1068px)">
>     <source srcset="" media="(max-width: 734px) and
> (-webkit-min-device-pixel-ratio: 1.5), (max-width: 734px) and
> (min-resolution: 1.5dppx), (max-width: 734px) and (min-resolution: 144dpi)">
>     <source srcset="" media="(max-width: 734px)">
>     <img width="400" height="250" class="jsx-528434275 picture__image">
> </picture>
> 
> Is that intended?

How do Firefox and Chrome behave here?

[Simon Fraser (smfr)]

Testcase: https://codepen.io/chrisyz/pen/NWbMbyb

This works in Firefox.

[Simon Fraser (smfr)]

I filed bug 222578