Bug 69924 – Web Inspector: add "Add to Watch" option to context menu on selection in source frame

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

- add "Add to Watch" option to context menu in the source frame while debugger is paused
- add a flag to platform context menu to enable both custom and default items be displayed

[Andrey Kosyakov]

Created attachment 110676 [details]
patch

[Yury Semikhatsky]

Comment on attachment 110676 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110676&action=review

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

[Yury Semikhatsky]

Comment on attachment 110676 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110676&action=review

> Source/WebCore/inspector/front-end/TextViewer.js:300
> +            }

style nit: else should go on the same line

[Pavel Feldman]

Comment on attachment 110676 [details]
patch

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?

[Andrey Kosyakov]

Created attachment 111262 [details]
patch

- removed explicit flag for including both custom and default items, just do this if we have selection

[WebKit Review Bot]

Attachment 111262 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 111262 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111262&action=review

> Source/WebCore/inspector/front-end/TextViewer.js:300
> +                this._delegate.populateTextAreaContextMenu(contextMenu);

Nit: I'd simply pass selection in.

[Roland Takacs]

This patch is already in the trunk.