Bug 62006 – Web Inspector: move Network log view to an iframe

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

[Andrey Kosyakov]

Network panel uses large amount of CSS that is mostly independent on the rest of the inspector. Moving a panel into iframe allows to decrease both number of HTML elements and number of CSS rules in a frame, thus largely reducing CSS rule matching time.
On a primitive test (inspector on techcrunch.com), median time NetworkPanel.show() takes was reduced from 71ms to 9ms, first-time NetworkPanel show time went down from 1.6s to 1s, and for Scripts panel (untouched), median show time went from 88ms to 15ms.

[Pavel Feldman]

(In reply to comment #0)
> Network panel uses large amount of CSS that is mostly independent on the rest of the inspector. Moving a panel into iframe allows to decrease both number of HTML elements and number of CSS rules in a frame, thus largely reducing CSS rule matching time.
> On a primitive test (inspector on techcrunch.com), median time NetworkPanel.show() takes was reduced from 71ms to 9ms, first-time NetworkPanel show time went down from 1.6s to 1s, and for Scripts panel (untouched), median show time went from 88ms to 15ms.

1.6s -> 1s for first show still sounds very slow. Do you know why?

[Andrey Kosyakov]

(In reply to comment #1)
> (In reply to comment #0)
> > Network panel uses large amount of CSS that is mostly independent on the rest of the inspector. Moving a panel into iframe allows to decrease both number of HTML elements and number of CSS rules in a frame, thus largely reducing CSS rule matching time.
> > On a primitive test (inspector on techcrunch.com), median time NetworkPanel.show() takes was reduced from 71ms to 9ms, first-time NetworkPanel show time went down from 1.6s to 1s, and for Scripts panel (untouched), median show time went from 88ms to 15ms.
> 
> 1.6s -> 1s for first show still sounds very slow. Do you know why?

1s is for the first show of techcrunch's network log, empty case is much faster. I did not dig it yet, but my guess is that it's just populating timeline with network items.

[Pavel Feldman]

> 1s is for the first show of techcrunch's network log, empty case is much faster.

I think it is reasonable to require a 100ms feedback on this one, even if it ends up with deferred load. Btw, content of offscreen rows in the network panel is not rendered, only bounding boxes are. So it should not depend on the amount of resources much.

[Andrey Kosyakov]

Created attachment 95897 [details]
patch

[Andrey Kosyakov]

(In reply to comment #3)
> > 1s is for the first show of techcrunch's network log, empty case is much faster.
> 
> I think it is reasonable to require a 100ms feedback on this one, even if it ends up with deferred load. Btw, content of offscreen rows in the network panel is not rendered, only bounding boxes are. So it should not depend on the amount of resources much.

I'll have a look at this, but let's treat it as an independent issue.

[Pavel Feldman]

Comment on attachment 95897 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95897&action=review

Overall looks good, couple of comments inline.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:67676
> +					RelativePath="..\inspector\front-end\inspectorCommon.js"

inspectorCommon.css?

> Source/WebCore/inspector/front-end/NetworkPanel.js:42
> +        "textViewer.css",

So we multiply the styles for editor. Is there a vision that converts views to iframes.

> Source/WebCore/inspector/front-end/NetworkPanel.js:46
> +    WebInspector.IFramePanel.call(this, "network", styles);

Instead of a generic "iframe per panel" approach, I'd rather introduce "iframe-based views". The real thing we need here is a component model with the fine-grained APIs. Having said that, entire network panel can be a view.

> Source/WebCore/inspector/front-end/Panel.js:493
> +        this._iframeElement.contentDocument.head.appendChild(style);

Are you fighting for the sync load here? It does not look pretty.

> Source/WebCore/inspector/front-end/Panel.js:505
> +        this._iframeElement = document.createElement("iframe");

I guess this should be done lazily in order not to regress the inspector open time.

> Source/WebCore/inspector/front-end/Panel.js:522
> +        var boundDispatchEvent = this._iframeElement.dispatchEvent.bind(this._iframeElement);

Sometimes drag'n'drop events suffer from such dispatching. Please check whether it works for you.

[Pavel Feldman]

One thing I noticed after applying the patch: inspector thinks it has no focus when network tab is selected.

[Pavel Feldman]

Comment on attachment 95897 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95897&action=review

> Source/WebCore/inspector/front-end/NetworkPanel.js:43
> +        "inspectorSyntaxHighlight.css",

These guys are concatenated in the release mode. r- for this.

[Andrey Kosyakov]

Created attachment 96109 [details]
patch

- Moved iframe logic into view, inherit IFramePanel from IFrameView
- Added delayed creation of IFrameView
- Fixed some bugs

[Andrey Kosyakov]

(In reply to comment #6)
> (From update of attachment 95897 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?
> > > inspectorCommon.css?

Yep, thanks!

> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:42
> > +        "textViewer.css",
> 
> So we multiply the styles for editor. Is there a vision that converts views to iframes.
> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:46
> > +    WebInspector.IFramePanel.call(this, "network", styles);
> 
> Instead of a generic "iframe per panel" approach, I'd rather introduce "iframe-based views". The real thing we need here is a component model with the fine-grained APIs. Having said that, entire network panel can be a view.

Added IFrameView. NetworkPanel still inherits is an IFramePanel, it looks like we borrow quite a bit from panels, and converting panels into views may be a bit messy.

> > Source/WebCore/inspector/front-end/Panel.js:493
> > +        this._iframeElement.contentDocument.head.appendChild(style);
> 
> Are you fighting for the sync load here? It does not look pretty.

Yes -- otherwise, we attempt to do things based on layout (e.g. table column sizing) before styles are loaded.

> > Source/WebCore/inspector/front-end/Panel.js:505
> > +        this._iframeElement = document.createElement("iframe");
> 
> I guess this should be done lazily in order not to regress the inspector open time.

Done.

> 
> > Source/WebCore/inspector/front-end/Panel.js:522
> > +        var boundDispatchEvent = this._iframeElement.dispatchEvent.bind(this._iframeElement);
> 
> Sometimes drag'n'drop events suffer from such dispatching. Please check whether it works for you.

column resizing works, is there anything else to check?

[Andrey Kosyakov]

(In reply to comment #7)
> One thing I noticed after applying the patch: inspector thinks it has no focus when network tab is selected.

Fixed.

[Andrey Kosyakov]

(In reply to comment #8)
> (From update of attachment 95897 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95897&action=review
> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:43
> > +        "inspectorSyntaxHighlight.css",
> 
> These guys are concatenated in the release mode. r- for this.

It was actually working fine -- we now also have standalone copies of the relevant CSS files in the resources. As discussed, I'm leaving this mostly as is, though now I concatenate files in run-time before appending a style element. On my tests, we spend 2ms on XHR and 5ms on inserting the style into document (cumulative for all stylesheets used in the panel).

[Pavel Feldman]

Comment on attachment 96109 [details]
patch

As we agreed offline, this is not ready for review.

[Andrey Kosyakov]

Created attachment 100958 [details]
Patch

[Andrey Kosyakov]

Created attachment 101145 [details]
patch

- fixed handling of keyboard events

[Andrey Kosyakov]

Created attachment 101158 [details]
patch

- fixed search
- NetworkLogView.EventNames -> EventTypes

[Pavel Feldman]

Comment on attachment 101158 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101158&action=review

> Source/WebCore/inspector/front-end/View.js:87
> +WebInspector.IFrameView = function(parent, stylesheets)

Could you extract this into a separate file?

> Source/WebCore/inspector/front-end/inspector.css:-2193
> -.data-grid {

Could you move data grid into data grid instead? Also, could you concatenate these files?

[Andrey Kosyakov]

Manually committed r91359: http://trac.webkit.org/changeset/91359