[webkit-reviews] review denied: [Bug 90307] Standalone Devtools Module for Timeline : [Attachment 150253] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 29 21:35:23 PDT 2012
Pavel Feldman <pfeldman at chromium.org> has denied Gabriel Peal
<gpeal at google.com>'s request for review:
Bug 90307: Standalone Devtools Module for Timeline
https://bugs.webkit.org/show_bug.cgi?id=90307
Attachment 150253: Patch
https://bugs.webkit.org/attachment.cgi?id=150253&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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._initi
alizeCapability.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.
More information about the webkit-reviews
mailing list