DBW: add console.time -> performance.mark() suggestion #712

ebidel · 2016-09-28T04:14:55Z

R: @brendankenny @paulirish @GoogleChrome/lighthouse

Makes article naming consistent across similar audits
Also refactors some common code for recording function calls into driver.

Usage is much simpler now to record API calls:

class DateNowUse extends Gatherer {
  beforePass(options) {
    this.collectUsage = options.driver.captureJSCalls(
        'Date.now', 'window.__dateNowStackTraces');
  }
  afterPass() {
    return this.collectUsage().then(dateNowUses => {
      this.artifact.usage = dateNowUses;
    }, _ => {
      this.artifact = -1;
      return;
    });
  }
}

- Also refactors common code into driver

brendankenny

a couple of initial thoughts :)

brendankenny · 2016-09-28T06:17:22Z

lighthouse-core/audits/dobetterweb/no-console-time.js

+        prev.push(err);
+      }
+      return prev;
+    }, []);


could also do something like

// Filter usage from other hosts. const results = artifacts.ConsoleTimeUsage.usage.filter(err => { return url.parse(err.url).host === pageHost; }).map(err => `(line: ${err.line}, col: ${err.col})`);

brendankenny · 2016-09-28T06:20:40Z

lighthouse-core/audits/dobetterweb/no-datenow.js

-      err.misc = `(line: ${err.line}, col: ${err.col})`;
-      return err;
-    });
+    }, []);


brendankenny · 2016-09-28T06:26:07Z

lighthouse-core/gather/gatherers/dobetterweb/console-time-usage.js

+  beforePass(options) {
+    const driver = options.driver;
+    return driver.evaluateScriptOnLoad(`
+        window.__consoleTimeStackTraces = new Set();


this is still a bit fiddly on the caller's side.

Could more of this be pulled into driver? Maybe captureAPIUsage could set up the collecting as it does here but also handles the evaluateScriptOnLoad part and returns a collectUsage function that could be called whenever (probably usually in afterPass) that takes care of fetching the array of callsites for you? all without having to know that you're evaluating a script, that the callsites are on __consoleTimeStackTraces in the host page, etc

But doesn't the evaluateScriptOnLoad stuff need to be setup in beforePass? Or can that be pushed off until afterPass time?

yeah, I meant like, in beforePass, have driver.captureAPIUsage().then(collectUsage => this.collectUsage = collectUsage) and then in afterPass have something like return this.collectUsage.then(uses => this.artifact = uses) or whatever

Yep, exactly what I ended up doing.

ebidel · 2016-09-28T18:18:33Z

PTAL

brendankenny

reviewed!

brendankenny · 2016-09-28T19:26:02Z

lighthouse-core/audits/dobetterweb/no-console-time.js

+      return url.parse(err.url).host === pageHost;
+    }).map(err => {
+      err.misc = `(line: ${err.line}, col: ${err.col})`;
+      return err;


shouldn't mutate the errors in the artifact. Maybe

return Object.assign({ misc: `(line: ${err.line}, col: ${err.col})` }, err);

?

brendankenny · 2016-09-28T19:26:19Z

lighthouse-core/audits/dobetterweb/no-datenow.js

+    // Filter usage from other hosts.
+    const results = artifacts.DateNowUse.usage.filter(err => {
+      return url.parse(err.url).host === pageHost;
+    }).map(err => {


brendankenny · 2016-09-28T19:27:23Z

lighthouse-core/gather/drivers/driver.js

+   *     on the global object.
+   * @return {Function} A wrapper around the original function.
+   */
+  static captureUsage(func, set) {


I don't think this should be in the Driver class since it's not a method for it. One option would be to define it outside of the class body to make that clear?

brendankenny · 2016-09-28T19:31:06Z

lighthouse-core/gather/drivers/driver.js

+
+  /**
+   * Tracks function call usage. Used by captureJSCalls to inject code into the page.
+   * @param {Function} func The function call to track.


could do {function(...*): *}, I think

brendankenny · 2016-09-28T19:31:15Z

lighthouse-core/gather/drivers/driver.js

+  /**
+   * Tracks function call usage. Used by captureJSCalls to inject code into the page.
+   * @param {Function} func The function call to track.
+   * @param {Set} set An empty set to populate with stack traces. Should be


brendankenny · 2016-09-28T19:32:32Z

lighthouse-core/gather/drivers/driver.js

+   * Keeps track of calls to a JS function and returns a list of {url, line, col}
+   * of the usage.
+   * @param {string} funcVar The function name to track ('Date.now', 'console.time').
+   * @param {string} variableToPopulate The variable name to populate with stack traces.


couldn't captureJSCalls just generate the variable name? That prevents the gatherers from having to care the variables each other are using

aye. left a comment along those lines, too.

brendankenny · 2016-09-28T19:34:24Z

lighthouse-core/gather/drivers/driver.js

+  captureJSCalls(funcVar, variableToPopulate) {
+    const collectUsage = () => {
+      return this.evaluateAsync(
+          `(__returnResults(Array.from(${variableToPopulate}).map(item => JSON.parse(item))))`);


do you need surrounding parens?

brendankenny · 2016-09-28T19:35:05Z

lighthouse-core/gather/drivers/driver.js

+
+  /**
+   * Keeps track of calls to a JS function and returns a list of {url, line, col}
+   * of the usage.


maybe a comment on usage? (must be called before page load, mostly)

brendankenny · 2016-09-28T19:37:13Z

lighthouse-core/gather/drivers/driver.js

+   * @param {string} funcVar The function name to track ('Date.now', 'console.time').
+   * @param {string} variableToPopulate The variable name to populate with stack traces.
+   *     This should unique and on the global object ('window.__dateNowStackTraces').
+   * @return {Function} Call this method when you want results.


@return {function(): !Promise<!Array<{url: string, line: number, col: number}>>} :)

@return {function(): !Promise<!Array<!{url: string, line: number, col: number}>>}

unless the contents of the array can be null also ;)

unless the contents of the array can be null also ;)

record types are non-nullable by default :P

brendankenny · 2016-09-28T19:38:06Z

lighthouse-core/gather/gatherers/dobetterweb/console-time-usage.js

+
+  afterPass() {
+    return this.collectUsage().then(consoleTimeUsage => {
+      this.artifact.usage = consoleTimeUsage;


do these have to saved on this.artifact.usage? Could also just save on this.artifact itself

I like the consistency between audits of this natural. Always accessing the same .usage array.

paulirish · 2016-09-28T19:40:14Z

lighthouse-core/gather/drivers/driver.js

+
+  /**
+   * Tracks function call usage. Used by captureJSCalls to inject code into the page.
+   * @param {Function} func The function call to track.


func to funcRef, just to be clearer it's not the string name of it anymore

brendankenny · 2016-09-28T19:42:26Z

what about captureFunctionCallSites instead of captureJSCalls just to be more precise?

paulirish · 2016-09-28T19:44:09Z

lighthouse-core/gather/drivers/driver.js

+          `(__returnResults(Array.from(${variableToPopulate}).map(item => JSON.parse(item))))`);
+    };
+
+    this.evaluateScriptOnLoad(`


I'm having a hard time parsing this magic. can we split it out a tad more?

I think this is basically what we're doing, eh?

this.evaluateScriptOnLoad(` ${variableToPopulate} = new Set(); function ${Driver.captureUsage.toString()}; ${funcVar} = ${Driver.captureUsage.name}(${funcVar}, ${variableToPopulate}); `);

Slightly more important is renaming funcVar to funcName since it's in string form until the template literal is evaluated

paulirish · 2016-09-28T19:52:56Z

lighthouse-core/gather/drivers/driver.js

+   *     This should unique and on the global object ('window.__dateNowStackTraces').
+   * @return {Function} Call this method when you want results.
+   */
+  captureJSCalls(funcVar, variableToPopulate) {


Kind of prefer an options object for API here.. but what do you guys think?

driver.captureJSCalls({ funcName: 'console.time', globalVarName: 'window.__consoleTimeStackTraces' })

alternatively we can leave it alone because i can imagine we can later refactor out this second argument from the exposed API.

given there's only two args right now, I think it's fine.

paulirish · 2016-09-28T19:54:39Z

this is definitely lgtm after this round.

brendankenny · 2016-09-28T20:05:33Z

lighthouse-core/gather/drivers/driver.js

+   * @return {function(): !Promise<!Array<{url: string, line: number, col: number}>>}
+   *     Call this method when you want results.
+   */
+  captureFunctionCallSites(funcName, globalVarToPopulate) {


why not just, like, const globalVarToPopulate = window['__${funcName}StackTraces']`` and then you'll get it for free and for most funcNames you'll be sure no one else will be overwriting it :)

yea that works

ebidel · 2016-09-28T20:09:32Z

PTAL

brendankenny

Looks good. Great addition to driver

paulirish · 2016-09-28T20:19:00Z

Boom.

DBW: add performance.mark() suggestion

8d8fea8

- Also refactors common code into driver

ebidel added the DoBetterWeb label Sep 28, 2016

Remove spread operator (not supported in Node < 6)

4cfbebb

brendankenny reviewed Sep 28, 2016

View reviewed changes

ebidel added 3 commits September 28, 2016 08:11

Terse filtering

5efdf18

Move to simplier setup for JS call audits

0d6f8d5

Remove duplicate test

558162e

Remove dupe comment

ba0e3b5

brendankenny requested changes Sep 28, 2016

View reviewed changes

paulirish reviewed Sep 28, 2016

View reviewed changes

ebidel added 3 commits September 28, 2016 12:55

Feedback

a0a2fed

Update closure comment

7a25026

Bckenny knows jsdocs

db4dc38

brendankenny reviewed Sep 28, 2016

View reviewed changes

Remove need for caller to pass in global var to populate

9113013

brendankenny approved these changes Sep 28, 2016

View reviewed changes

brendankenny merged commit 307a296 into master Sep 28, 2016

brendankenny deleted the mark branch September 28, 2016 20:18

DBW: add console.time -> performance.mark() suggestion #712

DBW: add console.time -> performance.mark() suggestion #712

Conversation

ebidel commented Sep 28, 2016 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Sep 28, 2016 • edited Loading

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

brendankenny Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Sep 28, 2016

brendankenny 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

brendankenny Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 28, 2016 • edited Loading

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

brendankenny Sep 28, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Sep 28, 2016

paulirish Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Sep 28, 2016

brendankenny Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Sep 28, 2016

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Sep 28, 2016

ebidel commented Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading

ebidel Sep 28, 2016 •

edited

Loading

paulirish Sep 28, 2016 •

edited

Loading

paulirish Sep 28, 2016 •

edited

Loading

brendankenny Sep 28, 2016 •

edited

Loading