Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <style>'s load event to
document's load event, because HTMLStyleElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress caused by
https://codereview.chromium.org/2275493002/.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress

Review-Url: https://codereview.chromium.org/2605173002
Cr-Commit-Position: refs/heads/master@{#444886}
Committed: https://chromium.googlesource.com/chromium/src/+/0712ce38d1a05590e731d07f9aad4b46870f53ae

[hiroshige, 2016-12-28 23:46:32]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 23:46:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-12-28 23:52:37]

Description was changed from

==========
Check Document onload synchronously in HTMLStyleElement::dispatchPendingEvent()

BUG=663823
==========

to

==========
Check Document onload synchronously in HTMLStyleElement::dispatchPendingEvent()

BUG=663823, 624697
==========

[hiroshige, 2016-12-29 20:41:26]

Description was changed from

==========
Check Document onload synchronously in HTMLStyleElement::dispatchPendingEvent()

BUG=663823, 624697
==========

to

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, HTMLStyleElement uses IncrementLoadEventDelayCount to block
Document load event until style element's load event is fired.
However, it causes one asynchronous hop from style's load event to document's
load event, because IncrementLoadEventDelayCount's destructor and
decrementLoadEventDelayCount() schedules checkCompleted() asynchronously.

This CL removes the asynchronous hop by synchronously calling checkCompleted()
when IncrementLoadEventDelayCount::clearAndCheckLoadEvent() is called, and
using it in HTMLStyleElement::dispatchPendingEvent().

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress

BUG=663823, 624697
==========

[hiroshige, 2016-12-29 20:41:26]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, yhirano@chromium.org

[hiroshige, 2016-12-29 20:49:53]

Description was changed from

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, HTMLStyleElement uses IncrementLoadEventDelayCount to block
Document load event until style element's load event is fired.
However, it causes one asynchronous hop from style's load event to document's
load event, because IncrementLoadEventDelayCount's destructor and
decrementLoadEventDelayCount() schedules checkCompleted() asynchronously.

This CL removes the asynchronous hop by synchronously calling checkCompleted()
when IncrementLoadEventDelayCount::clearAndCheckLoadEvent() is called, and
using it in HTMLStyleElement::dispatchPendingEvent().

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress

BUG=663823, 624697
==========

to

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <style>'s load event to
document's load event, because HTMLStyleElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by introducing and using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress caused by
https://codereview.chromium.org/2275493002/.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress
==========

[hiroshige, 2016-12-29 21:52:15]

Description was changed from

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <style>'s load event to
document's load event, because HTMLStyleElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by introducing and using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress caused by
https://codereview.chromium.org/2275493002/.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress
==========

to

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <style>'s load event to
document's load event, because HTMLStyleElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress caused by
https://codereview.chromium.org/2275493002/.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress
==========

[hiroshige, 2016-12-29 21:52:33]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 21:52:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-12-29 22:20:58]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:21:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-12-29 22:35:59]

hiroshige@chromium.org changed reviewers:
+ rego@igalia.com, timloh@chromium.org, yosin@chromium.org

[hiroshige, 2016-12-29 22:36:00]

PTAL.

yhirano@, japhet@ for loading.

rego@, yosin@, timloh@ for editing/caret test failures:

This CL makes the timing of Document load events earlier, and breaks
editing/caret/caret-color-*.html tests.
This is because actual focusing of an autofocused element occurs after
Document load event:
  Document::setAutofocusElement() is called before Document load event, but
  it posts asynchronously runAutoFocusTask() that calls Element::focus().
  If runAutoFocusTask() is called after Document load event (which is the case
  with this CL), tests fails with text diffs.
I checked the spec
https://www.w3.org/TR/html5/forms.html#autofocusing-a-form-control:-the-autof...
and it seems the focusing steps can be before or after Document load event.

Does anyone have idea to fix the tests deterministically?

[commit-bot: I haz the power, 2016-12-30 00:06:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-30 00:06:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Manuel Rego, 2016-12-30 09:08:56]

On 2016/12/29 22:36:00, hiroshige (OOO) wrote:
> rego@, yosin@, timloh@ for editing/caret test failures:
> 
> This CL makes the timing of Document load events earlier, and breaks
> editing/caret/caret-color-*.html tests.
> This is because actual focusing of an autofocused element occurs after
> Document load event:
>   Document::setAutofocusElement() is called before Document load event, but
>   it posts asynchronously runAutoFocusTask() that calls Element::focus().
>   If runAutoFocusTask() is called after Document load event (which is the case
>   with this CL), tests fails with text diffs.
> I checked the spec
>
https://www.w3.org/TR/html5/forms.html#autofocusing-a-form-control:-the-autof...
> and it seems the focusing steps can be before or after Document load event.
> 
> Does anyone have idea to fix the tests deterministically?


Regarding the caret-color tests, we've the possibility to modify them
as we need. They're imported manually from W3C csswg-test repo
(because they're not ref-tests).

Could you check if using something like this instead of "autofocus"
will fix the tests?
  <textarea id="textarea"></textarea>
  ...
  <script>
    window.onload = function() {
      document.getElementById("textarea").focus();
    }
  </script>

That's already used in caret-color-011.html, and that test
is not failing with this CL.
So hopefully that could be a solution, if you confirm it I'll update
the tests at csswg-tests repo too.

BTW, regarding the results of these tests, I was expecting that
we also get different images, but it seems that part is right and
we only get a text diff. That text diff is not really relevant
for caret-color tests (what we care is about the images).
But it seems a better idea to modify the tests
if the proposed solution works.

[hiroshige, 2017-01-03 19:49:21]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 19:49:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-01-03 21:47:24]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 21:47:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 23:24:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 23:24:18]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-05 22:59:46]

On 2016/12/30 09:08:56, Manuel Rego wrote:
> On 2016/12/29 22:36:00, hiroshige (OOO) wrote:
> > rego@, yosin@, timloh@ for editing/caret test failures:
> > 
> > This CL makes the timing of Document load events earlier, and breaks
> > editing/caret/caret-color-*.html tests.
> > This is because actual focusing of an autofocused element occurs after
> > Document load event:
> >   Document::setAutofocusElement() is called before Document load event, but
> >   it posts asynchronously runAutoFocusTask() that calls Element::focus().
> >   If runAutoFocusTask() is called after Document load event (which is the
case
> >   with this CL), tests fails with text diffs.
> > I checked the spec
> >
>
https://www.w3.org/TR/html5/forms.html#autofocusing-a-form-control:-the-autof...
> > and it seems the focusing steps can be before or after Document load event.
> > 
> > Does anyone have idea to fix the tests deterministically?
> 
> 
> Regarding the caret-color tests, we've the possibility to modify them
> as we need. They're imported manually from W3C csswg-test repo
> (because they're not ref-tests).
> 
> Could you check if using something like this instead of "autofocus"
> will fix the tests?
>   <textarea id="textarea"></textarea>
>   ...
>   <script>
>     window.onload = function() {
>       document.getElementById("textarea").focus();
>     }
>   </script>
> 
> That's already used in caret-color-011.html, and that test
> is not failing with this CL.
> So hopefully that could be a solution, if you confirm it I'll update
> the tests at csswg-tests repo too.
> 
> BTW, regarding the results of these tests, I was expecting that
> we also get different images, but it seems that part is right and
> we only get a text diff. That text diff is not really relevant
> for caret-color tests (what we care is about the images).
> But it seems a better idea to modify the tests
> if the proposed solution works.

rego@, Thanks for your suggestion!
So these tests are not testing autofocus behavior, right?
By updating the tests and -expected.txt (only to add "LayoutText {#text} at
(0,0) size 0x0"), the tests passed (Patch Set 6).

[Manuel Rego, 2017-01-09 09:11:01]

On 2017/01/05 22:59:46, hiroshige wrote:
> rego@, Thanks for your suggestion!
> So these tests are not testing autofocus behavior, right?
> By updating the tests and -expected.txt (only to add "LayoutText {#text} at
> (0,0) size 0x0"), the tests passed (Patch Set 6).

Yes, these tests are not testing "autofocus" at all
The changes on these tests seem fine to me.
Actually I'm going to propose them upstream too (on csswg-tests repo).

[hiroshige, 2017-01-13 18:41:32]

PTAL,

japhet@ as a core/ OWNER,
rego@ for editing/ test changes.

[Manuel Rego, 2017-01-16 08:24:51]

On 2017/01/13 18:41:32, hiroshige wrote:
> rego@ for editing/ test changes.

Regarding tests they seem good to me,
however as I've already updated them upstream (W3C csswg-test repo):
https://github.com/w3c/csswg-test/pull/1169

I think it's a better idea to import them in a different patch,
so I've done that in: https://codereview.chromium.org/2632963002/
There are some slight differences on the expected results compared to your
changes,
due to the position of the <script> tag that are not important at all.

Now you won't need any change on the caret-color tests for this patch.

[hiroshige, 2017-01-17 19:46:42]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 19:47:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 23:29:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:29:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2017-01-17 23:38:15]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 23:38:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 03:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 03:19:47]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-18 19:55:55]

On 2017/01/16 08:24:51, Manuel Rego wrote:
> On 2017/01/13 18:41:32, hiroshige wrote:
> > rego@ for editing/ test changes.
> 
> Regarding tests they seem good to me,
> however as I've already updated them upstream (W3C csswg-test repo):
> https://github.com/w3c/csswg-test/pull/1169
> 
> I think it's a better idea to import them in a different patch,
> so I've done that in: https://codereview.chromium.org/2632963002/
> There are some slight differences on the expected results compared to your
> changes,
> due to the position of the <script> tag that are not important at all.
> 
> Now you won't need any change on the caret-color tests for this patch.

Great, thanks!
I rebased this CL.

[hiroshige, 2017-01-19 20:25:57]

hiroshige@chromium.org changed reviewers:
- rego@igalia.com, timloh@chromium.org, yosin@chromium.org

[hiroshige, 2017-01-19 20:25:58]

Nate, could you also look at this?

(Removed editing-related reviewers as the related test changes are already
committed in a separate CL)

[Nate Chapin, 2017-01-19 20:39:09]

lgtm

[hiroshige, 2017-01-19 21:55:28]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-19 21:56:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 23:23:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484862928837930,
"parent_rev": "29aa23c34afa7f5879aa6563a025c388b80c96a5", "commit_rev":
"0712ce38d1a05590e731d07f9aad4b46870f53ae"}

[commit-bot: I haz the power, 2017-01-19 23:23:48]

Description was changed from

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <style>'s load event to
document's load event, because HTMLStyleElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress caused by
https://codereview.chromium.org/2275493002/.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress
==========

to

==========
Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <style>'s load event to
document's load event, because HTMLStyleElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This fixes performance (memory consumption) regression in
memory.top_10_mobile_stress caused by
https://codereview.chromium.org/2275493002/.

BUG=663823, 624697
TEST=https_www_google_co_uk_hl_en_q_science
  in memory:chrome:all_processes:reported_by_chrome:v8:effective_size
  in memory.top_10_mobile_stress

Review-Url: https://codereview.chromium.org/2605173002
Cr-Commit-Position: refs/heads/master@{#444886}
Committed:
https://chromium.googlesource.com/chromium/src/+/0712ce38d1a05590e731d07f9aad...
==========

[commit-bot: I haz the power, 2017-01-19 23:23:49]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0712ce38d1a05590e731d07f9aad...