Custom Elements: Encapsulates Async and Sync Queues into one class.

Custom Elements: Encapsulates Async and Sync Queues into one class.

This refactoring encapsulates sync and async microtask queues into
CustomElementMicrotaskStepDispatcher class.
Also, it renames CustomElementMicrotaskQueue to CustomElementSyncMicrotaskQueue
for clarifying the difference from the async queue.

This is a preparation for fixing crbug.com/384516

BUG=384516
TEST=none
R=dglazkov@chromium.org, dominicc@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176434

[dglazkov, 2014-06-16 22:46:37]

I am not super-familiar with these lands of code. Dominic, can I count on your
review here? If not, let me know, I'll dig in.

[dominicc (has gone to gerrit), 2014-06-17 00:29:23]

I don't get the direction yet but this is the first patch I looked at so for now
there are some inline comments.

There's a typo in the description.

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementMicrotaskQueue.h (right):

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:41: class
CustomElementMicrotaskQueue : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementMicrotaskQueue> {
Shouldn't you IWYU RefCountedWillBeGarbageCollectedFinalized, etc.?

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:41: class
CustomElementMicrotaskQueue : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementMicrotaskQueue> {
I think the naming of these types is wrong.

Given CEMQ, CESyncMQ and CEMQBase I think it would be reasonable to expect

CEMQ : CEMQBase

and some relationship to CESyncMQ, perhaps because of the presence of CEMQBase
something like

CESyncMQ : CEMQBase.

I think you need to think of some names that reinforce the distinctions between
these types. Otherwise it is just AdjectiveSaladNoun.

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:46:
CustomElementMicrotaskQueue();
Why is this visible?

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:59:
RefPtrWillBeMember<CustomElementSyncMicrotaskQueue> m_syncQueue;
Why do these need to be pointers? Are they shared anywhere? Why not just
members?

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementSyncMicrotaskQueue.cpp (right):

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementSyncMicrotaskQueue.cpp:50: m_queue.remove(0,
i);
Doesn't queue have a clear or something?

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementSyncMicrotaskQueue.h (right):

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementSyncMicrotaskQueue.h:1: /*
Use the short copyright notice on new files.

[Hajime Morrita, 2014-06-17 17:19:51]

Thanks for the review Dominic!
Please take another look?

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementMicrotaskQueue.h (right):

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:41: class
CustomElementMicrotaskQueue : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementMicrotaskQueue> {
On 2014/06/17 00:29:23, dominicc wrote:
> Shouldn't you IWYU RefCountedWillBeGarbageCollectedFinalized, etc.?

Done.

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:41: class
CustomElementMicrotaskQueue : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementMicrotaskQueue> {
On 2014/06/17 00:29:23, dominicc wrote:
> I think the naming of these types is wrong.
> 
> Given CEMQ, CESyncMQ and CEMQBase I think it would be reasonable to expect
> 
> CEMQ : CEMQBase
> 
> and some relationship to CESyncMQ, perhaps because of the presence of CEMQBase
> something like
> 
> CESyncMQ : CEMQBase.
> 
> I think you need to think of some names that reinforce the distinctions
between
> these types. Otherwise it is just AdjectiveSaladNoun.

Well, right. This isn't queue at all. Renaed to
QustomElementPendingMicrotaskSet.

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:46:
CustomElementMicrotaskQueue();
On 2014/06/17 00:29:23, dominicc wrote:
> Why is this visible?

Done.

https://codereview.chromium.org/334253005/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:59:
RefPtrWillBeMember<CustomElementSyncMicrotaskQueue> m_syncQueue;
On 2014/06/17 00:29:23, dominicc wrote:
> Why do these need to be pointers? Are they shared anywhere? Why not just
> members?
Yes. They are ref counted objects and one of them is shared by a
CustomElementAsyncImportMicrotaskStep.

[dominicc (has gone to gerrit), 2014-06-18 00:11:28]

More comments inline.

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:38:
m_resolutionAndImports->enqueue(parentLoader, step);
I guess if it is a set this method should be add and not enqueue?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskImportStep.cpp (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskImportStep.cpp:35: #include
"core/dom/custom/CustomElementPendingMicrotaskSet.h"
What uses this include?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp:22:
ASSERT(m_queue->isEmpty() || !hasReenter());
Depending on doDispatch, is it worth making this
ASSERT_WITH_SECURITY_IMPLICATION and always doing this check?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp:31: bool hasReenter()
const
This looks like a move, but consider renaming this hasReentered?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp:54:
MicrotaskQueueInvocationScope scope(this);
Why do all of this when a simple bool m_inDispatch will give you the same
assertion in four lines of code instead of ~25?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementPendingMicrotaskSet.h (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementPendingMicrotaskSet.h:16: class
CustomElementPendingMicrotaskSet : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementPendingMicrotaskSet> {
This isn't really a "set" in the mathematical or std::set sense of that word.

I think "pending" does not add much here either. The
CustomElementSyncMicrotaskQueue is equally "pending", right? But it does not
have "pending" in the name.

Who uses this? The CustomElementMicrotaskDispatcher; who else?

This is a bad name, but here is a seed of an idea: This is almost like a
priority queue, because the sync stuff goes first.

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementScheduler.cpp (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.cpp:42: #include
"core/dom/custom/CustomElementPendingMicrotaskSet.h"
Was this a spurious include? What needs this in here?

[Hajime Morrita, 2014-06-18 00:57:06]

PTAL?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp:22:
ASSERT(m_queue->isEmpty() || !hasReenter());
On 2014/06/18 00:11:28, dominicc wrote:
> Depending on doDispatch, is it worth making this
> ASSERT_WITH_SECURITY_IMPLICATION and always doing this check?

Done.

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp:31: bool hasReenter()
const
On 2014/06/18 00:11:28, dominicc wrote:
> This looks like a move, but consider renaming this hasReentered?

Done.

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueueBase.cpp:54:
MicrotaskQueueInvocationScope scope(this);
On 2014/06/18 00:11:28, dominicc wrote:
> Why do all of this when a simple bool m_inDispatch will give you the same
> assertion in four lines of code instead of ~25?

Done.

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementPendingMicrotaskSet.h (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementPendingMicrotaskSet.h:16: class
CustomElementPendingMicrotaskSet : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementPendingMicrotaskSet> {
Noticed that this is the only object which dispatches microtask steps (besides
the queue itself).
So how about CustomElementMicrotaskStepDispatcher?

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementScheduler.cpp (right):

https://codereview.chromium.org/334253005/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.cpp:42: #include
"core/dom/custom/CustomElementPendingMicrotaskSet.h"
On 2014/06/18 00:11:28, dominicc wrote:
> Was this a spurious include? What needs this in here?

Done.

[dominicc (has gone to gerrit), 2014-06-18 01:04:45]

YES! LGTM.

[Hajime Morrita, 2014-06-18 16:07:24]

The CQ bit was checked by morrita@chromium.org

[Hajime Morrita, 2014-06-18 16:07:31]

On 2014/06/18 01:04:45, dominicc wrote:
> YES! LGTM.

Thanks you!

[commit-bot: I haz the power, 2014-06-18 16:08:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/334253005/40001

[commit-bot: I haz the power, 2014-06-18 16:52:33]

Change committed as 176434