Bug 49864 – Web Inspector: refactor ResourceView hierarchy.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

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)

[Pavel Feldman]

Created attachment 74550 [details]
[PATCH] Proposed change.

[Andrey Kosyakov]

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

[Andrey Kosyakov]

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?

[Pavel Feldman]

> 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

[Pavel Feldman]

Created attachment 74674 [details]
[PATCH] Review comments addressed.

[Yury Semikhatsky]

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

[Pavel Feldman]

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