[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 bugzilla-daemon at webkit.org
Sun Oct 16 10:48:25 PDT 2011

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

Attachment 110676: patch

------- 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 &&

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
> nit: WebKit coding style guide recommends to use enum instead of boolean for

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?

More information about the webkit-reviews mailing list