[webkit-reviews] review denied: [Bug 69924] Web Inspector: add "Add to Watch" option to context menu on selection in source frame : [Attachment 110676] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 69924: Web Inspector: add "Add to Watch" option to context menu on
selection in source frame
https://bugs.webkit.org/show_bug.cgi?id=69924

Attachment 110676: patch
https://bugs.webkit.org/attachment.cgi?id=110676&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110676&action=review


> Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:114
> +    bool includeDefaultItems = args.Length() > 2 &&
args[2]->ToBoolean()->Value();

what if it is not boolean?

> Source/WebCore/inspector/front-end/ContextMenu.js:91
> +	   this._includeDefaultItems = !!value;

make it = value and annotate as @param {boolean} instead!

> Source/WebCore/inspector/front-end/SourceFrame.js:657
> +	   if (!this._delegate.debuggerPaused())

Is there a reason you want it to be only enabled while paused?

>> Source/WebCore/page/ContextMenuController.h:56
>> +	    void showContextMenu(Event*, PassRefPtr<ContextMenuProvider>, bool
includeDefaultItems);
> 
> nit: WebKit coding style guide recommends to use enum instead of boolean for
parameters.

It sounds like you are adding this flag for the sake of chromium port only.
Could we do a better job in detecting the 'input' or 'selection' context menus
instead and show standard menu items for those by default?