[webkit-reviews] review canceled: [Bug 90871] Web Inspector: Display Named Flows in the "CSS Named Flows" drawer : [Attachment 163837] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled 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 163837: Patch
https://bugs.webkit.org/attachment.cgi?id=163837&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163837&action=review


> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:68
> +    appendApplicableItems: function(contextMenu, target)

Please annotate the methods that have arguments and/or return values. This will
immensely simplify the code maintenance and improve code quality. Sorry I
didn't mention this in the original review.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:249
> +    _hashNamedFlow: function(namedFlow, eventData)

Ugh, this is definitely not what I meant... I didn't notice that the methods
had different argument types. But to keep things simple, you can provide
documentNodeId and flowName as separate arguments to this method (since these
are the only things that are actually needed.) The calls will be, respectively,


this._hashNamedFlow(flow.documentNodeId, flow.name);
and
this._hashNamedFlow(event.data.documentNodeId, event.data.flowName);

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:252
> +	       return "flow" + namedFlow.documentNodeId + "|" + namedFlow.name;


While we are here, does the "flow" prefix do anything useful? Are you perhaps
going to store some other kind of hash in the same "map"?