core(critical-request-chains): Remove iframe as Critical Request by wardpeet · Pull Request #3583 · GoogleChrome/lighthouse

wardpeet · 2017-10-16T21:58:18Z

It was pretty hard after all to see if a html document was an iframe or not.
I needed this for the preload audit.

patrickhulce · 2017-10-17T18:16:37Z

lighthouse-core/gather/computed/critical-request-chains.js

    const resourceTypeCategory = request._resourceType && request._resourceType._category;

+    // Iframes are considered High Priority but they are not render blocking
+    const isIframe = resourceTypeCategory === WebInspector.resourceTypes.Document._category


is there an actual initiator type we can look at? what about frameId would that help us?

parse or script I guess. Main document is set to other. What does frameid means?

requests have a frameId associated with them, so I'm wondering if we can use that to check if the request was for a frame other than the main frame

https://chromedevtools.github.io/devtools-protocol/tot/Network/

patrickhulce · 2017-10-18T17:36:55Z

lighthouse-core/gather/computed/critical-request-chains.js

   * @see https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuMlc
-   * @param  {any} request
+   * @param {any} request
+   * @param {!WebInspector.NetworkRequest}


nit: missing mainResource name

patrickhulce · 2017-10-18T17:37:09Z

lighthouse-core/gather/computed/critical-request-chains.js


+    // Iframes are considered High Priority but they are not render blocking
+    const isIframe = resourceTypeCategory === WebInspector.resourceTypes.Document._category
+      && request.frameId !== mainResource.frameId;


perfect! 💯

niiiiice. let's use _frameId tho. i think that public api will be changing in our next bump.. haha.

i just verified that the frameid of the iframe resource is the new iframe.. we're good!

patrickhulce · 2017-10-18T17:38:10Z

lighthouse-core/gather/computed/critical-request-chains.js

    const resourceTypeCategory = request._resourceType && request._resourceType._category;

+    // Iframes are considered High Priority but they are not render blocking
+    const isIframe = resourceTypeCategory === WebInspector.resourceTypes.Document._category


the other examples in here I believe are a bit more verbose than they need to be, you should be able to just check request._resourceType === WebInspector.resourceTypes.Document

patrickhulce · 2017-10-18T17:40:19Z

lighthouse-core/test/runner-test.js

    return Runner.run({}, {url, config}).then(results => {
      const audits = results.audits;
-      assert.equal(audits['critical-request-chains'].displayValue, 9);
+      assert.equal(audits['critical-request-chains'].displayValue, 5);


this is the good kind of test change, right? :D

(i.e. we were wrong before and now we're right)

yes, you can see the diff here.
https://www.diffchecker.com/ec6WBOxh

As you can see we removed 3 iframes from the critical requests:

https://www.redditmedia.com/ads/display/300x250-companion

https://www.redditmedia.com/gtm/jail?cb=8CqR7FcToP

https://www.redditmedia.com/ads/display/300x250

so in total we removed 5 leaf nodes but we only remove 4 chains as https://www.redditstatic.com/reddit-init.en.belOwpfqWtw.jsis still a root node.

https://www.redditstatic.com/ad-dependencies.5O7sMdAReBw.js

https://www.redditstatic.com/companion.KW1mOM2pJek.js

https://www.redditstatic.com/gtm.aX_QHhLRPyo.js

https://www.redditstatic.com/ad-dependencies.5O7sMdAReBw.js

https://www.redditstatic.com/display.PGSStRgaC6w.js

patrickhulce · 2017-10-18T17:41:36Z

lighthouse-core/test/gather/computed/critical-request-chains-test.js

+    networkRecords[1].url = 'https://example.com/iframe.html';
+    networkRecords[1].mimeType = 'text/html';
+    networkRecords[1]._resourceType._category = WebInspector.resourceTypes.Document._category;
+    networkRecords[0].frameId = '2';


is this intentional to keep changing frameId of the root document?

also is there a document request case that is within the same frame we should add to tests?

Only html documents that are being fetched are Documents from iframe. XHR is not counted here

paulirish

this'll be good once it comes back. nice work

paulirish · 2017-10-19T01:44:46Z

lighthouse-core/gather/computed/critical-request-chains.js


+    // Iframes are considered High Priority but they are not render blocking
+    const isIframe = resourceTypeCategory === WebInspector.resourceTypes.Document._category
+      && request.frameId !== mainResource.frameId;


niiiiice. let's use _frameId tho. i think that public api will be changing in our next bump.. haha.

paulirish · 2017-10-19T01:45:22Z

lighthouse-core/gather/computed/critical-request-chains.js


+    // Iframes are considered High Priority but they are not render blocking
+    const isIframe = resourceTypeCategory === WebInspector.resourceTypes.Document._category
+      && request.frameId !== mainResource.frameId;


i just verified that the frameid of the iframe resource is the new iframe.. we're good!

paulirish · 2017-10-19T01:47:05Z

lighthouse-core/gather/computed/critical-request-chains.js

-  static isCritical(request) {
+  static isCritical(request, mainResource) {
    const resourceTypeCategory = request._resourceType && request._resourceType._category;



should we add a console.assert(mainResource, 'mainResource not provided')?

we'll need https://eslint.org/docs/rules/no-console#options set up

I could wrap it between disable statements? This is just for debugging purposes right?

actually instead:

const assert = require('assert'); ... assert.ok(mainResource, 'mainResource not provided');

paulirish · 2017-10-19T01:48:49Z

lighthouse-core/test/gather/computed/critical-request-chains-test.js

+    // 2nd record is an iframe in the page
+    networkRecords[1].url = 'https://example.com/iframe.html';
+    networkRecords[1].mimeType = 'text/html';
+    networkRecords[1]._resourceType._category = WebInspector.resourceTypes.Document._category;


along with the change above, this can now be

networkRecords[1]._resourceType = WebInspector.resourceTypes.Document;

patrickhulce

awesome thanks @wardpeet!

wardpeet added 2 commits October 16, 2017 23:56

core(critical-request-chains): Remove iframe as Critical Request

39b42d0

Fix tests

c4f7d4d

patrickhulce reviewed Oct 17, 2017

View reviewed changes

paulirish added the waiting4committer label Oct 17, 2017

Use frameid instead of initatior type to see if a request is an iframe

3ffac65

wardpeet requested a review from paulirish as a code owner October 18, 2017 05:46

patrickhulce suggested changes Oct 18, 2017

View reviewed changes

paulirish approved these changes Oct 19, 2017

View reviewed changes

wardpeet added 3 commits October 19, 2017 07:14

Fixed review changes

e9d4f28

Fix unit tests

c85fb5e

Add assert.ok to give notice when mainresource is forgotten.

95d3b3c

wardpeet added waiting4reviewer and removed waiting4committer labels Oct 19, 2017

Fix linting

dcbad26

paulirish assigned patrickhulce Oct 20, 2017

patrickhulce approved these changes Oct 20, 2017

View reviewed changes

patrickhulce merged commit cd14d38 into master Oct 20, 2017

patrickhulce deleted the bug/remove-iframe-critical branch October 20, 2017 02:37

ghost mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

paulirish removed the waiting4reviewer label Mar 6, 2018

Conversation

wardpeet commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulirish Oct 19, 2017 •

edited

Loading