Web Inspector: Copy ... actions in the context menu don't work in the remote debugging mode.

[Mikhail Naganov]

Programmatic access to clipboard is disabled by default.

For Chrome, this can be solved by installing a simple extension like this one: http://pastebin.com/kDxUacJd

We also need to support copying in InspectorFrontendHostStub.

[Mikhail Naganov]

Created attachment 142244 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 142244 [details]
Patch

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

You don't seem to have added a corresponding localizedStrings.js entry for the message.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:117
> +            event.clipboardData.setData('text', this._textToCopy);

We use double-quotes around strings in the frontend.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:121
> +        } else {

We don't use braces around one-line blocks

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:129
> +        if (!document.execCommand('copy')) {

We use double-quotes around strings in the frontend.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:206
> +    p.textContent = WebInspector.UIString("You need to install \"Chrome Developer Tools Frontend\" Chrome extension.");

Missing "the"? "...install the \"Chrome....."

[Mikhail Naganov]

Comment on attachment 142244 [details]
Patch

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

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:117
>> +            event.clipboardData.setData('text', this._textToCopy);
> 
> We use double-quotes around strings in the frontend.

Done.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:121
>> +        } else {
> 
> We don't use braces around one-line blocks

Sorry, I was thinking they need to be symmetrical for both if branches. Removed.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:129
>> +        if (!document.execCommand('copy')) {
> 
> We use double-quotes around strings in the frontend.

Done.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:206
>> +    p.textContent = WebInspector.UIString("You need to install \"Chrome Developer Tools Frontend\" Chrome extension.");
> 
> Missing "the"? "...install the \"Chrome....."

Added, thanks!

[Mikhail Naganov]

Created attachment 142250 [details]
Comments addressed

I will add strings to localizedStrings once we agree on wording.

[Peter Beverloo]

Comment on attachment 142250 [details]
Comments addressed

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

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:205
> +    p.textContent = WebInspector.UIString("You need to install the \"Chrome Developer Tools Frontend\" Chrome extension.");

Since this code is in WebCore, it probably shouldn't be referring to specific products (in this case, Chrome)?

[Pavel Feldman]

Comment on attachment 142250 [details]
Comments addressed

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

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:114
> +    documentCopy: function(event)

Why do you override this document's copy?

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:205
>> +    p.textContent = WebInspector.UIString("You need to install the \"Chrome Developer Tools Frontend\" Chrome extension.");
> 
> Since this code is in WebCore, it probably shouldn't be referring to specific products (in this case, Chrome)?

This code should be in WebKit/chromium/src/js/DevTools.js. Who is going to own this extension? Could it be more generic (i.e. "Clipboard access enabler"?) Or we could combine it with the Chrome extension used for filesystem persistence. I would not want to see a separate "Chrome Developer Tools Front-end" as an extension.

[Mikhail Naganov]

Comment on attachment 142250 [details]
Comments addressed

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

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:114
>> +    documentCopy: function(event)
> 
> Why do you override this document's copy?

We need to get the Event instance passed to the copy command handler in order to set clipboardData. I've changed the code insert FrontendHost's action after the event has been processed by the default documentCopy handler.

>>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:205
>>> +    p.textContent = WebInspector.UIString("You need to install the \"Chrome Developer Tools Frontend\" Chrome extension.");
>> 
>> Since this code is in WebCore, it probably shouldn't be referring to specific products (in this case, Chrome)?
> 
> This code should be in WebKit/chromium/src/js/DevTools.js. Who is going to own this extension? Could it be more generic (i.e. "Clipboard access enabler"?) Or we could combine it with the Chrome extension used for filesystem persistence. I would not want to see a separate "Chrome Developer Tools Front-end" as an extension.

Moved platform-specific code to DevTools.js

The extension (in fact, this is a simple Web App) specifies the starting URL (localhost:9222), adds clipboard access permissions, and specifies the frontend hosting URL as the App's URL. I've posted a link to the sample already, have you seen it? http://pastebin.com/kDxUacJd

Do you already have the extension used for filesystem persistence? Can you please point me to it?

[Mikhail Naganov]

Created attachment 142270 [details]
Comments addressed

[Pavel Feldman]

> We need to get the Event instance passed to the copy command handler in order to set clipboardData. I've changed the code insert FrontendHost's action after the event has been processed by the default documentCopy handler.

You can only use event object while it is being dispatched. Storing it is not right.

> The extension (in fact, this is a simple Web App) specifies the starting URL (localhost:9222), adds clipboard access permissions, and specifies the frontend hosting URL as the App's URL. I've posted a link to the sample already, have you seen it? http://pastebin.com/kDxUacJd

Nope, now that I've seen it, name slearly looks wrong ("Chrome Developer Tools Frontend"). You probably want to point to a wiki describing how to enable copy / paste or something.

> Do you already have the extension used for filesystem persistence? Can you please point me to it?

https://github.com/NV/chrome-devtools-autosave,
http://code.google.com/p/devtools-save/,

[Pavel Feldman]

Comment on attachment 142270 [details]
Comments addressed

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

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:116
> +        if (!this._textToCopy)

I still don't understand what is happening here...

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:195
> +function clipboardAccessDeniedMessage() {

{ on the next line. You should not declare top-level functions.

[Pavel Feldman]

Comment on attachment 142270 [details]
Comments addressed

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

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:120
> +        this._textToCopy = null;

delete this._textToCopy.

> Source/WebKit/chromium/src/js/DevTools.js:60
> +    return "You need to install the \"Chrome Developer Tools Frontend\" Chrome extension.";

You need to install Chrome extension that allows clipboard access for this page.

[Mikhail Naganov]

Comment on attachment 142270 [details]
Comments addressed

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

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:116
>> +        if (!this._textToCopy)
> 
> I still don't understand what is happening here...

Discussed offline.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:120
>> +        this._textToCopy = null;
> 
> delete this._textToCopy.

Done.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:195
>> +function clipboardAccessDeniedMessage() {
> 
> { on the next line. You should not declare top-level functions.

Done.

>> Source/WebKit/chromium/src/js/DevTools.js:60
>> +    return "You need to install the \"Chrome Developer Tools Frontend\" Chrome extension.";
> 
> You need to install Chrome extension that allows clipboard access for this page.

Done.

[Mikhail Naganov]

Created attachment 142452 [details]
Comments addressed

[Yury Semikhatsky]

Comment on attachment 142452 [details]
Comments addressed

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

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:216
> +}

Please remove this.

[Mikhail Naganov]

Comment on attachment 142452 [details]
Comments addressed

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

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:216
>> +}
> 
> Please remove this.

What do you mean by "this"? The bracket is a closing for the one in the beginning of the file: "if (!window.InspectorFrontendHost) {". It's the highlighting that is a bit misguiding.

[Mikhail Naganov]

Committed r117481: <http://trac.webkit.org/changeset/117481>