Ensure the scheduler always changes from UseCase LOADING to NONE.

Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2738813003
Cr-Commit-Position: refs/heads/master@{#457049}
Committed: https://chromium.googlesource.com/chromium/src/+/9d780d130566384f6aff63f127e92340f31a815d

[Dan Elphick, 2017-03-08 16:28:08]

Description was changed from

==========
Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
==========

to

==========
Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Dan Elphick, 2017-03-08 17:22:31]

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

[commit-bot: I haz the power, 2017-03-08 17:22:55]

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-03-08 18:11:36]

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

[commit-bot: I haz the power, 2017-03-08 18:11:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Elphick, 2017-03-09 12:30:29]

Description was changed from

==========
Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Dan Elphick, 2017-03-09 12:30:29]

delphick@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2017-03-09 13:35:07]

Looks great -- could we also test this in FirstMeaningfulPaintDetectorTest?

[Dan Elphick, 2017-03-13 11:34:52]

On 2017/03/09 13:35:07, Sami wrote:
> Looks great -- could we also test this in FirstMeaningfulPaintDetectorTest?

Added a test. WDYT?

[Sami, 2017-03-13 12:06:34]

Thanks, lgtm!

[Dan Elphick, 2017-03-13 12:15:27]

delphick@chromium.org changed reviewers:
+ kouhei@chromium.org

[Dan Elphick, 2017-03-13 12:17:25]

+kouhei@ This extends Sami's previous work and ensures that we always leave the
LOADING state.

[kouhei (in TOK), 2017-03-13 13:04:47]

kouhei@chromium.org changed reviewers:
+ ksakamoto@google.com

[kouhei (in TOK), 2017-03-13 13:04:47]

+ksakamoto

[kouhei (in TOK), 2017-03-13 13:05:01]

kouhei@chromium.org changed reviewers:
+ ksakamoto@chromium.org
- ksakamoto@google.com

[kouhei (in TOK), 2017-03-13 19:22:49]

https://codereview.chromium.org/2738813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2738813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:129:
m_paintTiming->markFirstMeaningfulPaintCandidate();
I'm convinced that we need to signal scheduler here, but I'm not sure if we
should mark PaintTiming::m_firstMeaningfulPaintCandidate.
delegating review to ksakamoto@

[Kunihiko Sakamoto, 2017-03-14 01:44:26]

https://codereview.chromium.org/2738813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2738813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:129:
m_paintTiming->markFirstMeaningfulPaintCandidate();
On 2017/03/13 19:22:48, kouhei wrote:
> I'm convinced that we need to signal scheduler here, but I'm not sure if we
> should mark PaintTiming::m_firstMeaningfulPaintCandidate.
> delegating review to ksakamoto@

Yeah, this would set a non-paint timestamp to
PaintTiming::m_firstMeaningfulPaintCandidate.

Instead, would you change

  PaintTiming::markFirstMeaningfulPaintCandidate() {
     ...
     m_firstMeaningfulPaintCandidate = monotonicallyIncreasingTime();
     ...
  }

to

  PaintTiming::setFirstMeaningfulPaintCandidate(double timestamp) {
     ...
     m_firstMeaningfulPaintCandidate = timestamp;
     ...
  }

and call it with m_provisionalFirstMeaningfulPaint?

[Dan Elphick, 2017-03-14 11:11:53]

On 2017/03/14 01:44:26, Kunihiko Sakamoto wrote:
>
https://codereview.chromium.org/2738813003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
> (right):
> 
>
https://codereview.chromium.org/2738813003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:129:
> m_paintTiming->markFirstMeaningfulPaintCandidate();
> On 2017/03/13 19:22:48, kouhei wrote:
> > I'm convinced that we need to signal scheduler here, but I'm not sure if we
> > should mark PaintTiming::m_firstMeaningfulPaintCandidate.
> > delegating review to ksakamoto@
> 
> Yeah, this would set a non-paint timestamp to
> PaintTiming::m_firstMeaningfulPaintCandidate.
> 
> Instead, would you change
> 
>   PaintTiming::markFirstMeaningfulPaintCandidate() {
>      ...
>      m_firstMeaningfulPaintCandidate = monotonicallyIncreasingTime();
>      ...
>   }
> 
> to
> 
>   PaintTiming::setFirstMeaningfulPaintCandidate(double timestamp) {
>      ...
>      m_firstMeaningfulPaintCandidate = timestamp;
>      ...
>   }
> 
> and call it with m_provisionalFirstMeaningfulPaint?

Done. PTAL

[Kunihiko Sakamoto, 2017-03-15 01:59:19]

lgtm with comments

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp
(right):

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:122:
// candidate.
Remove this line

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintTiming.h:46: void
markFirstMeaningfulPaintCandidate(double timestamp);
Please rename this to setFirstMeaningfulPaintCandidate.

[kouhei (in TOK), 2017-03-15 02:00:26]

rs lgtm

[Dan Elphick, 2017-03-15 09:25:07]

Thanks

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp
(right):

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:122:
// candidate.
On 2017/03/15 01:59:19, Kunihiko Sakamoto wrote:
> Remove this line

Done.

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2738813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintTiming.h:46: void
markFirstMeaningfulPaintCandidate(double timestamp);
On 2017/03/15 01:59:19, Kunihiko Sakamoto wrote:
> Please rename this to setFirstMeaningfulPaintCandidate.

Done.

[Dan Elphick, 2017-03-15 09:26:06]

The CQ bit was checked by delphick@chromium.org

[Dan Elphick, 2017-03-15 09:26:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, ksakamoto@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2738813003/#ps60001
(title: "rename markFirstMeaningfulPaintCandidate to
setFirstMeaningfulPaintCandidate. delete errant comment.")

[commit-bot: I haz the power, 2017-03-15 09:26:20]

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-03-15 10:45:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489569966746190,
"parent_rev": "2100813cb0c0d18a87434921433fda24d04cda16", "commit_rev":
"9d780d130566384f6aff63f127e92340f31a815d"}

[commit-bot: I haz the power, 2017-03-15 10:46:16]

Description was changed from

==========
Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Ensure the scheduler always changes from UseCase LOADING to NONE.

Always mark a first meaningful paint candidate to ensure the scheduler
changes UseCase from LOADING to NONE. This is necessary because fast
loading pages may only see one contentful paint which means the FMPD
doesn't fire again and the Scheduler stays in LOADING long after the page
is done.

BUG=699101
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2738813003
Cr-Commit-Position: refs/heads/master@{#457049}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d780d130566384f6aff63f127e9...
==========

[commit-bot: I haz the power, 2017-03-15 10:46:17]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9d780d130566384f6aff63f127e9...