1290230 - replace uses of nsIClipboardHelper

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Various spots in the inspector use nsIClipboardHelper.
These uses must be replaced for de-chrome-ification.

Comment 1

[Tom Tromey :tromey]

The web way to do this seems to be document.execCommand("copy"),
coupled with a "copy" event listener that modifies the event
to set the desired text.

See https://w3c.github.io/clipboard-apis/

Comment 2

[Tom Tromey :tromey]

There's also uses of sdk/clipboard

Comment 3

[Tom Tromey :tromey]

Dropping this for the time being in favor of a more important task.

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1290230 - remove unneeded uses of nsIClipboardHelper from devtools;

Review commit: https://reviewboard.mozilla.org/r/72052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72052/

Comment 5

[Tom Tromey :tromey]

Attachment: Bug 1290230 - move clipboard helper to new devtools platform module;

Review commit: https://reviewboard.mozilla.org/r/72054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72054/

Comment 6

[Greg Tatum [:gregtatum]]

Comment on attachment 8781702 [details]
Bug 1290230 - remove unneeded uses of nsIClipboardHelper from devtools;

https://reviewboard.mozilla.org/r/72052/#review69966

::: devtools/client/debugger/debugger-controller.js
(Diff revision 1)
>    "resource://devtools/shared/Parser.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "ShortcutUtils",
>    "resource://gre/modules/ShortcutUtils.jsm");
>  
> -XPCOMUtils.defineLazyServiceGetter(this, "clipboardHelper",

It looks like the clipboardHelper is used as a global, and is being used in 
devtools/client/netmonitor/netmonitor-view.js

https://dxr.mozilla.org/mozilla-central/search?q=clipboardHelper+path%3Aclient%2Fdebugger&redirect=false

I think this change will break things.

::: devtools/client/netmonitor/netmonitor-controller.js
(Diff revision 1)
>    "resource://devtools/client/shared/Curl.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
>    "resource://gre/modules/PluralForm.jsm");
>  
> -XPCOMUtils.defineLazyServiceGetter(this, "clipboardHelper",

Same here:

https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Fnetmonitor+clipboardHelper&redirect=false

Comment 7

[Greg Tatum [:gregtatum]]

Comment on attachment 8781703 [details]
Bug 1290230 - move clipboard helper to new devtools platform module;

https://reviewboard.mozilla.org/r/72054/#review69964

See my comment on the test, but everything else looks great.

::: devtools/shared/platform/content/test/test_clipboard.html:16
(Diff revision 1)
> +        href="chrome://mochikit/content/tests/SimpleTest/test.css">
> +  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
> +
> +<script type="application/javascript;version=1.8">
> +"use strict";
> +var exports = {}

This structure feels weird to me and hard to understand. Would it be possible to load in the clipboard interface through a loader using require? It seems like it would be good to test the clipboard in the same way you would use it in an actual tool.

Comment 8

[Tom Tromey :tromey]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #6)

> It looks like the clipboardHelper is used as a global, and is being used in 
> devtools/client/netmonitor/netmonitor-view.js

Ugh, I hadn't considered that possibility.
If this breaks the tests I will just drop this patch.


> This structure feels weird to me and hard to understand. Would it be
> possible to load in the clipboard interface through a loader using require?
> It seems like it would be good to test the clipboard in the same way you
> would use it in an actual tool.

I don't think we have a version of require() that works in content.
I wrote the test as a plain mochitest to ensure that the environment
would be most content-like.  And, when we are running this in the real
content environment, require() will be some special webpack thing anyway,
so I think testing it via our loaders won't help that eventual goal.
Instead when it all works we'll need an integration test that is exercising
the true code paths.

I hope this makes sense!

Comment 9

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #8)

> If this breaks the tests I will just drop this patch.

Definitely dropping that one.
It isn't really needed for our project anyway, and it did cause failures.

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8781703 [details]
Bug 1290230 - move clipboard helper to new devtools platform module;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72054/diff/1-2/

Comment 11

[Greg Tatum [:gregtatum]]

Comment on attachment 8781703 [details]
Bug 1290230 - move clipboard helper to new devtools platform module;

https://reviewboard.mozilla.org/r/72054/#review70390

Okie doke, that sounds good to me.

Comment 12

[Tom Tromey :tromey]

I'm going to push this one after I can rebase it on top of bug 1290233,
so I can remove that last require("chrome") and avoid breaking eslint as well.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8781703 [details]
Bug 1290230 - move clipboard helper to new devtools platform module;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72054/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58af0782df06
move clipboard helper to new devtools platform module; r=gregtatum

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58af0782df06