[webkit-reviews] review granted: [Bug 213335] Image `crossorigin` mutations should be considered "relevant mutations" : [Attachment 402428] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 213335: Image `crossorigin` mutations should be considered "relevant
mutations"
https://bugs.webkit.org/show_bug.cgi?id=213335

Attachment 402428: Patch

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




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