| Summary: | Web Inspector: refactor ResourceView hierarchy. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Pavel Feldman <pfeldman> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, caseq, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
Created attachment 74550 [details]
[PATCH] Proposed change.
Comment on attachment 74550 [details] [PATCH] Proposed change. View in context: https://bugs.webkit.org/attachment.cgi?id=74550&action=review > WebCore/inspector/front-end/CookieItemsView.js:132 > if (!pushed) { > pushed = true; > - this._cookies.push(allCookies[i]); > + cookies.push(allCookies[i]); We're calculating size for all cookies, not just for given domain, is it the intent? If not, I guess bailing out with break is the way to go instead of using a flag. > WebCore/inspector/front-end/CookieItemsView.js:182 > if (addedCookies[cookies[i].name]) > - continue; > + continue; Wrong indent. > WebCore/inspector/front-end/CookiesTable.js:89 > + addCookiesFolder: function(folderName, cookies) > + { > + this._data.push({cookies: cookies, folderName: folderName}); > + this._rebuildTable(); So we're doing a full rebuild upon adding every folder -- is this the intent? > WebCore/inspector/front-end/CookiesTable.js:116 > + reset: function() Where is this used? > WebCore/inspector/front-end/CookiesTable.js:124 > + var selectedCookie = this._dataGrid.selectedNode ? this._dataGrid.selectedNode.cookie : null; You seem to have selectedCookie getter just for this. > WebCore/inspector/front-end/CookiesTable.js:181 > + case 0: comparator = localeCompare.bind(this, "name"); break; > + case 1: comparator = localeCompare.bind(this, "value"); break; > + case 2: comparator = localeCompare.bind(this, "domain"); break; > + case 3: comparator = localeCompare.bind(this, "path"); break; > + case 4: comparator = expiresCompare; break; > + case 5: comparator = numberCompare.bind(this, "size"); break; > + case 6: comparator = localeCompare.bind(this, "httpOnly"); break; > + case 7: comparator = localeCompare.bind(this, "secure"); break; > + default: localeCompare.bind(this, "name"); I know it's been there before, so just a sugegstion: DataGrid seems to have been designed to handle symbolic column identifiers, with an unfortunate exception of _editingCommitted(), which brutally abuses column class names and should probably be fixed anyway. Using original cookie field names as a column identifier will spare us for this switch, simplify code in _createGridNode() and make column info initialization in constructor way more readable. > WebCore/inspector/front-end/CookiesTable.js:198 > + data[6] = (cookie.httpOnly ? "\u2713" : ""); // Checkmark > + data[7] = (cookie.secure ? "\u2713" : ""); // Checkmark const checkmark = "\u2713"? > WebCore/inspector/front-end/FileSystemView.js:128 > - this._tabbedPane.selectTabById("persistent"); > + this._tabbedPane.selectTab("persistent"); Did you test FileSystemView? The changes you've made to TabbedPane will apparently break it, as it relies on tabs being Elements, not Views. > WebCore/inspector/front-end/NetworkItemView.js:48 > + if (this._contentView.hasContent()) { > + // Reusing this view, so hide it at first. > + this._contentView.visible = false; > + this._tabbedPane.appendTab("content", WebInspector.UIString("Content"), this._contentView); > + } > + this._tabbedPane.appendTab("cookies", WebInspector.UIString("Cookies"), this._cookiesView); So we're not showing content tab if there's no content, yet show cookies all the time -- seems a bit inconsistent. > WebCore/inspector/front-end/NetworkItemView.js:67 > + this._isInFallbackSelection = true; > + this._tabbedPane.selectTab("headers"); > + delete this._isInFallbackSelection; Appears to be a bit of hack. Can we rather pass additional parameter along with tab-selected event that signifies whether the selection was a result of user gesture? > WebCore/inspector/front-end/NetworkItemView.js:85 > + selectContentTab: function() Is this still being used? > WebCore/inspector/front-end/NetworkPanel.js:836 > + delete this.visibleView; Redundant, isn't it? > WebCore/inspector/front-end/ResourceCookiesView.js:50 > + this._emptyMsgElement = document.createElement("div"); > + this._emptyMsgElement.className = "storage-empty-view"; > + this._emptyMsgElement.textContent = WebInspector.UIString("This request has no cookies."); > + this.element.appendChild(this._emptyMsgElement); consider this.element.createChild("div", "storage-empty-view").textContent = WebInspector.UIString("This request has no cookies."); > WebCore/inspector/front-end/ResourceView.js:30 > WebInspector.ResourceView = function(resource) Rename to ResourceContentView? > WebCore/inspector/front-end/SourceView.js:209 > this.tabbedPane.selectTabById("local"); Does this work? > WebCore/inspector/front-end/StylesSidebarPane.js:117 > + return; Huh?? > WebCore/inspector/front-end/TabbedPane.js:53 > + selectTab: function(id, e) What's e? Seems to be unused. > WebCore/inspector/front-end/TabbedPane.js:60 > + delete this._currentTab; Seems to be redundant. > WebCore/inspector/front-end/networkPanel.css:719 > + white-space:pre-wrap; style Comment on attachment 74550 [details] [PATCH] Proposed change. View in context: https://bugs.webkit.org/attachment.cgi?id=74550&action=review > WebCore/inspector/front-end/CookiesTable.js:63 > + columns[0].width = "25%"; > + columns[1].title = WebInspector.UIString("Value"); > + columns[1].sortable = true; > + columns[1].width = "35%"; > + columns[2].title = WebInspector.UIString("Domain"); > + columns[2].sortable = true; > + columns[2].width = "7%"; > + columns[3].title = WebInspector.UIString("Path"); > + columns[3].sortable = true; > + columns[3].width = "8%"; > + columns[4].title = WebInspector.UIString("Expires"); > + columns[4].sortable = true; > + columns[4].width = "7%"; > + columns[5].title = WebInspector.UIString("Size"); > + columns[5].aligned = "right"; > + columns[5].sortable = true; > + columns[5].width = "8%"; > + columns[6].title = WebInspector.UIString("HTTP"); > + columns[6].aligned = "centered"; > + columns[6].sortable = true; > + columns[6].width = "7%"; > + columns[7].title = WebInspector.UIString("Secure"); > + columns[7].aligned = "centered"; > + columns[7].sortable = true; > + columns[7].width = "8%"; Percents sum up to 105, I guess that's the downside of doing this by hands. Did we loose any hope with auto-sizing? > We're calculating size for all cookies, not just for given domain, is it the intent? If not, I guess bailing out with break is the way to go instead of using a flag. > Nope, this counter is simply collecting the overhead (i.e. all cookies for all resources per this domain). > > WebCore/inspector/front-end/CookieItemsView.js:182 > > if (addedCookies[cookies[i].name]) > > - continue; > > + continue; > > Wrong indent. > Done. > > WebCore/inspector/front-end/CookiesTable.js:89 > > + addCookiesFolder: function(folderName, cookies) > > + { > > + this._data.push({cookies: cookies, folderName: folderName}); > > + this._rebuildTable(); > > So we're doing a full rebuild upon adding every folder -- is this the intent? > Yes, this is easy, should not happen often, preserves proper order. > > WebCore/inspector/front-end/CookiesTable.js:116 > > + reset: function() > > Where is this used? > Nowhere, nuked! > > WebCore/inspector/front-end/CookiesTable.js:124 > > + var selectedCookie = this._dataGrid.selectedNode ? this._dataGrid.selectedNode.cookie : null; > > You seem to have selectedCookie getter just for this. > Yep, added it after the usage. Using it now. > > WebCore/inspector/front-end/CookiesTable.js:181 > > + case 0: comparator = localeCompare.bind(this, "name"); break; > > + case 1: comparator = localeCompare.bind(this, "value"); break; > > + case 2: comparator = localeCompare.bind(this, "domain"); break; > > + case 3: comparator = localeCompare.bind(this, "path"); break; > > + case 4: comparator = expiresCompare; break; > > + case 5: comparator = numberCompare.bind(this, "size"); break; > > + case 6: comparator = localeCompare.bind(this, "httpOnly"); break; > > + case 7: comparator = localeCompare.bind(this, "secure"); break; > > + default: localeCompare.bind(this, "name"); > > I know it's been there before, so just a sugegstion: DataGrid seems to have been designed to handle symbolic column identifiers, with an unfortunate exception of _editingCommitted(), which brutally abuses column class names and should probably be fixed anyway. Using original cookie field names as a column identifier will spare us for this switch, simplify code in _createGridNode() and make column info initialization in constructor way more readable. > Some other time. > > WebCore/inspector/front-end/CookiesTable.js:198 > > + data[6] = (cookie.httpOnly ? "\u2713" : ""); // Checkmark > > + data[7] = (cookie.secure ? "\u2713" : ""); // Checkmark > > const checkmark = "\u2713"? > Done. > > WebCore/inspector/front-end/FileSystemView.js:128 > > - this._tabbedPane.selectTabById("persistent"); > > + this._tabbedPane.selectTab("persistent"); > > Did you test FileSystemView? The changes you've made to TabbedPane will apparently break it, as it relies on tabs being Elements, not Views. > I know. FSV is not working, hidden behind a flag, needs UI mock and review. > > WebCore/inspector/front-end/NetworkItemView.js:48 > > + if (this._contentView.hasContent()) { > > + // Reusing this view, so hide it at first. > > + this._contentView.visible = false; > > + this._tabbedPane.appendTab("content", WebInspector.UIString("Content"), this._contentView); > > + } > > + this._tabbedPane.appendTab("cookies", WebInspector.UIString("Cookies"), this._cookiesView); > > So we're not showing content tab if there's no content, yet show cookies all the time -- seems a bit inconsistent. > We maintain existing behavior - no content tab for resources we can't show content for (redirects, etc.) > > WebCore/inspector/front-end/NetworkItemView.js:67 > > + this._isInFallbackSelection = true; > > + this._tabbedPane.selectTab("headers"); > > + delete this._isInFallbackSelection; > > Appears to be a bit of hack. Can we rather pass additional parameter along with tab-selected event that signifies whether the selection was a result of user gesture? > Done. > > WebCore/inspector/front-end/NetworkItemView.js:85 > > + selectContentTab: function() > > Is this still being used? > Nope, nuked. > > WebCore/inspector/front-end/NetworkPanel.js:836 > > + delete this.visibleView; > > Redundant, isn't it? > It kinda shows the state control flow is in. > > WebCore/inspector/front-end/ResourceCookiesView.js:50 > > + this._emptyMsgElement = document.createElement("div"); > > + this._emptyMsgElement.className = "storage-empty-view"; > > + this._emptyMsgElement.textContent = WebInspector.UIString("This request has no cookies."); > > + this.element.appendChild(this._emptyMsgElement); > > consider this.element.createChild("div", "storage-empty-view").textContent = WebInspector.UIString("This request has no cookies."); > No way :) > > WebCore/inspector/front-end/ResourceView.js:30 > > WebInspector.ResourceView = function(resource) > > Rename to ResourceContentView? > It is only content view from the network panel perspective. > > WebCore/inspector/front-end/SourceView.js:209 > > this.tabbedPane.selectTabById("local"); > > Does this work? > Nuked as a whole. > > WebCore/inspector/front-end/StylesSidebarPane.js:117 > > + return; > > Huh?? > That as a temporary measure. > > WebCore/inspector/front-end/TabbedPane.js:53 > > + selectTab: function(id, e) > > What's e? Seems to be unused. > > > WebCore/inspector/front-end/TabbedPane.js:60 > > + delete this._currentTab; > > Seems to be redundant. > I like this style more. > > WebCore/inspector/front-end/networkPanel.css:719 > > + white-space:pre-wrap; > > style Created attachment 74674 [details]
[PATCH] Review comments addressed.
Comment on attachment 74674 [details] [PATCH] Review comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=74674&action=review > WebCore/WebCore.gypi:4435 > + 'inspector/front-end/CookiesTable.js', nit: wrong sorting order > WebCore/WebCore.gypi:4488 > + 'inspector/front-end/ResourceCookiesView.js', ditto Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/English.lproj/localizedStrings.js M WebCore/WebCore.gypi M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/inspector/front-end/CookieItemsView.js A WebCore/inspector/front-end/CookiesTable.js M WebCore/inspector/front-end/FileSystemView.js M WebCore/inspector/front-end/FontView.js M WebCore/inspector/front-end/ImageView.js A WebCore/inspector/front-end/NetworkItemView.js M WebCore/inspector/front-end/NetworkPanel.js A WebCore/inspector/front-end/ResourceCookiesView.js A WebCore/inspector/front-end/ResourceHeadersView.js M WebCore/inspector/front-end/ResourceView.js M WebCore/inspector/front-end/ResourcesPanel.js M WebCore/inspector/front-end/ScriptsPanel.js M WebCore/inspector/front-end/SourceView.js M WebCore/inspector/front-end/TabbedPane.js M WebCore/inspector/front-end/WebKit.qrc M WebCore/inspector/front-end/inspector.css M WebCore/inspector/front-end/inspector.html M WebCore/inspector/front-end/networkPanel.css Committed r72655 |
Things that I am planning to do: - Extract headers rendering from ResourceView into HTTPRequestView and HTTPResponseView; - Introduce NetworkItemView that would contain tabbed pane. Tabbed pane will contain: Request: HTTPRequestView <new, extracted from ResourceView> Response: HTTPResponseView <new, extracted from ResourceView> Content: ResourceView's ancestor (SourceView or ImageView or FontView, etc.) Cookies: ResourceCookiesView <new, extracted from ResourceView> Timeline: ResourceTimelineView <new, mimics Timeline popover in Network panel> Size: <will be added in the future> As a result - nasty headersVisible logic will go away - Network panel, ResourcePanel and Scripts panel will reuse SourceView itself, with no headers business. - It will be easy to add tabs to the network panel (and resources panel in the future)