Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2712043002:
[DevTools] Separate frames and resources handling (Closed)
|
Created:
3 years, 10 months ago by eostroukhov Modified:
3 years, 9 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Separate frames and resources handling
BUG=698027
Review-Url: https://codereview.chromium.org/2712043002
Cr-Commit-Position: refs/heads/master@{#454956}
Committed: https://chromium.googlesource.com/chromium/src/+/517b840ba9cf91c4a33bd3c0552b74bf4b2a64e9
Patch Set 1 #Patch Set 2 : Reduced reliance on a single target. #Patch Set 3 : Added missing null check #
Total comments: 12
Patch Set 4 : [DevTools] Separate frames and resources handling #
Total comments: 4
Patch Set 5 : [DevTools] Separate frames and resources handling #
Messages
Total messages: 36 (23 generated)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org
Please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eostroukhov@chromium.org changed reviewers: - caseq@chromium.org
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
I updated this change with a better support for OOPIF. Please take a look.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I believe I didn't understand what this patch does. It needs a bug and a better description. And a screenshot. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js (right): https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:28: this.populateFrame(mainFrame); Why populate with main frame? https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:123: Resources.FrameTreeElement = class extends Resources.BaseStorageTreeElement { Didn't we agree on the frame picker? Why is there a frame tree element?
https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js (right): https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:28: this.populateFrame(mainFrame); On 2017/02/25 00:54:42, dgozman wrote: > Why populate with main frame? It is the root node. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:123: Resources.FrameTreeElement = class extends Resources.BaseStorageTreeElement { On 2017/02/25 00:54:42, dgozman wrote: > Didn't we agree on the frame picker? Why is there a frame tree element? This is a refactoring change, functionality remains. I am implementing the picker on top of this change. I don't feel comfortable submitting a single monolytic CL...
On 2017/02/25 01:00:23, eostroukhov wrote: > https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js > (right): > > https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:28: > this.populateFrame(mainFrame); > On 2017/02/25 00:54:42, dgozman wrote: > > Why populate with main frame? > > It is the root node. > > https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:123: > Resources.FrameTreeElement = class extends Resources.BaseStorageTreeElement { > On 2017/02/25 00:54:42, dgozman wrote: > > Didn't we agree on the frame picker? Why is there a frame tree element? > > This is a refactoring change, functionality remains. I am implementing the > picker on top of this change. I don't feel comfortable submitting a single > monolytic CL... I see. Let's not include better OOPIF support then? Also, please attribute to a bug and add a better description.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [DevTools] Separate frames and resources handling BUG= ========== to ========== [DevTools] Separate frames and resources handling BUG=698027 ==========
On 2017/02/25 01:07:06, dgozman wrote: > On 2017/02/25 01:00:23, eostroukhov wrote: > > > https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... > > File > third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js > > (right): > > > > > https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:28: > > this.populateFrame(mainFrame); > > On 2017/02/25 00:54:42, dgozman wrote: > > > Why populate with main frame? > > > > It is the root node. > > > > > https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:123: > > Resources.FrameTreeElement = class extends Resources.BaseStorageTreeElement { > > On 2017/02/25 00:54:42, dgozman wrote: > > > Didn't we agree on the frame picker? Why is there a frame tree element? > > > > This is a refactoring change, functionality remains. I am implementing the > > picker on top of this change. I don't feel comfortable submitting a single > > monolytic CL... > > I see. Let's not include better OOPIF support then? Also, please attribute to a > bug and add a better description. Walking starting from the top frame had not changed - I simply moved the code from the ResourcesPanel.js. What had changed was: 1. Instead of listening on the ResourceTreeModel directly, the code now listens to events from all targets through the targetManager. This change was made to reduce the coupling between the panel and the section - e.g. panel no longer needs to explicitly set ResourceTreeModel. 2. Added a code to link frame with empty parent with a top frame from the parent target. All other changes were about extracting a code to this smaller file. Future change will split it into the ResourcesSection and the frame dropdown.
https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js (right): https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:19: addListener(SDK.ResourceTreeModel.Events.FrameAdded, this.frameAdded, this); let's keep listeners private. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:40: while (parentTarget && !parentTarget.hasDOMCapability()) Does it ever happen? https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:55: console.warn('No frame to route ' + frame.url + ' to.'); nit: `No frame to route ${frame.url} to.` https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:123: Resources.FrameTreeElement = class extends Resources.BaseStorageTreeElement { Mind re-uploading with --similarity=<something-small> so it's easier to see it's old code.
Thank you for the review. I updated the CL, please take another look. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js (right): https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:19: addListener(SDK.ResourceTreeModel.Events.FrameAdded, this.frameAdded, this); On 2017/03/03 21:59:54, caseq wrote: > let's keep listeners private. Done. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:40: while (parentTarget && !parentTarget.hasDOMCapability()) On 2017/03/03 21:59:54, caseq wrote: > Does it ever happen? Changed to console.assert, as per offline discussion. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:55: console.warn('No frame to route ' + frame.url + ' to.'); On 2017/03/03 21:59:54, caseq wrote: > nit: `No frame to route ${frame.url} to.` Done. https://codereview.chromium.org/2712043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:123: Resources.FrameTreeElement = class extends Resources.BaseStorageTreeElement { On 2017/03/03 21:59:55, caseq wrote: > Mind re-uploading with --similarity=<something-small> so it's easier to see it's > old code. I tried that.
lgtm https://codereview.chromium.org/2712043002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js (right): https://codereview.chromium.org/2712043002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:112: populateFrame(frame) { _populateFrame(frame)? https://codereview.chromium.org/2712043002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:277: static resourceViewForResource(resource) { could remain private as well.
Thank you for the review. https://codereview.chromium.org/2712043002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js (right): https://codereview.chromium.org/2712043002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:112: populateFrame(frame) { On 2017/03/04 01:46:06, caseq wrote: > _populateFrame(frame)? Done. https://codereview.chromium.org/2712043002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesSection.js:277: static resourceViewForResource(resource) { On 2017/03/04 01:46:06, caseq wrote: > could remain private as well. It is referenced from ResourcesPanel.js.
The CQ bit was checked by eostroukhov@chromium.org
The CQ bit was unchecked by eostroukhov@chromium.org
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by eostroukhov@chromium.org
The CQ bit was checked by eostroukhov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2712043002/#ps80001 (title: "[DevTools] Separate frames and resources handling")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488820269027570,
"parent_rev": "6a8d425faa0c5cec9e5648d8297329d65764247e", "commit_rev":
"517b840ba9cf91c4a33bd3c0552b74bf4b2a64e9"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Separate frames and resources handling BUG=698027 ========== to ========== [DevTools] Separate frames and resources handling BUG=698027 Review-Url: https://codereview.chromium.org/2712043002 Cr-Commit-Position: refs/heads/master@{#454956} Committed: https://chromium.googlesource.com/chromium/src/+/517b840ba9cf91c4a33bd3c0552b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/517b840ba9cf91c4a33bd3c0552b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
