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.
<rdar://problem/35418350>
Created attachment 326351 [details] Patch
Let's hold off until we have consensus.
https://github.com/whatwg/html/issues/1920
Issue has some new naming now.
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"?
Comment on attachment 326351 [details] Patch This needs to be updated to match the proposed spec changes.
Created attachment 327218 [details] Patch
Created attachment 327219 [details] Patch
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'?
Created attachment 327267 [details] Patch
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.
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.
Created attachment 327662 [details] Patch
Created attachment 327670 [details] Patch
Comment on attachment 327670 [details] Patch I think we can land this now.
Created attachment 328664 [details] Patch
Comment on attachment 328664 [details] Patch Clearing flags on attachment: 328664 Committed r225616: <https://trac.webkit.org/changeset/225616>
All reviewed patches have been landed. Closing bug.