Bug 62006 - Web Inspector: move Network log view to an iframe
Summary: Web Inspector: move Network log view to an iframe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-03 04:53 PDT by Andrey Kosyakov
Modified: 2011-07-20 07:04 PDT (History)
10 users (show)

See Also:


Attachments
patch (51.88 KB, patch)
2011-06-03 05:36 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (72.08 KB, patch)
2011-06-06 12:29 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (73.11 KB, patch)
2011-07-15 05:20 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (73.61 KB, patch)
2011-07-18 04:45 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (80.57 KB, patch)
2011-07-18 07:53 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2011-06-03 04:53:40 PDT
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.
Comment 1 Pavel Feldman 2011-06-03 05:18:02 PDT
(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?
Comment 2 Andrey Kosyakov 2011-06-03 05:25:56 PDT
(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.
Comment 3 Pavel Feldman 2011-06-03 05:32:58 PDT
> 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.
Comment 4 Andrey Kosyakov 2011-06-03 05:36:58 PDT
Created attachment 95897 [details]
patch
Comment 5 Andrey Kosyakov 2011-06-03 05:38:20 PDT
(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.
Comment 6 Pavel Feldman 2011-06-03 05:53:26 PDT
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.
Comment 7 Pavel Feldman 2011-06-03 05:54:53 PDT
One thing I noticed after applying the patch: inspector thinks it has no focus when network tab is selected.
Comment 8 Pavel Feldman 2011-06-03 06:00:00 PDT
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.
Comment 9 Andrey Kosyakov 2011-06-06 12:29:35 PDT
Created attachment 96109 [details]
patch

- Moved iframe logic into view, inherit IFramePanel from IFrameView
- Added delayed creation of IFrameView
- Fixed some bugs
Comment 10 Andrey Kosyakov 2011-06-06 12:35:52 PDT
(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?
Comment 11 Andrey Kosyakov 2011-06-06 12:36:14 PDT
(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.
Comment 12 Andrey Kosyakov 2011-06-06 12:39:21 PDT
(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).
Comment 13 Pavel Feldman 2011-06-15 04:53:26 PDT
Comment on attachment 96109 [details]
patch

As we agreed offline, this is not ready for review.
Comment 14 Andrey Kosyakov 2011-07-15 05:20:22 PDT
Created attachment 100958 [details]
Patch
Comment 15 Andrey Kosyakov 2011-07-18 04:45:22 PDT
Created attachment 101145 [details]
patch

- fixed handling of keyboard events
Comment 16 Andrey Kosyakov 2011-07-18 07:53:48 PDT
Created attachment 101158 [details]
patch

- fixed search
- NetworkLogView.EventNames -> EventTypes
Comment 17 Pavel Feldman 2011-07-19 04:22:10 PDT
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?
Comment 18 Andrey Kosyakov 2011-07-20 07:04:36 PDT
Manually committed r91359: http://trac.webkit.org/changeset/91359