Bug 179432

Summary: Support the decoding="sync/async" syntax for image async attribute
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: DOMAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, mike, sabouhallawa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Simon Fraser (smfr)
Reported 2017-11-08 10:50:39 PST
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.
Attachments
Patch (3.57 KB, patch)
2017-11-08 13:10 PST, Said Abou-Hallawa
no flags
Patch (13.82 KB, patch)
2017-11-17 12:53 PST, Said Abou-Hallawa
no flags
Patch (13.33 KB, patch)
2017-11-17 13:01 PST, Said Abou-Hallawa
no flags
Patch (17.60 KB, patch)
2017-11-17 16:32 PST, Said Abou-Hallawa
no flags
Patch (26.16 KB, patch)
2017-11-27 13:02 PST, Said Abou-Hallawa
no flags
Patch (28.08 KB, patch)
2017-11-27 13:35 PST, Said Abou-Hallawa
no flags
Patch (29.64 KB, patch)
2017-12-06 17:35 PST, Said Abou-Hallawa
no flags
Radar WebKit Bug Importer
Comment 1 2017-11-08 10:51:05 PST
Said Abou-Hallawa
Comment 2 2017-11-08 13:10:21 PST
Simon Fraser (smfr)
Comment 3 2017-11-08 13:11:30 PST
Let's hold off until we have consensus.
Simon Fraser (smfr)
Comment 4 2017-11-08 13:30:30 PST
Simon Fraser (smfr)
Comment 5 2017-11-09 17:27:56 PST
Issue has some new naming now.
Darin Adler
Comment 6 2017-11-12 22:30:33 PST
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 7 2017-11-13 10:30:28 PST
Comment on attachment 326351 [details] Patch This needs to be updated to match the proposed spec changes.
Said Abou-Hallawa
Comment 8 2017-11-17 12:53:02 PST
Said Abou-Hallawa
Comment 9 2017-11-17 13:01:14 PST
Simon Fraser (smfr)
Comment 10 2017-11-17 13:43:35 PST
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
Comment 11 2017-11-17 16:32:19 PST
Said Abou-Hallawa
Comment 12 2017-11-17 16:37:38 PST
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 13 2017-11-21 08:58:38 PST
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
Comment 14 2017-11-27 13:02:10 PST
Said Abou-Hallawa
Comment 15 2017-11-27 13:35:41 PST
Simon Fraser (smfr)
Comment 16 2017-12-05 11:19:46 PST
Comment on attachment 327670 [details] Patch I think we can land this now.
Said Abou-Hallawa
Comment 17 2017-12-06 17:35:43 PST
WebKit Commit Bot
Comment 18 2017-12-06 19:36:06 PST
Comment on attachment 328664 [details] Patch Clearing flags on attachment: 328664 Committed r225616: <https://trac.webkit.org/changeset/225616>
WebKit Commit Bot
Comment 19 2017-12-06 19:36:08 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.