Clovis: contentful paint upgrades.

Clovis: contentful paint upgrades.

Use last contentful paint event for timing, and emit request fingerprints so
that critical resources can be collated across runs. This adds the implied lens
that defines critical resources from a fingerprint list.

Committed: https://crrev.com/89dbec666c4357c58b3b7ba748cfb842059f90b8
Cr-Commit-Position: refs/heads/master@{#390359}

[mattcary, 2016-04-15 15:15:37]

mattcary@chromium.org changed reviewers:
+ blundell@chromium.org, lizeb@chromium.org

[mattcary, 2016-04-15 15:15:37]

Hi Clovis!

Here's updates to the contentful paint lens. The most immediate practical change
is to start using the last rather than the first paint. The fingerprinting
ability is only useful in conjunction with some other support for extracting &
combining multiple traces, which is dependent on sandwich/WPR support and so not
clear how to integrate into Clovis.

[blundell, 2016-04-19 14:26:56]

On 2016/04/15 15:15:37, mattcary wrote:
> Hi Clovis!
> 
> Here's updates to the contentful paint lens. The most immediate practical
change
> is to start using the last rather than the first paint.

I think that the change we actually want is to use the first contentful paint
from the main frame.

> The fingerprinting
> ability is only useful in conjunction with some other support for extracting &
> combining multiple traces, which is dependent on sandwich/WPR support and so
not
> clear how to integrate into Clovis.

[mattcary, 2016-04-26 11:43:47]

On 2016/04/19 14:26:56, blundell (OOO until May 9) wrote:
> On 2016/04/15 15:15:37, mattcary wrote:
> > Hi Clovis!
> > 
> > Here's updates to the contentful paint lens. The most immediate practical
> change
> > is to start using the last rather than the first paint.
> 
> I think that the change we actually want is to use the first contentful paint
> from the main frame.
> 
> > The fingerprinting
> > ability is only useful in conjunction with some other support for extracting
&
> > combining multiple traces, which is dependent on sandwich/WPR support and so
> not
> > clear how to integrate into Clovis.

Yes. This is an approximation to that, as the frame ids in the tracing events
are different than the ones in the Page domain where we currently figure out the
main frame.

Can we get this change in as it matches the most recent analysis I did?
Correlating the frame ids is a bigger change.

[mattcary, 2016-04-27 08:04:27]

Ah, I see now you added the frame detection code. Thanks! Incorporating that.

[mattcary, 2016-04-27 10:22:15]

Updated to use the main frame. Re-ran the wiki science page tests and the
results are consistent with my report---on that page, at least, the main frame
was the last contentful paint.

[Benoit L, 2016-04-27 12:40:50]

lgtm, thanks.

However I'm not sure that I see the rationale behind the introduction of the
"fingerprint". Is it to compare several loads of the same page? And then, why
don't you want to use the full URL.

https://codereview.chromium.org/1888343003/diff/20001/tools/android/loading/u...
File tools/android/loading/user_satisfied_lens.py (right):

https://codereview.chromium.org/1888343003/diff/20001/tools/android/loading/u...
tools/android/loading/user_satisfied_lens.py:64: self._critical_requests = [rq
for rq in trace.request_track.GetEvents()
The fingerprint is constructed from the URL. What happens when the same URL is
fetched several times?

[mattcary, 2016-04-27 12:47:47]

On 2016/04/27 12:40:50, Benoit L wrote:
> lgtm, thanks.
> 
> However I'm not sure that I see the rationale behind the introduction of the
> "fingerprint". Is it to compare several loads of the same page? And then, why
> don't you want to use the full URL.
> 
The fingerprint is for doing the comparing of resources across runs. I wanted to
abstract it away from the URL for two reasons: (1) there are inconveniently long
urls (like inline data) that are convenient to shorten, and (2) I wanted to
start abstracting away from URL for when we do things like compare content or
size or whatever to deal with resources with per-session urls.


>
https://codereview.chromium.org/1888343003/diff/20001/tools/android/loading/u...
> File tools/android/loading/user_satisfied_lens.py (right):
> 
>
https://codereview.chromium.org/1888343003/diff/20001/tools/android/loading/u...
> tools/android/loading/user_satisfied_lens.py:64: self._critical_requests = [rq
> for rq in trace.request_track.GetEvents()
> The fingerprint is constructed from the URL. What happens when the same URL is
> fetched several times?

[mattcary, 2016-04-27 12:47:55]

https://codereview.chromium.org/1888343003/diff/20001/tools/android/loading/u...
File tools/android/loading/user_satisfied_lens.py (right):

https://codereview.chromium.org/1888343003/diff/20001/tools/android/loading/u...
tools/android/loading/user_satisfied_lens.py:64: self._critical_requests = [rq
for rq in trace.request_track.GetEvents()
On 2016/04/27 12:40:50, Benoit L wrote:
> The fingerprint is constructed from the URL. What happens when the same URL is
> fetched several times?

It's counted as the same request. I wasn't sure what the best way to deal with
multiple requests. Do we want to check that it's the same number of times or not
(ie, http://foo # 2, or http://foo # 2 of 5)? Might fetch order not always be
the same?

Since I think the bigger issue is identifying resources that have different
URLs, rather than splitting those with the same URL, I didn't think it was worth
spending much time on this now.

[mattcary, 2016-04-28 11:09:12]

The CQ bit was checked by mattcary@chromium.org

[commit-bot: I haz the power, 2016-04-28 11:09:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888343003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888343003/20001

[commit-bot: I haz the power, 2016-04-28 11:10:54]

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

[commit-bot: I haz the power, 2016-04-28 11:10:56]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[mattcary, 2016-04-28 12:03:02]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2016-04-28 12:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1888343003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-28 12:03:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888343003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888343003/40001

[commit-bot: I haz the power, 2016-04-28 12:47:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-30 17:17:53]

Patchset 3 (id:??) landed as
https://crrev.com/89dbec666c4357c58b3b7ba748cfb842059f90b8
Cr-Commit-Position: refs/heads/master@{#390359}

[commit-bot: I haz the power, 2016-04-30 17:55:24]

Description was changed from

==========
Clovis: contentful paint upgrades.

Use last contentful paint event for timing, and emit request fingerprints so
that critical resources can be collated across runs. This adds the implied lens
that defines critical resources from a fingerprint list.
==========

to

==========
Clovis: contentful paint upgrades.

Use last contentful paint event for timing, and emit request fingerprints so
that critical resources can be collated across runs. This adds the implied lens
that defines critical resources from a fingerprint list.

Committed: https://crrev.com/89dbec666c4357c58b3b7ba748cfb842059f90b8
Cr-Commit-Position: refs/heads/master@{#390359}
==========