RESOLVED FIXED 163878
Custom elements reactions should have a queue per element
https://bugs.webkit.org/show_bug.cgi?id=163878
Summary Custom elements reactions should have a queue per element
Ryosuke Niwa
Reported 2016-10-23 20:50:31 PDT
Update our reactions queue' semantics to match the latest specifications.
Attachments
Fixes the bug (39.44 KB, patch)
2016-10-23 21:48 PDT, Ryosuke Niwa
no flags
Patch for landing (39.25 KB, patch)
2016-10-24 22:14 PDT, Ryosuke Niwa
no flags
Radar WebKit Bug Importer
Comment 1 2016-10-23 21:46:49 PDT
Ryosuke Niwa
Comment 2 2016-10-23 21:48:08 PDT
Created attachment 292580 [details] Fixes the bug
WebKit Commit Bot
Comment 3 2016-10-23 21:49:58 PDT
Attachment 292580 [details] did not pass style-queue: ERROR: Source/WebCore/dom/CustomElementReactionQueue.cpp:250: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Antti Koivisto
Comment 4 2016-10-24 02:38:52 PDT
Comment on attachment 292580 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=292580&action=review > Source/WebCore/dom/CustomElementReactionQueue.cpp:254 > +class BackupElementQueueMicrotask final : public Microtask { > + WTF_MAKE_FAST_ALLOCATED; > +private: > + Result run() final > + { > + CustomElementReactionStack::processBackupQueue(); > + return Result::Done; > + } > +}; Someone should modernize Microtask to take lambdas rather than require subclassing. > Source/WebCore/dom/CustomElementReactionQueue.cpp:264 > +CustomElementReactionStack::ElementQueue& CustomElementReactionStack::ensureBackupQueue() > +{ > + auto& queue = backupElementQueue(); > + if (queue) > + return *queue; // If the processing the backup element queue flag is set, abort this algorithm. > + queue = std::make_unique<ElementQueue>(); // Set the processing the backup element queue flag. > + MicrotaskQueue::mainThreadQueue().append(std::make_unique<BackupElementQueueMicrotask>()); > + return *queue; > +} I wonder if this would be easier to follow if there was an actual "processing the backup element queue" boolean flag instead of using the existence of backupElementQueue object. backupElementQueue could just stay alive like normal global NeverDestroyed.
Ryosuke Niwa
Comment 5 2016-10-24 22:08:30 PDT
(In reply to comment #4) > Comment on attachment 292580 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292580&action=review > > > Source/WebCore/dom/CustomElementReactionQueue.cpp:254 > > +class BackupElementQueueMicrotask final : public Microtask { > > + WTF_MAKE_FAST_ALLOCATED; > > +private: > > + Result run() final > > + { > > + CustomElementReactionStack::processBackupQueue(); > > + return Result::Done; > > + } > > +}; > > Someone should modernize Microtask to take lambdas rather than require > subclassing. Yeah, we should do that. > > Source/WebCore/dom/CustomElementReactionQueue.cpp:264 > > +CustomElementReactionStack::ElementQueue& CustomElementReactionStack::ensureBackupQueue() > > +{ > > + auto& queue = backupElementQueue(); > > + if (queue) > > + return *queue; // If the processing the backup element queue flag is set, abort this algorithm. > > + queue = std::make_unique<ElementQueue>(); // Set the processing the backup element queue flag. > > + MicrotaskQueue::mainThreadQueue().append(std::make_unique<BackupElementQueueMicrotask>()); > > + return *queue; > > +} > > I wonder if this would be easier to follow if there was an actual > "processing the backup element queue" boolean flag instead of using the > existence of backupElementQueue object. backupElementQueue could just stay > alive like normal global NeverDestroyed. Done that.
Ryosuke Niwa
Comment 6 2016-10-24 22:14:13 PDT
Created attachment 292718 [details] Patch for landing
WebKit Commit Bot
Comment 7 2016-10-24 23:21:02 PDT
Comment on attachment 292718 [details] Patch for landing Clearing flags on attachment: 292718 Committed r207810: <http://trac.webkit.org/changeset/207810>
WebKit Commit Bot
Comment 8 2016-10-24 23:21:10 PDT
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.