core: remove dependency on devtools-frontend NetworkRequest #5451

patrickhulce · 2018-06-08T01:15:35Z

title speaks for itself, big change :)

part of the broader goal of removing our dependency on devtools-frontend

tests should be passing now! 🎉 I want to do more comparisons with my lantern traces and land our safety checks before this one

brendankenny

title speaks for itself

is it more removing dependency on devtools-frontend to create network records?

brendankenny · 2018-06-11T16:36:41Z

typings/web-inspector.d.ts

      _isLinkPreload?: boolean;
-      initiatorRequest(): NetworkRequest | null;
-      redirects?: NetworkRequest[];
+    }


shouldn't we be able to get rid of this and use the class definition?

yes, that jump seemed a bit bigger in the same go since I'm using this to typecheck the other class, but we definitely will be able to

brendankenny · 2018-06-11T16:39:21Z

lighthouse-core/lib/web-inspector.js

@@ -317,5 +317,7 @@ module.exports = (function() {
  // Restore setImmediate, see comment at top.
  global.setImmediate = _setImmediate;

+  // TODO(phulce): replace client requires to not need this
+  WebInspector.resourceTypes = require('../../third-party/devtools/ResourceType').TYPES;


can more be removed from this file?

eventually yes!

patrickhulce · 2018-06-11T17:16:24Z

is it more removing dependency on devtools-frontend to create network records?

er, yes! depends on what we're calling a network record, I guess that's somewhat ambiguous

patrickhulce · 2018-06-11T17:22:37Z

lighthouse-core/test/fixtures/wikipedia-redirect.critical-request-chains.json

@@ -1,11 +1,11 @@
 {
-  "33097.1:redirected.0": {


open question if we want to match exactly the old policy instead?

i dun care. this wfm.

paulirish

can we remove some of the requires from lib/web-inspector now?

paulirish · 2018-06-12T21:06:11Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -165,6 +166,7 @@ class UnusedBytes extends Audit {
      const networkNode = /** @type {NetworkNode} */ (node);
      const originalTransferSize = originalTransferSizes.get(networkNode.record.requestId);
      if (originalTransferSize === undefined) return;
+      networkNode.record.transferSize = originalTransferSize;
      networkNode.record._transferSize = originalTransferSize;


should we just use one of these? this PR is switching to use the other already.
Also happy to fix the _prop use in a followup to minimize the diff.

yeah eliminated _transferSize, the dance was necessary for transferSize since the weird way the getter interacts with our simulation but the rest will be followup 👍

paulirish · 2018-06-12T21:08:55Z

lighthouse-core/audits/uses-rel-preconnect.js

@@ -90,7 +91,7 @@ class UsesRelPreconnectAudit extends Audit {
          // filter out all resources where timing info was invalid
          !UsesRelPreconnectAudit.hasValidTiming(record) ||
          // filter out all resources that are loaded by the document
-          record.initiatorRequest() === mainResource ||
+          record._initiator.url === mainResource.url ||


these aren't totally equivalent, fwiw. but the difference may not matter. And I suppose devtools was making this assumption previously. :p

yeah I was kinda shocked that the whole initiatorRequest logic is literally just first one with matching URL and manager 😮

paulirish · 2018-06-12T21:09:57Z

lighthouse-core/lib/network-recorder.js

@@ -34,6 +38,10 @@ class NetworkRecorder extends EventEmitter {
    );
  }

+  getRecords() {
+    return this._records.slice();


Array.from() ?

haha, man between you and dgozman I'm gonna lose myself! 😆

done

paulirish · 2018-06-12T21:11:21Z

lighthouse-core/lib/network-recorder.js

+    this._records = [];
+    /** @type {Map<string, NetworkRequest>} */
+    this._recordsById = new Map();
+
    this.networkManager = NetworkManager.createWithFakeTarget();


do we still need this around? followup?

that one's totally safe to go right now actually 👍

paulirish · 2018-06-12T21:18:46Z

lighthouse-core/lib/network-recorder.js

+      recordsByURL.set(record.url, record);
+    }
+
+    // set the initiator and redirects array


at first glance, it seems odd that this happens only when we're hydrating the records from logs. what about when records are created via this._networkStatusMonitor.dispatch(event); in driver?

yeah it's a bit weird, AFAICT we actually only ever use the ones created by the recordsFromLogs, the ones created incrementally are just for the status notifications

lighthouse/lighthouse-core/gather/gather-runner.js

Line 301 in 996c5d7

const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);

(and obvs the computed artifact uses this)

long-term though I hope to eliminate the initiatorRequest logic here entirely and move it to the page-dependency-graph since it's just CRC-based ones that need it right now

redirects can also move to direct usage of source/destination which I think becomes more explicit and clear

is there a way to do this incrementally as (presumably) NetworkManager or whatever does? If the same thing is available in the page dependency graph I guess that's fine too, but at that point do we need NetworkRecords...

I really, really want to nix these entirely so network request can be trivially serializable. Because of that, I don't really want to do unnecessary work to make them nice only to remove them

yeah it's a bit weird, AFAICT we actually only ever use the ones created by the recordsFromLogs, the ones created incrementally are just for the status notifications

ha, I think it was in anticipation of exactly this sort of situation. Since audits can only access network records created by recordsFromLogs(devtoolsLogs), we wanted to make sure gatherers also saw exactly the same thing (in afterPass()) and so switched everything to using it.

So this impl and then migration from it sounds good. Nothing should be able to observe the difference since driver never actually inspects the records while listening to the quiet events

paulirish · 2018-06-12T21:27:30Z

lighthouse-core/lib/network-request.js

+  /**
+   * @param {LH.Crdp.Network.ResourceTiming} timing
+   */
+  _recomputeTimesWithResourceTiming(timing) {


add a comment referencing SDK.NetworkRequest.timing setter and link to https://github.com/ChromeDevTools/devtools-frontend/blob/56a99365197\/front_end/sdk/NetworkRequest.js#L485-L502

paulirish · 2018-06-12T21:32:19Z

lighthouse-core/test/fixtures/wikipedia-redirect.critical-request-chains.json

@@ -1,11 +1,11 @@
 {
-  "33097.1:redirected.0": {


i dun care. this wfm.

paulirish · 2018-06-12T21:33:50Z

lighthouse-core/lib/network-recorder.js

-      case 'Network.loadingFailed': return this.onLoadingFailed(event.params);
-      case 'Network.resourceChangedPriority': return this.onResourceChangedPriority(event.params);
-      default: return;
+      case 'Network.requestWillBeSent':


revert the ws change

paulirish · 2018-06-12T21:36:20Z

lighthouse-core/lib/network-request.js

+   * @param {LH.Crdp.Network.Headers} headersMap
+   * @return {Array<LH.WebInspector.HeaderValue>}
+   */
+  static _headersMapToHeadersArray(headersMap) {


headersDict or headersObj or headersHash seem fine..

paulirish · 2018-06-12T21:38:53Z

lighthouse-core/lib/network-request.js

+  static _headersMapToHeadersArray(headersMap) {
+    const result = [];
+    for (const name of Object.keys(headersMap)) {
+      const values = headersMap[name].split('\n');


which method in sdk.networkrequest is this disguised as? _computeHeaderValue

Not even disguised verbatim s/in/of/ ;)
https://github.com/ChromeDevTools/devtools-frontend/blob/3415ee28e86a3f4bcc2e15b652d22069938df3a6/front_end/sdk/NetworkManager.js#L285-L297

added a comment

patrickhulce · 2018-06-13T00:40:08Z

Alright I think I'm comfortable taking this out of WIP, tests look good, Paul's initial feedback addressed, added some better coverage around the redirect cases

I'm still going to put this through the lantern wringer before being 100% cool to merge 👍

patrickhulce · 2018-06-13T23:08:20Z

so more fun discoveries as part of this, mixed-content has been broken for some time :) #5127 removed the URL gatherer, so the mixed content config tried to treat the url npm module as a gatherer and config craps out.

on the plus side it seems that chrome changes to interception also broke it (it seems to think example.com is not upgradeable?) so we didn't break anything very useful? 🤷‍♀️

brendankenny · 2018-06-13T23:20:17Z

lol we really needed an integration test on that one... Config was hopefully catching this and we just weren't seeing it?

brendankenny

Some initial comments. This seems pretty nice, and considering how long it's been since we've updated our frontend and it still (mostly) works, I'm not too concerned about maintaining our own version :)

brendankenny · 2018-06-14T00:42:30Z

third-party/devtools/ResourceType.js

@@ -0,0 +1,326 @@
+/*


should add a LICENSE file (maybe this one?) in devtools/ as well

brendankenny · 2018-06-14T01:03:51Z

lighthouse-core/lib/network-recorder.js

@@ -96,8 +93,11 @@ class NetworkRecorder extends EventEmitter {
   * @return {boolean}
   */
  static _isQUICAndFinished(record) {
-    const isQUIC = record._responseHeaders && record._responseHeaders
-        .some(header => header.name.toLowerCase() === 'alt-svc' && /quic/.test(header.value));
+    const isQUIC =


brendankenny · 2018-06-14T01:13:49Z

lighthouse-core/lib/network-recorder.js

-        data.timestamp, data.wallTime, data.initiator, data.redirectResponse,
-        data.type);
+    const originalRequest = this._findRealRequest(data.requestId);
+    if (originalRequest) {


a comment on what this case is would be 👍👍

brendankenny · 2018-06-14T01:14:08Z

lighthouse-core/lib/network-recorder.js

+      return;
+    }
+
+    const request = new NetworkRequest();


maybe move this case to the early return as it's so much simpler/shorter?

brendankenny · 2018-06-14T01:22:18Z

lighthouse-core/lib/network-recorder.js

@@ -270,18 +294,66 @@ class NetworkRecorder extends EventEmitter {
    }
  }

+  /**
+   * @param {string} requestId


comment on what a "real" request is?

For my own confusion, the IDs of a chain of redirected requests are all the same? Hence the redirectDestination chain traversal? And the assumption is that if there's more data for a request it can only be the last in the redirect chain because the rest are finished?

For my own confusion, the IDs of a chain of redirected requests are all the same?

Yup

And the assumption is that if there's more data for a request it can only be the last in the redirect chain because the rest are finished?

Yup, after another requestWillBeSent is issued signaling a redirect, that is the final message of the original request and the rest are about the redirected request.

added a comment basically saying this 👍

brendankenny · 2018-06-14T01:25:49Z

lighthouse-core/lib/network-recorder.js

+      recordsByURL.set(record.url, record);
+    }
+
+    // set the initiator and redirects array


is there a way to do this incrementally as (presumably) NetworkManager or whatever does? If the same thing is available in the page dependency graph I guess that's fine too, but at that point do we need NetworkRecords...

brendankenny · 2018-06-14T01:26:29Z

lighthouse-core/lib/web-inspector.js

@@ -100,67 +100,13 @@ module.exports = (function() {
    return this._moduleSettings[settingName];
  };

-  // Enum from chromium//src/third_party/WebKit/Source/core/loader/MixedContentChecker.h
-  global.NetworkAgent = {


🎉 🎈 🎉

brendankenny · 2018-06-14T01:27:05Z

lighthouse-core/lib/network-request.js

+    /** @type {number} */
+    this._responseReceivedTime = -1;
+
+    this.transferSize = 0;


if you just went with this._transferSize I feel like this PR would shrink by 30%, but presumable there was a reason :)

well transferSize is the only one we have to standardize in the middle of this PR to get everything to work thanks to the way that the old network requests worked, since we want to end up on transferSize in the end, makes more sense to just go straight to it :)

patrickhulce · 2018-06-18T16:38:36Z

@brendankenny would you be able to give some post-initial thoughts? :)

patrickhulce · 2018-06-19T02:04:12Z

alright I'm pretty confident in this at this point, I've run it against the lantern expectations, found 2 more subtle places where we weren't matching DT behavior (though arguably we could've left it as is), and the only 8 sites that changed were small ordering differences that should be eliminated by #5362

the actual network record objects that were created were identical across all critical properties on all 100 sites

brendankenny

ok, big feedback this time, but almost all clarification/style stuff, I think. Overall this looks great

brendankenny · 2018-06-19T17:45:30Z

lighthouse-core/test/lib/web-inspector-test.js

@@ -17,7 +17,6 @@ describe('Web Inspector lib', function() {
    assert.ok(WebInspector.FilmStripModel);
    assert.ok(WebInspector.TimelineProfileTree);
    assert.ok(WebInspector.TimelineAggregator);
-    assert.ok(WebInspector.NetworkManager);


lol this test

brendankenny · 2018-06-19T17:49:05Z

typings/web-inspector.d.ts

@@ -6,8 +6,8 @@

 declare global {
  module LH.WebInspector {
-    // TODO(bckenny): standardize on underscored internal API
-    // externs for chrome-devtools-frontend/front_end/sdk/NetworkRequest.js
+    // TODO(phulce): standardize on non-underscored property names


switch this to "TODO(phulce): migrate to using network-request.js"?

brendankenny · 2018-06-19T17:52:11Z

typings/web-inspector.d.ts

-      }
+      redirectSource?: {url: string;};
+      redirectDestination?: {url: string;};
+      redirects?: NetworkRequest[];


can you add a /** ... */ comment to these? e.g. request that redirected to this request, if it exists, request this request redirected to, in order redirect chain to this request (if those are correct)

though maybe these comments should live in network-request.js

done over there 👍

brendankenny · 2018-06-19T17:58:23Z

typings/web-inspector.d.ts

@@ -62,16 +65,10 @@ declare global {
    }

    export interface ResourceType {


drop this and import ResourceType.js for the type?

brendankenny · 2018-06-19T18:00:37Z

third-party/devtools/ResourceType.js

+};
+
+/**
+ * Keep these in sync with WebCore::InspectorPageAgent::resourceTypeJson


you could maybe type this with Record<LH.Crdp.Page.ResourceType, ResourceType> to make sure they stay in sync (at least with our types for the protocol)

hmm, but this has more keys than that. I would have expected them to match. The types in the protocol repo have more of them (I should update us to use that), but still missing SourceMapScript and SourceMapStyleSheet

FWIW that was a devtools comment I didn't add that :)

we only need those ones that match so I'm cool with that, and honestly this entire file can be deleted after we stop that WebInspector.resourceTypes === nonsense :D

brendankenny · 2018-06-19T18:33:50Z

lighthouse-core/lib/network-request.js

+  /**
+   * @return {NetworkRequest}
+   */
+  clone() {


used anywhere?

nah just testing, I'll remove

brendankenny · 2018-06-19T18:39:41Z

lighthouse-core/lib/network-request.js

+   * @param {LH.Crdp.Network.LoadingFinishedEvent} data
+   */
+  onLoadingFinished(data) {
+    if (this.finished) return;


when does this happen and is that bad or just expected?

it seems to be expected for a certain class of request failures (aborted redirected requests)

not sure how to best comment it...maybe?
// On cancelled requests, DevTools can send duplicate erroneous events, prefer the first one for best timing data

I think that's 👍

brendankenny · 2018-06-19T18:40:49Z

lighthouse-core/lib/network-request.js

+    this.endTime = data.timestamp;
+
+    this.failed = true;
+    this._resourceType = data.type && resourceTypes[data.type];


why updated here?

because DT does :) https://cs.chromium.org/chromium/src/third_party/blink/renderer/devtools/front_end/sdk/NetworkManager.js?type=cs&q=%22setResourceType%22+-f:out+lang:js&sq=package:chromium&g=0&l=600

I think it's because resourceTypes can always lie, so you can always get more information when the response comes back, also probably has to do with signed-exchanges and some corner cases we don't really care about

ah, yeah, good to mirror the real deal. I was also wondering because onLoadingFinished doesn't do the same

brendankenny · 2018-06-19T18:47:59Z

lighthouse-core/lib/network-request.js

+    // Take startTime and responseReceivedTime from timing data for better accuracy.
+    // Timing's requestTime is a baseline in seconds, rest of the numbers there are ticks in millis.
+    this.startTime = timing.requestTime;
+    const headersReceivedTime = timing.requestTime + timing.receiveHeadersEnd / 1000.0;


brendankenny · 2018-06-19T18:48:34Z

lighthouse-core/lib/network-request.js

+    this.endTime = Math.max(this.endTime, this._responseReceivedTime);
+  }
+
+  _updateResponseReceivedTimeIfNecessary() {


add a comment for what this is doing/when it's "necessary"?

brendankenny

some last comment suggestions, but otherwise LGTM! Let's do this

brendankenny · 2018-06-19T22:30:06Z

lighthouse-core/lib/network-recorder.js

+    const redirectRequest = new NetworkRequest();
+
+    redirectRequest.onRequestWillBeSent(modifiedData);
+    originalRequest.onRedirectResponse(data);


maybe add a comment so that anyone rearranging in the future won't accidentally do that?

brendankenny · 2018-06-19T22:30:55Z

lighthouse-core/lib/network-request.js

+    this._timing = /** @type {LH.Crdp.Network.ResourceTiming|undefined} */ (undefined);
+    this._resourceType = /** @type {LH.WebInspector.ResourceType|undefined} */ (undefined);
+    this._mimeType = '';
+    this.priority = () => /** @type {LH.Crdp.Network.ResourcePriority} */ ('Low');


yes, don't want to add machinery for things I immediately want to get rid of :)

ha, oh right. SG

brendankenny · 2018-06-19T22:32:44Z

lighthouse-core/lib/network-request.js

+   * @param {LH.Crdp.Network.LoadingFinishedEvent} data
+   */
+  onLoadingFinished(data) {
+    if (this.finished) return;


I think that's 👍

brendankenny · 2018-06-19T22:33:30Z

lighthouse-core/lib/network-request.js

+    this.endTime = data.timestamp;
+
+    this.failed = true;
+    this._resourceType = data.type && resourceTypes[data.type];


ah, yeah, good to mirror the real deal. I was also wondering because onLoadingFinished doesn't do the same

brendankenny · 2018-06-19T22:36:40Z

typings/web-inspector.d.ts

-    export interface ResourceCategory {
-      title: string;
-    }
+    export type ResourceType = _ResourceType;


it doesn't seem like anything is referring to this type except the NetworkRequest interface, so can drop this declaration?

patrickhulce added 3 commits June 7, 2018 17:01

remove network record dependency

3848575

fix byte-efficiency-audit

58e7c51

SUPER WIP

f35c56c

patrickhulce requested review from brendankenny and paulirish as code owners June 8, 2018 01:15

patrickhulce added 3 commits June 8, 2018 15:22

fix tests

ade9def

fix tests

05194ea

golden LHR diffs

dd11851

patrickhulce changed the title ~~[SUPER WIP] core: remove dependency on network records~~ [Slightly WIP] core: remove dependency on network records Jun 8, 2018

brendankenny reviewed Jun 11, 2018

View reviewed changes

patrickhulce changed the title ~~[Slightly WIP] core: remove dependency on network records~~ [Slightly WIP] core: remove dependency on devtools-frontend NetworkRequest Jun 11, 2018

patrickhulce commented Jun 11, 2018

View reviewed changes

patrickhulce added 2 commits June 11, 2018 10:29

remove commented code

5988b11

more tests

1c16232

paulirish reviewed Jun 12, 2018

View reviewed changes

patrickhulce added 3 commits June 12, 2018 17:13

more feedback

aecb428

dump more of WebInspector

432b989

fix resource category

0324bc0

patrickhulce changed the title ~~[Slightly WIP] core: remove dependency on devtools-frontend NetworkRequest~~ core: remove dependency on devtools-frontend NetworkRequest Jun 13, 2018

patrickhulce added 4 commits June 12, 2018 17:46

Merge branch 'master' into standardize_network_req_api

c5db54d

merge conflicts

5836b4c

cleanup ts-nocheck files

cee1f0e

fix mixed content

4212ab6

typo

7634cf8

brendankenny mentioned this pull request Jun 14, 2018

mixed-content fix and smoke tests #5498

Closed

2 tasks

brendankenny requested changes Jun 14, 2018

View reviewed changes

patrickhulce added 3 commits June 14, 2018 14:08

feedback

21fe6df

Merge branch 'master' into standardize_network_req_api

1f0ac70

remove unused portions of ResourceType

915449f

patrickhulce added 5 commits June 18, 2018 12:59

Merge branch 'master' into standardize_network_req_api

95551ee

match two subtle DT behaviors

92e8abf

more DT behavior matching

b831cbb

Merge branch 'master' into standardize_network_req_api

911a268

update lantern expectations

d11d2e7

brendankenny requested changes Jun 19, 2018

View reviewed changes

feedback

a2015af

brendankenny approved these changes Jun 19, 2018

View reviewed changes

more feedback

e5bea12

brendankenny mentioned this pull request Jun 20, 2018

core(tsc): use Config class to define Config type #5525

Merged

patrickhulce merged commit 3f8b498 into master Jun 20, 2018

patrickhulce deleted the standardize_network_req_api branch June 20, 2018 18:19

patrickhulce mentioned this pull request Jun 28, 2018

core: convert requestIds before sending to backend #5580

Merged

brendankenny mentioned this pull request Sep 26, 2019

core(network-recorder): remove quic-request-finished workaround #9744

Merged

brendankenny mentioned this pull request Apr 5, 2021

deps: update eslint to latest #12333

Merged

		@@ -62,16 +65,10 @@ declare global {
		}

		export interface ResourceType {

core: remove dependency on devtools-frontend NetworkRequest #5451

core: remove dependency on devtools-frontend NetworkRequest #5451

Conversation

patrickhulce commented Jun 8, 2018 • edited Loading

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

patrickhulce commented Jun 11, 2018

patrickhulce Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish 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

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

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 commented Jun 13, 2018

patrickhulce commented Jun 13, 2018

brendankenny commented Jun 13, 2018

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

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

Choose a reason for hiding this comment

patrickhulce commented Jun 18, 2018

patrickhulce commented Jun 19, 2018 • edited Loading

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

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 Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jun 8, 2018 •

edited

Loading

patrickhulce Jun 11, 2018 •

edited

Loading

patrickhulce commented Jun 19, 2018 •

edited

Loading

patrickhulce Jun 19, 2018 •

edited

Loading

brendankenny Jun 19, 2018 •

edited

Loading