Custom elements reactions should have a queue per element

[Ryosuke Niwa]

Update our reactions queue' semantics to match the latest specifications.

[Radar WebKit Bug Importer]

<rdar://problem/28910901>

[Ryosuke Niwa]

Created attachment 292580 [details]
Fixes the bug

[WebKit Commit Bot]

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 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]

(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]

Created attachment 292718 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292718 [details]
Patch for landing

Clearing flags on attachment: 292718

Committed r207810: <http://trac.webkit.org/changeset/207810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.