Bug 179432 – Support the decoding="sync/async" syntax for image async attribute

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

[Simon Fraser (smfr)]

We should support the on/off values for the async attribute, per https://github.com/whatwg/html/issues/1920. This is what Chrome is implementing.

[Radar WebKit Bug Importer]

<rdar://problem/35418350>

[Said Abou-Hallawa]

Created attachment 326351 [details]
Patch

[Simon Fraser (smfr)]

Let's hold off until we have consensus.

[Simon Fraser (smfr)]

https://github.com/whatwg/html/issues/1920

[Simon Fraser (smfr)]

Issue has some new naming now.

[Darin Adler]

Comment on attachment 326351 [details]
Patch

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

Some thoughts on this patch if we come back here to implement this.

> Source/WebCore/html/HTMLImageElement.cpp:230
> +        if (equalLettersIgnoringASCIICase(value, "yes") || (value.isEmpty() && !value.isNull()))
> +            m_async = true;
> +        if (equalLettersIgnoringASCIICase(value, "no"))
> +            m_async = false;

This needs to be improved if we decide to do this. The way this code is written if someone removes the async attribute or sets it to a value other than "yes", "no", or "", it will retain a "memory" of the old state. To correctly implement this, the clause needs to always set m_async to some value based on the attribute value, never leave it alone.

Or we can avoid this problem by not having m_async, and have a function that queries the attribute at the time it asks the question. There are two reasons to write code in parseAttribute:

1) If we need to immediately respond when an attribute changes, rather than just letting it affect future behavior.
2) If we need to cache something that is expensive to compute over and over again. The parseAttribute function can either cache the value, or invalidate the cache.

ChangeLog says "on/off", this code says "yes/no". Which is right? Why no test?

> Source/WebCore/html/HTMLImageElement.h:99
> +    bool hasAsync() const { return m_async; }

This function name could be improved. What would it mean to say that an image element "has async"?

[Simon Fraser (smfr)]

Comment on attachment 326351 [details]
Patch

This needs to be updated to match the proposed spec changes.

[Said Abou-Hallawa]

Created attachment 327218 [details]
Patch

[Said Abou-Hallawa]

Created attachment 327219 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 327219 [details]
Patch

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

> Source/WebCore/platform/graphics/DecodingOptions.h:59
> +        return hasDecodingMode() && WTF::get<DecodingMode>(m_decodingModeOrSize) == DecodingMode::Auto;

Don't you want to return true if !hasDecodingMode()?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:327
> +        if ((auto decodingMode = downcast<HTMLImageElement>(element())->decodingMode()) != DecodingMode::Auto)

This could be downcast<HTMLImageElement>(*element()).decodingMode()

> LayoutTests/fast/images/decoding-attribute-async-small-image.html:2
> +    <img decoding="async">

Can we also test setting the attribute on a JS image? var image = new Image(); image.decoding = 'async'?

[Said Abou-Hallawa]

Created attachment 327267 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 327219 [details]
Patch

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

>> Source/WebCore/platform/graphics/DecodingOptions.h:59
>> +        return hasDecodingMode() && WTF::get<DecodingMode>(m_decodingModeOrSize) == DecodingMode::Auto;
> 
> Don't you want to return true if !hasDecodingMode()?

No because if hasDecodingMode() is false that means hasSize() is true which means this DecodingOptions is Asynchronous. This function is currently used for checking whether the DecodingOptions is initialized or not. Having a DecodingMode equals to Auto means it is not initialized.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:327
>> +        if ((auto decodingMode = downcast<HTMLImageElement>(element())->decodingMode()) != DecodingMode::Auto)
> 
> This could be downcast<HTMLImageElement>(*element()).decodingMode()

Done.

>> LayoutTests/fast/images/decoding-attribute-async-small-image.html:2
>> +    <img decoding="async">
> 
> Can we also test setting the attribute on a JS image? var image = new Image(); image.decoding = 'async'?

Done. I also added a test which uses both the 'decoding' attribute with the decode() method.

[Darin Adler]

Comment on attachment 327267 [details]
Patch

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

> Source/WebCore/platform/graphics/DecodingOptions.h:42
> +    DecodingOptions(DecodingMode decodingMode = DecodingMode::Auto)

This constructor should be marked explicit.

[Said Abou-Hallawa]

Created attachment 327662 [details]
Patch

[Said Abou-Hallawa]

Created attachment 327670 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 327670 [details]
Patch

I think we can land this now.

[Said Abou-Hallawa]

Created attachment 328664 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 328664 [details]
Patch

Clearing flags on attachment: 328664

Committed r225616: <https://trac.webkit.org/changeset/225616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.