Standalone Devtools Module for Timeline
Created attachment 150253 [details] Patch
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.
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.
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.
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.
Created attachment 151472 [details] Patch
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.
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.
(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.
Created attachment 151754 [details] Patch
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.
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.
Created attachment 151761 [details] Patch
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.
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
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
Created attachment 151934 [details] Patch
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.
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
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
Created attachment 152008 [details] Patch
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.
Created attachment 152011 [details] Patch
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.
Created attachment 152035 [details] Patch
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
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
Comment on attachment 152035 [details] Patch Clearing r? since this is likely to be a work in progress patch not intended for landing.