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

Fri Jun 29 21:35:23 PDT 2012

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

Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #150253|review?                     |review-
               Flag|                            |

--- Comment #4 from Pavel Feldman <pfeldman at chromium.org>  2012-06-29 21:35:22 PST ---
(From update of attachment 150253)
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.