HTML Imports: Get rid of needsProcessOrStop() from dom/custom/

HTML Imports: Get rid of needsProcessOrStop() from dom/custom/

This is a revision of r173794, which introduced extra complexity
to CustomElementMicrotaskStep classes.

This change moves the responsibility that introduces the complexity
to HTMLImportChild. This localizes the tricky part in the single class
and helps overall code sanity.

TEST=no
BUG=371654
R=dominicc@chromium.org, dglazkov@chromium.org

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

[Hajime Morrita, 2014-05-16 10:40:24]

Dominic: PTAL?

This is a follow up for https://codereview.chromium.org/272243004.
If this looks good, I'll revert the change, combine this, then
re-land it. That'll make the merge easier.

[dglazkov, 2014-05-16 16:10:49]

I am going to try an grok this machinery so that I can make more meaningful
comments :)

https://codereview.chromium.org/288323004/diff/1/Source/core/html/imports/HTM...
File Source/core/html/imports/HTMLImportChild.cpp (right):

https://codereview.chromium.org/288323004/diff/1/Source/core/html/imports/HTM...
Source/core/html/imports/HTMLImportChild.cpp:209: bool
HTMLImportChild::isWaitingCustomElementMicrotaskSteps() const
"is waiting on" or "is waiting for" might feel a touch smoother?

[Hajime Morrita, 2014-05-16 17:28:06]

On 2014/05/16 16:10:49, dglazkov wrote:
> I am going to try an grok this machinery so that I can make more meaningful
> comments :)

Thanks for taking look! Custom Elements - Imports integration is kinda Achilles
heel.It's a bit hard to reason.
That is one of what Dominic is worried about here.

> 
>
https://codereview.chromium.org/288323004/diff/1/Source/core/html/imports/HTM...
> File Source/core/html/imports/HTMLImportChild.cpp (right):
> 
>
https://codereview.chromium.org/288323004/diff/1/Source/core/html/imports/HTM...
> Source/core/html/imports/HTMLImportChild.cpp:209: bool
> HTMLImportChild::isWaitingCustomElementMicrotaskSteps() const
> "is waiting on" or "is waiting for" might feel a touch smoother?
Done.

[Hajime Morrita, 2014-05-19 20:58:08]

Dominic: Ping?

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

QQ inline.

https://codereview.chromium.org/288323004/diff/20001/Source/core/html/imports...
File Source/core/html/imports/HTMLImportChild.cpp (right):

https://codereview.chromium.org/288323004/diff/20001/Source/core/html/imports...
Source/core/html/imports/HTMLImportChild.cpp:221: return ignorableStepCount <
m_loader->microtaskQueue()->size();
Wrote a huge comment about this, but let me try and simplify:

The localization is good.

Checking the sizes is kind of implicit, so let's try to think of something
better; I have some ideas but I need to know more.

When would a non-sync import's Custom Element Microtask Import Step show up in
the parent import's CEMT queue? Why?

[Hajime Morrita, 2014-05-23 00:38:49]

Hi Dominic, Thanks for the review!

On 2014/05/23 00:29:24, dominicc wrote:
> The localization is good.
> 
> Checking the sizes is kind of implicit, so let's try to think of something
> better; I have some ideas but I need to know more.
> 
> When would a non-sync import's Custom Element Microtask Import Step show up in
> the parent import's CEMT queue? Why?

They always do. Currently CEMT is the only way to execute microsteps in imports,
regardless whether one is async or not.
I think it's reasonable to centralize step execution in CEMT. Debugging is much
easier if you can see all the pending spec at once.

One of the idea that was in my mind was, to have some API to traverse all the
steps in the (nested) queue.
Then imports side walks through it and see if it has to wait. It breaks certain
level of encapsulation though.

[dominicc (has gone to gerrit), 2014-05-23 01:23:39]

On 2014/05/23 00:38:49, morrita1 wrote: 
> They always do. Currently CEMT is the only way to execute microsteps in
imports,
> regardless whether one is async or not.

Yes. But it seems having async imports in that queue is what causes a lot of
complexity: there's this size comparison thing, which is fine now but might be
fragile for changes, and there's the "continue processing the queue but keep me
alive and try again later" thing.

One local change you could make is to give the CustomElementMicrotaskImportStep
two queues; one for elements and sync subimports, and a separate one for async
imports. Then instead of this loop you should just be able to do
!m_theFirstQueue.isEmpty() (This doesn't remove the need for the "keep me alive
to be called again" thing though.)

That idea could be taken futher: Instead of giving CEMImportStep two queues, you
could give the import two *steps*, one for the "sync part" and one for the async
part. And you could have two separate microtasks. Only the second async one has
this "run without stopping and ask the queue if it should be deleted" behavior.

> One of the idea that was in my mind was, to have some API to traverse all the
> steps in the (nested) queue.
> Then imports side walks through it and see if it has to wait. It breaks
certain
> level of encapsulation though.

Yes, I was thinking about something like this too. I was thinking you could
eliminate the need for lots of casting and imports-specific virtuals by just
having contains(CEMTStep*) and using that to marry the state of the queue to the
state on the imports side. I could't come up with anything clean, efficient and
keeping the concerns well separated.

[Hajime Morrita, 2014-05-24 01:54:48]

PTAL?

I switched to double-queue approach and it seems promising. 
Overall complexity decreases, import specific bits are localized to
CustomElementMicrotaskImportStep and newly added
CustomElementAsyncImportMicrotaskQueue.
CustomElementMicrotaskDispatcher now explicitly aware the notion of imports, but
I think this explicitness rather clarifies what's going.

[dominicc (has gone to gerrit), 2014-05-25 23:55:09]

I think this is shaping up nicely.

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementAsyncImportMicrotaskQueue.h (right):

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementAsyncImportMicrotaskQueue.h:48: virtual void
doDispatch();
Does this need OVERRIDE?

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

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:60: ASSERT(m_phase
== Quiescent || m_phase == DispatchingCallbacks);
Can we refactor to eliminate the duplication of these two lines with
enqueue(HTMLImportLoader*, ...)?

I'm fine if you change the signature, calling of the other one to do it.

For example, I think whether an import is "sync" is already part of its public
API; can we keep the fact that CustomElementMicrotaskDispatcher has two separate
queues a private implementation detail of this class? (At least at the API
level; I understand that the order will change and the async ones will may go
later than they do today.)

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:99:
m_asyncImports->dispatch();
Could we dispatch these anyway? WDYT?

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.h (right):

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.h:30: void
enqueueAsyncImportStep(PassOwnPtr<CustomElementMicrotaskImportStep>);
Why does the signature differ from enqueue(HTMLImportLoader*, ...)?

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskImportStep.h (left):

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskImportStep.h:68: bool
shouldStopProcessing() const;
Woo.

[Hajime Morrita, 2014-05-27 21:12:46]

PTAL?

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

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:60: ASSERT(m_phase
== Quiescent || m_phase == DispatchingCallbacks);
On 2014/05/25 23:55:10, dominicc wrote:
> Can we refactor to eliminate the duplication of these two lines with
> enqueue(HTMLImportLoader*, ...)?
> 
> I'm fine if you change the signature, calling of the other one to do it.
> 
> For example, I think whether an import is "sync" is already part of its public
> API; can we keep the fact that CustomElementMicrotaskDispatcher has two
separate
> queues a private implementation detail of this class? (At least at the API
> level; I understand that the order will change and the async ones will may go
> later than they do today.)

Done. Made some rearrangement around signature and also extracted the "ensure"
bit.

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:99:
m_asyncImports->dispatch();
On 2014/05/25 23:55:10, dominicc wrote:
> Could we dispatch these anyway? WDYT?
That makes some tests fail.
Async imports don't block followers, but they are blocked by imports that came
before.

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.h (right):

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.h:30: void
enqueueAsyncImportStep(PassOwnPtr<CustomElementMicrotaskImportStep>);
On 2014/05/25 23:55:10, dominicc wrote:
> Why does the signature differ from enqueue(HTMLImportLoader*, ...)?
As async steps go to top-level global queueue, there is parent loader
(that is the first argument of the original enqueue()).

[dominicc (has gone to gerrit), 2014-05-27 23:41:25]

LGTM

Some comments inline; I trust your taste and judgement.

Happy to do another round if you think it would be useful.

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

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:99:
m_asyncImports->dispatch();
On 2014/05/27 21:12:47, morrita1 wrote:
> On 2014/05/25 23:55:10, dominicc wrote:
> > Could we dispatch these anyway? WDYT?
> That makes some tests fail.
> Async imports don't block followers, but they are blocked by imports that came
> before.

I remembered this in the middle of the night last night. I like the behavior of
async imports going later like this, so this is nice as-is.

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.h (right):

https://codereview.chromium.org/288323004/diff/40001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.h:30: void
enqueueAsyncImportStep(PassOwnPtr<CustomElementMicrotaskImportStep>);
On 2014/05/27 21:12:47, morrita1 wrote:
> On 2014/05/25 23:55:10, dominicc wrote:
> > Why does the signature differ from enqueue(HTMLImportLoader*, ...)?
> As async steps go to top-level global queueue, there is parent loader
> (that is the first argument of the original enqueue()).

Maybe we should name the parameter in the signature "parent" or something? WDYT?

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:42:
willOrDidEnqueueStep();
Thanks for fighting duplication.

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:52: if
(step->isSync())
Very neat; I love it.

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:68:
willOrDidEnqueueStep();
Think willOrDidEnqueueStep is mis-named, because in this case the step was
enqueued a LOOOONG time ago.

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskImportStep.cpp:69: bool
CustomElementMicrotaskImportStep::isSync() const
It looks like this is only used during that initial enqueuing, so having a whole
method seems heavy. Would you consider a parameter to the enqueue method?

Even if you keep the method, it might be confusing that it can change when the
import is cleared. It doesn't change the nature of the queued task.

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskImportStep.h (right):

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskImportStep.h:62: bool isSync()
const;
What about isForSyncImport? The step itself is basically sync...

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.cpp:91:
CustomElementMicrotaskDispatcher::instance().enqueue(import->parent()->loader(),
step.release());
... would just passing isSync() here be neater than adding isSync to the step?

[Hajime Morrita, 2014-05-28 00:41:36]

Thanks for the review!
Could you PTAL just in case? The diff is small.

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:68:
willOrDidEnqueueStep();
On 2014/05/27 23:41:25, dominicc wrote:
> Think willOrDidEnqueueStep is mis-named, because in this case the step was
> enqueued a LOOOONG time ago.

Agreed. I just did't came up with any better name...
What about ensureMicrotaskScheduledForMicrotaskStep() ?

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskImportStep.cpp:69: bool
CustomElementMicrotaskImportStep::isSync() const
On 2014/05/27 23:41:25, dominicc wrote:
> It looks like this is only used during that initial enqueuing, so having a
whole
> method seems heavy. Would you consider a parameter to the enqueue method?
> 
> Even if you keep the method, it might be confusing that it can change when the
> import is cleared. It doesn't change the nature of the queued task.
Probably that's better. Will do.

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.cpp:91:
CustomElementMicrotaskDispatcher::instance().enqueue(import->parent()->loader(),
step.release());
On 2014/05/27 23:41:25, dominicc wrote:
> ... would just passing isSync() here be neater than adding isSync to the step?

Done.

[dominicc (has gone to gerrit), 2014-05-28 01:41:34]

LGTM modulo one comment inline.

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

https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:68:
willOrDidEnqueueStep();
On 2014/05/28 00:41:37, morrita1 wrote:
> On 2014/05/27 23:41:25, dominicc wrote:
> > Think willOrDidEnqueueStep is mis-named, because in this case the step was
> > enqueued a LOOOONG time ago.
> 
> Agreed. I just did't came up with any better name...
> What about ensureMicrotaskScheduledForMicrotaskStep() ?

OK. Can you extract the two lines from enqueue(CustomElementCallbackQueue*) and
call them ensureMicrotaskScheduledForElementQueue() and put it right next to
ensureMicrotaskScheduledForMicrotaskStep. That will explain what the methods are
by contrast. Then there should be no callers of ensureMicrotaskScheduled except
those two methods.

[Hajime Morrita, 2014-05-28 21:17:50]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-05-28 21:18:13]

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

[Hajime Morrita, 2014-05-28 21:19:26]

On 2014/05/28 01:41:34, dominicc wrote:
> LGTM modulo one comment inline.
> 
>
https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
> File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp (right):
> 
>
https://codereview.chromium.org/288323004/diff/60001/Source/core/dom/custom/C...
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:68:
> willOrDidEnqueueStep();
> On 2014/05/28 00:41:37, morrita1 wrote:
> > On 2014/05/27 23:41:25, dominicc wrote:
> > > Think willOrDidEnqueueStep is mis-named, because in this case the step was
> > > enqueued a LOOOONG time ago.
> > 
> > Agreed. I just did't came up with any better name...
> > What about ensureMicrotaskScheduledForMicrotaskStep() ?
> 
> OK. Can you extract the two lines from enqueue(CustomElementCallbackQueue*)
and
> call them ensureMicrotaskScheduledForElementQueue() and put it right next to
> ensureMicrotaskScheduledForMicrotaskStep. That will explain what the methods
are
> by contrast. Then there should be no callers of ensureMicrotaskScheduled
except
> those two methods.
done.

[commit-bot: I haz the power, 2014-05-28 22:30:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-05-28 23:00:12]

Change committed as 175007

[haraken, 2014-05-29 02:22:33]

sof@: I think this CL conflicted with your CL to move custom elements to
oilpan's heap. Although I fixed the build issue, I'm not sure if there is any
more serious conflict that affects behavior. Would you take a quick look at this
CL?

[Hajime Morrita, 2014-05-29 20:02:15]

Ouch, did this break the build? I'm sorry about that :-(