Summary: | Web Inspector: HAR pageref attributes are wrong and inconsistent with pages array | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2012-01-16 11:59:09 PST
Created attachment 123275 [details]
Patch
Created attachment 123306 [details]
Patch
Comment on attachment 123306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123306&action=review A bunch of small nits. > Source/WebCore/inspector/front-end/HAREntry.js:325 > + _convertPage: function(page) If you move this method to the Page class, you wouldn't need to expose all of the page's properties to public. > Source/WebCore/inspector/front-end/NetworkLog.js:56 > + pageForResource: function(resource) pageLoadForResource ? > Source/WebCore/inspector/front-end/NetworkLog.js:58 > + return resource.__page; Please use Map for this. > Source/WebCore/inspector/front-end/NetworkLog.js:76 > + resource.__page = this._currentPageLoad; ditto > Source/WebCore/inspector/front-end/NetworkLog.js:88 > + var frame = WebInspector.resourceTreeModel.frameForId(resource.frameId); this seems to be unused. > Source/WebCore/inspector/front-end/NetworkLog.js:89 > + resource.__page = this._currentPageLoad; ditto (In reply to comment #3) > (From update of attachment 123306 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123306&action=review > > A bunch of small nits. > > > Source/WebCore/inspector/front-end/HAREntry.js:325 > > + _convertPage: function(page) > > If you move this method to the Page class, you wouldn't need to expose all of the page's properties to public. As discussed offline, this is not quite practical, as convertPage() relies on certain methods internal to HAR. Besides, we pretend this is a more generic, model level than HAR support -- I think this may be eventually reused in NetworkPanel, perhaps in audits. As agreed, I'm just removing getters and use fields directly. > > > Source/WebCore/inspector/front-end/NetworkLog.js:56 > > + pageForResource: function(resource) > > pageLoadForResource ? done. > > > Source/WebCore/inspector/front-end/NetworkLog.js:58 > > + return resource.__page; > > Please use Map for this. That would be cumbersome due to need to eventually release map entries. For HAR at least, the resource entries are primary and we only need PageLoad entries as long as there are some (network) Resources that refer to them. The life-time of resource is a bit complicated, as some may be cleaned manually in the network panel, some may survive page reloads (again due to network panel), and life-time of resource references in NetworkLog is different from the Network panel. WebInspector.resourceTreeModel.frameForId(resource.frameId); > > this seems to be unused. Thanks, fixed! Created attachment 123319 [details]
Patch
Committed r105596: <http://trac.webkit.org/changeset/105596> |