Bug 163878 - Custom elements reactions should have a queue per element
Summary: Custom elements reactions should have a queue per element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 154907
  Show dependency treegraph
 
Reported: 2016-10-23 20:50 PDT by Ryosuke Niwa
Modified: 2016-10-24 23:21 PDT (History)
8 users (show)

See Also:


Attachments
Fixes the bug (39.44 KB, patch)
2016-10-23 21:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (39.25 KB, patch)
2016-10-24 22:14 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-10-23 20:50:31 PDT
Update our reactions queue' semantics to match the latest specifications.
Comment 1 Radar WebKit Bug Importer 2016-10-23 21:46:49 PDT
<rdar://problem/28910901>
Comment 2 Ryosuke Niwa 2016-10-23 21:48:08 PDT
Created attachment 292580 [details]
Fixes the bug
Comment 3 WebKit Commit Bot 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.
Comment 4 Antti Koivisto 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 2016-10-24 22:14:13 PDT
Created attachment 292718 [details]
Patch for landing
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2016-10-24 23:21:10 PDT
All reviewed patches have been landed.  Closing bug.