core(fr): convert trace-elements gatherer #12442

adamraine · 2021-05-04T22:55:05Z

Adds support for TraceElements gatherer in FR.

connorjclark · 2021-05-04T22:59:03Z

lighthouse-core/gather/gatherers/trace-elements.js

@@ -308,10 +314,28 @@ class TraceElements extends Gatherer {
      }
    }

-    await driver.sendCommand('Animation.disable');
+    session.sendCommand('Animation.disable');


should these be in stopInstrumentation? does it matter?

this is tricky, I'm assuming that Animation domain needs to remain enabled for Animation.resolveAnimation to work?

if not, then yeah definitely move to stopInstrumentation

why was the await removed though? I seem to recall bad things happening in LR if there are unhandled promise rejections

alternate option: collect animation id -> name mapping by listening for Animation.animationCreated and fetching at the end of stopInstrumentation

I tried this and yeah, Animation domain needs to be enabled. When I moved it back I forgot the await though haha.

what do you think about the alternate option @adamraine? it's more code and possible more animation resolution at the benefit of more idiomatic "instrumentation" followed by the artifact computation.

I could see us going either way, just interested in your thoughts.

We're still going to have some "instrumentation" in getArtifact when we call Runtime.callFunctionOn. Some less destructive options I came up with:

Remove the call to Animation.disable entirely

Call Animation.disable in stopInstrumentation, then reenable it for getArtifact.

I'll give your suggestion a shot though to see how destructive it is.

So I went a little deep down this rabbit hole. I had trouble resolving the animation name from animationCreated events, however I found that we can just get the animation name directly from animationStarted events. Kinda wish I had thought of this when I first added animated elements to TraceElements 😆

lighthouse-core/test/gather/gatherers/trace-elements-test.js

patrickhulce · 2021-05-05T02:30:22Z

lighthouse-core/test/fraggle-rock/api-test-pptr.js

@@ -159,7 +159,7 @@ describe('Fraggle Rock API', () => {
      const {lhr} = result;
      const {auditResults, failedAudits, erroredAudits} = getAuditsBreakdown(lhr);
      // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
-      expect(auditResults.length).toMatchInlineSnapshot(`103`);
+      expect(auditResults.length).toMatchInlineSnapshot(`105`);


~~hm, didn't bump the timespan one?~~ oh right trace isn't supported in timespan yet 👍

patrickhulce

nice! I like this a lot 👍 feels exactly as it would be if we wrote it with FR in mind from the first place :)

lighthouse-core/gather/gatherers/trace-elements.js

patrickhulce · 2021-05-05T19:10:46Z

lighthouse-core/gather/gatherers/trace-elements.js

+   * @param {LH.Gatherer.FRTransitionalContext} context
+   */
+  async stopInstrumentation(context) {
+    if (this.onAnimationCreated) {


could avoid this if by moving onAnimationCreated to class property next to the map, but understand if you'd rather keep the impl inside startInstrumentation :)

why not a private function?

Needed access to this, I wound up defining it in a new constructor.

See JSUsage gatherer constructor for how to use this in class callback functions.

lighthouse-core/test/gather/gatherers/trace-elements-test.js

lighthouse-core/gather/gatherers/trace-elements.js

connorjclark · 2021-05-05T19:33:05Z

lighthouse-core/gather/gatherers/trace-elements.js

+   * @param {LH.Gatherer.FRTransitionalContext} context
+   */
+  async stopInstrumentation(context) {
+    if (this.onAnimationCreated) {


why not a private function?

patrickhulce · 2021-05-05T21:06:57Z

lighthouse-core/gather/gatherers/trace-elements.js

+  constructor() {
+    super();
+    this._onAnimationStarted = this._onAnimationStarted.bind(this);
+  }
+
+  /** @param {LH.Crdp.Animation.AnimationStartedEvent} args */
+  _onAnimationStarted({animation: {id, name}}) {
+    if (name) this.animationIdToName.set(id, name);
+  }


Suggested change

constructor() {

super();

this._onAnimationStarted = this._onAnimationStarted.bind(this);

}

/** @param {LH.Crdp.Animation.AnimationStartedEvent} args */

_onAnimationStarted({animation: {id, name}}) {

if (name) this.animationIdToName.set(id, name);

}

/** @param {LH.Crdp.Animation.AnimationStartedEvent} args */

_onAnimationStarted = ({animation: {id, name}}) => {

if (name) this.animationIdToName.set(id, name);

}

FWIW, this was my suggestion if you're going to separate already :)

Can't do it, need access to this

that shouldn't be a problem

$ cat > foo.js <<EOF class Foo { x = 1 foo = () => this.x } console.log(new Foo().foo()) const detached = new Foo().foo console.log(detached()) // this works too :) EOF $ node foo.js 1 1

Unfortunately eslint disagrees. I guess it technically works though.

That's because they don't support class fields eslint/eslint#14343

adamraine added 6 commits May 4, 2021 18:32

core(fr): convert trace-elements gatherer

abbff10

rn context

9070191

fix tests

ffe39dc

convert old tests

10cf229

i cant decide

e30f536

add to config

f73fc4b

adamraine requested a review from a team as a code owner May 4, 2021 22:55

adamraine requested review from connorjclark and removed request for a team May 4, 2021 22:55

google-cla bot added the cla: yes label May 4, 2021

devtools-bot assigned connorjclark May 4, 2021

devtools-bot added the waiting4reviewer label May 4, 2021

connorjclark approved these changes May 4, 2021

View reviewed changes

mspaansen approved these changes May 5, 2021

View reviewed changes

lighthouse-core/test/gather/gatherers/trace-elements-test.js Outdated Show resolved Hide resolved

mspaansen approved these changes May 5, 2021

View reviewed changes

patrickhulce reviewed May 5, 2021

View reviewed changes

GoogleChrome deleted a comment from mspaansen May 5, 2021

use instrumentation

8cd5172

vercel bot deployed to Preview May 5, 2021 18:03 View deployment

patrickhulce approved these changes May 5, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels May 5, 2021

connorjclark reviewed May 5, 2021

View reviewed changes

comments

4db9ee9

vercel bot deployed to Preview May 5, 2021 20:12 View deployment

bind

4935e46

vercel bot deployed to Preview May 5, 2021 20:49 View deployment

patrickhulce reviewed May 5, 2021

View reviewed changes

adamraine merged commit 0b00b5c into master May 6, 2021

adamraine deleted the fr-trace-elements branch May 6, 2021 15:04

paulirish pushed a commit that referenced this pull request May 13, 2021

core(fr): convert trace-elements gatherer (#12442)

b080e30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(fr): convert trace-elements gatherer #12442

core(fr): convert trace-elements gatherer #12442

adamraine commented May 4, 2021

connorjclark May 4, 2021

patrickhulce May 5, 2021

patrickhulce May 5, 2021

patrickhulce May 5, 2021

adamraine May 5, 2021

patrickhulce May 5, 2021

adamraine May 5, 2021

adamraine May 5, 2021

patrickhulce May 5, 2021 •

edited

Loading

patrickhulce left a comment

patrickhulce May 5, 2021

connorjclark May 5, 2021

adamraine May 5, 2021

connorjclark May 5, 2021

connorjclark May 5, 2021

patrickhulce May 5, 2021

adamraine May 5, 2021

patrickhulce May 5, 2021 •

edited

Loading

adamraine May 5, 2021 •

edited

Loading

patrickhulce May 6, 2021

core(fr): convert trace-elements gatherer #12442

core(fr): convert trace-elements gatherer #12442

Conversation

adamraine commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 5, 2021 • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 5, 2021 • edited Loading

Choose a reason for hiding this comment

adamraine May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 5, 2021 •

edited

Loading

patrickhulce May 5, 2021 •

edited

Loading

adamraine May 5, 2021 •

edited

Loading