Bug 90307 – Web Inspector: Standalone Devtools Module for Timeline

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

[Gabriel Peal]

Standalone Devtools Module for Timeline

[Gabriel Peal]

Created attachment 150253 [details]
Patch

[WebKit Review Bot]

Attachment 150253 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabriel Peal]

The timeline panel has been pulled out of devtools so it can be embedded in an iframe on any site (only tested in Chrome so far) and prepopulated with an existing timeline recording.

To use it, create with an iframe and set the source to WebKit/Source/WebCore/inspector/front-end/timeline.html

Once the timeline is loaded, it will prepopulate itself with the recording stored at WebKit/Source/WebCore/inspector/front-end/Logs/timeline_data


NOTE: two slight modifications were made in the WebKit source that were required to make this work that shouldn't be rolled into the WebKit trunk so advice for the best way to incorporate it better would be much appreciated.

NOTE: there are not tests yet because the overall concept should evaluated and better integrated into the WebKit trunk first.

[Pavel Feldman]

Comment on attachment 150253 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +

Please explain what and why you are doing in this change. I will do the sanity review, but it will always be r- unless description here makes sense.

> Source/WebCore/inspector/front-end/ResourceUtils.js:317
> +    var anchor = WebInspector.linkifyURLAsNode(url, linkText, classes, true, tooltipText);

This regresses inspector functionality. Why isExternal is only used when inspector intercepts the <a> clicks. Do you intend to intercept it in your embedder as well?

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:6
> +    this.timelineManager = new WebInspector.TimelineManager();

It sounds like too much context for the timeline. We should use some dependency injection scheme to workaround it better.

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:16
> +    this.timelinePanel._isShowing = true;

You should not use private members of other classes.

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:22
> +WebInspector.TimelineEmbedded = function() {}

Space between { }, Please annotate this as the constructor.

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:33
> +        //WekKit doesn't want you to embed inspector elements in a page so they identify elements that came from the inspector because they have

We(b)Kit

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:35
> +        var __view = timelineDiv.__view;

Do not access private members.

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:54
> +        if(arguments.length == 1) {

No need for { }

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:61
> +        var data = JSON.parse(xhr.responseText);

poor indentation.

> Source/WebCore/inspector/front-end/TimelineEmbedded.js:88
> +window.addEventListener("DOMContentLoaded", pageLoaded, false);

I am not sure this class should be a part of the WebKit. It is not included in the build and is not tested, hence it is likely to turn into broken code eventually.

> Source/WebCore/inspector/front-end/TimelinePanel.js:199
> +        var statusBarItems = [ this.toggleFilterButton.element, this._glueParentButton.element, this.statusBarFilters ];

Why do you think clear button does not make sense? I think you could extract the main part of the timeline into a view. You will re-use the view, but not the panel. Panel will return composition of its own status bar items and the view's ones.

> Source/WebCore/inspector/front-end/inspector.css:438
> +#main-no-status-bar {

This should be a part of the embedder's styles.

> Source/WebCore/inspector/front-end/inspector.js:400
>      DebuggerAgent.supportsSeparateScriptCompilationAndExecution(WebInspector._initializeCapability.bind(WebInspector, "separateScriptCompilationAndExecutionEnabled", null));

Because you are using the newer front-end with the older backend (ToT front-end against Dev channel?)

> Source/WebCore/inspector/front-end/inspector.js:-826
> -    if (WebInspector._toggleConsoleButton && !WebInspector._toggleConsoleButton.toggled) {

Please do not change inspector's code.

> Source/WebCore/inspector/front-end/timeline.html:28
> +<!DOCTYPE html>

This file should not be a part of WebKit.

> Source/WebCore/inspector/front-end/timeline.html:232
> +        <script type="text/javascript" src="TimelineEmbedded.js"></script>

You don't need most of this stuff. Take a look at compile-front-end.py for the list of dependencies to take.

[Gabriel Peal]

Thanks for the sanity check. I'll make the necessary changes this week.

The goal of this change was to improve the functionality of webpagetest.org by using an embedded devtools timeline instead of the currently used waterfall chart. The purpose of this bug is to determine
a) whether there are any glaring problems with this way of implementing an embeddable timeline
b) if there is any use for such a feature in WebKit
c) if so, where it should go within the source

(In reply to comment #4)
> (From update of attachment 150253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150253&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +
> 
> Please explain what and why you are doing in this change. I will do the sanity review, but it will always be r- unless description here makes sense.
> 
> > Source/WebCore/inspector/front-end/ResourceUtils.js:317
> > +    var anchor = WebInspector.linkifyURLAsNode(url, linkText, classes, true, tooltipText);
> 
> This regresses inspector functionality. Why isExternal is only used when inspector intercepts the <a> clicks. Do you intend to intercept it in your embedder as well?
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:6
> > +    this.timelineManager = new WebInspector.TimelineManager();
> 
> It sounds like too much context for the timeline. We should use some dependency injection scheme to workaround it better.
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:16
> > +    this.timelinePanel._isShowing = true;
> 
> You should not use private members of other classes.
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:22
> > +WebInspector.TimelineEmbedded = function() {}
> 
> Space between { }, Please annotate this as the constructor.
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:33
> > +        //WekKit doesn't want you to embed inspector elements in a page so they identify elements that came from the inspector because they have
> 
> We(b)Kit
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:35
> > +        var __view = timelineDiv.__view;
> 
> Do not access private members.
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:54
> > +        if(arguments.length == 1) {
> 
> No need for { }
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:61
> > +        var data = JSON.parse(xhr.responseText);
> 
> poor indentation.
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:88
> > +window.addEventListener("DOMContentLoaded", pageLoaded, false);
> 
> I am not sure this class should be a part of the WebKit. It is not included in the build and is not tested, hence it is likely to turn into broken code eventually.
> 
> > Source/WebCore/inspector/front-end/TimelinePanel.js:199
> > +        var statusBarItems = [ this.toggleFilterButton.element, this._glueParentButton.element, this.statusBarFilters ];
> 
> Why do you think clear button does not make sense? I think you could extract the main part of the timeline into a view. You will re-use the view, but not the panel. Panel will return composition of its own status bar items and the view's ones.
> 
> > Source/WebCore/inspector/front-end/inspector.css:438
> > +#main-no-status-bar {
> 
> This should be a part of the embedder's styles.
> 
> > Source/WebCore/inspector/front-end/inspector.js:400
> >      DebuggerAgent.supportsSeparateScriptCompilationAndExecution(WebInspector._initializeCapability.bind(WebInspector, "separateScriptCompilationAndExecutionEnabled", null));
> 
> Because you are using the newer front-end with the older backend (ToT front-end against Dev channel?)
> 
> > Source/WebCore/inspector/front-end/inspector.js:-826
> > -    if (WebInspector._toggleConsoleButton && !WebInspector._toggleConsoleButton.toggled) {
> 
> Please do not change inspector's code.
> 
> > Source/WebCore/inspector/front-end/timeline.html:28
> > +<!DOCTYPE html>
> 
> This file should not be a part of WebKit.
> 
> > Source/WebCore/inspector/front-end/timeline.html:232
> > +        <script type="text/javascript" src="TimelineEmbedded.js"></script>
> 
> You don't need most of this stuff. Take a look at compile-front-end.py for the list of dependencies to take.

[Gabriel Peal]

Created attachment 151472 [details]
Patch

[WebKit Review Bot]

Attachment 151472 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabriel Peal]

Updated with unobstrusive overrides.

Note: this the overrides provide examples of what would need dependency injection but were implemented unobtrusively until a plan to roll it into WebKit is made.

[Michael Klepikov]

(In reply to comment #4)
> (From update of attachment 150253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150253&action=review
> 
> > Source/WebCore/inspector/front-end/TimelineEmbedded.js:6
> > +    this.timelineManager = new WebInspector.TimelineManager();
> 
> It sounds like too much context for the timeline. We should use some dependency injection scheme to workaround it better.

I think this is the crux of the argument, also per our email conversation: this patch should become a refactoring that makes the Timeline more modular, with more explicit dependencies. Gabriel has uploaded the next version of the patch, where he no longer modifies existing files, but replaces some of the dependent classes with his own inherited versions. The main purpose of that new patch version is to *identify* these Timeline dependencies and give an idea of what it would take to reduce the list below.

We don't know how the DevTools team would prefer to deal with injection, and we would like some guidance. Right now WebInspector is kind of a god object -- it knows about all the other objects in the system, and it makes them public, and the other classes access these objects through the WebInspector's public variables, not via their own members, making the dependencies implicit. I would think that ideally we may want to get away from this setup (please correct me if I'm wrong), but it would be too much of a sweeping change to do that in one shot. We need to go in smaller steps, and I'd like to get on the same page how exactly you would prefer us to do that.

We could for example, as Gabriel proposed, introduce constructors with arguments -- explicit dependency injection, and fall back to the current behavior if the args are undefined -- allowing the current system to continue to work while we begin to use injection in some scenarios.

Another possibility is a poor man's GUICE -- basically a map of class -> object, which would either be DevTools-global or belong to the WebInspector (which is more or less the same thing). The other objects, instead of "new", would ask the WebInspector to give them an instance of that class. The startup code would set up the map.

[Gabriel Peal]

Created attachment 151754 [details]
Patch

[WebKit Review Bot]

Attachment 151754 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabriel Peal]

Patch update #3:

The latest patch includes a prototype implementation of dependency injection in ResourceTreeModel and TimelinePanel.

This implementation of dependency injection should not introduce any incompatibilities with existing code yet exposes external dependencies explicitly.

[Gabriel Peal]

Created attachment 151761 [details]
Patch

[WebKit Review Bot]

Attachment 151761 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 151761 [details]
Patch

Attachment 151761 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13202589

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
inspector/console/console-log-toString-object.html
fast/loader/unload-form-post-about-blank.html
inspector/extensions/extensions-reload.html
inspector/console/console-uncaught-exception-in-eval.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/resource-tree/resource-tree-errors-reload.html
inspector/audits/audits-panel-functional.html
http/tests/inspector/network/network-initiator-from-console.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
inspector/extensions/extensions-network.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/indexeddb/resources-panel.html
fast/forms/range/slider-mouse-events.html
fast/frames/cached-frame-counter.html
http/tests/inspector/network/network-disable-cache-memory.html
inspector/console/alert-toString-exception.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/resource-tree/resource-tree-reload.html
inspector/console/console-preserve-log.html
inspector/debugger/debugger-breakpoints-not-activated-on-reload.html
fast/forms/range/slider-onchange-event.html
fast/loader/local-CSS-from-local.html
inspector/audits/audits-panel-noimages-functional.html

[WebKit Review Bot]

Created attachment 151869 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Gabriel Peal]

Created attachment 151934 [details]
Patch

[WebKit Review Bot]

Attachment 151934 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 151934 [details]
Patch

Attachment 151934 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13180861

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
inspector/console/console-log-toString-object.html
fast/loader/unload-form-post-about-blank.html
inspector/console/console-uncaught-exception-in-eval.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/resource-tree/resource-tree-errors-reload.html
inspector/audits/audits-panel-functional.html
inspector/elements/elements-panel-selection-on-refresh.html
http/tests/inspector/network/network-initiator-from-console.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/indexeddb/resources-panel.html
fast/forms/range/slider-mouse-events.html
fast/frames/cached-frame-counter.html
http/tests/inspector/network/network-disable-cache-memory.html
inspector/console/alert-toString-exception.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/extensions-network-redirect.html
inspector/elements/elements-panel-reload-assert.html
http/tests/inspector/resource-tree/resource-tree-reload.html
inspector/console/console-preserve-log.html
inspector/debugger/debugger-breakpoints-not-activated-on-reload.html
fast/forms/range/slider-onchange-event.html
fast/loader/local-CSS-from-local.html
inspector/audits/audits-panel-noimages-functional.html

[WebKit Review Bot]

Created attachment 151970 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Gabriel Peal]

Created attachment 152008 [details]
Patch

[WebKit Review Bot]

Attachment 152008 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabriel Peal]

Created attachment 152011 [details]
Patch

[WebKit Review Bot]

Attachment 152011 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabriel Peal]

Created attachment 152035 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152035 [details]
Patch

Attachment 152035 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13236101

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/extensions-headers.html
http/tests/inspector-enabled/injected-script-discard.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/web-socket-frame-error.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html

[WebKit Review Bot]

Created attachment 152225 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pavel Feldman]

Comment on attachment 152035 [details]
Patch

Clearing r? since this is likely to be a work in progress patch not intended for landing.