[Webkit-unassigned] [Bug 90307] Standalone Devtools Module for Timeline

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90307





--- Comment #5 from Gabriel Peal <gpeal at google.com>  2012-07-01 16:42:14 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.