[webkit-reviews] review denied: [Bug 90871] Web Inspector: Display Named Flows in the "CSS Named Flows" drawer : [Attachment 163805] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 13 02:47:15 PDT 2012
Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Andrei Poenaru
<poenaru at adobe.com>'s request for review:
Bug 90871: Web Inspector: Display Named Flows in the "CSS Named Flows" drawer
https://bugs.webkit.org/show_bug.cgi?id=90871
Attachment 163805: Patch
https://bugs.webkit.org/attachment.cgi?id=163805&action=review
------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163805&action=review
You may want to somehow remove the tEXt ancillary chunks containing "Creation
Date" and "Software" from namedFlowOverflow.png in order to reduce its size.
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:1
> +/**
License boilerplate header missing
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:72
> + _sidebarHasContent: function()
This naming convention (same for _sidebarIsEmpty) implies that the methods are
accessors return a boolean. You should use something like
"_setSidebarHasContent(boolean)" instead.
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:93
> + var flowContainer = {
No need to expand this onto multiple lines, we usually put this onto a single
line.
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:96
> + }
Trailing ';' missing
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:194
> + // FIXME: we only have support for Named Flows in the main document
Even though it is quite arbitrary in the legacy code, the text following FIXME:
should be a full sentence (capitalized, with a trailing period), and we try to
stick to this convention in the new code. Ditto for the following methods.
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:204
> + if (event.data.documentNodeId !== this._document.id)
In order to avoid compilation errors, declare
var data = /** @type {WebInspector.NamedFlow} */ event.data;
and then handle the |data| var instead.
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:219
> + hashNamedFlow: function(namedFlow)
All of these should be private methods (with a leading underscore)
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:221
> + return "flow" + namedFlow.documentNodeId + namedFlow.name;
Can a flow name start with a digit? If so, we have a potential collision here.
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:224
> + hashRemovedNamedFlow: function(eventData)
You don't need this method (use hashNamedFlow instead).
> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:244
> + showNamedFlowForNode: function(node)
Should be private
> Source/WebCore/inspector/front-end/cssNamedFlows.css:1
> +.css-named-flow-collections-view .split-view-sidebar-left {
License boilerplate header missing
More information about the webkit-reviews
mailing list