951374 - Lazy load ClipboardHelper

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[:Margaret Leibovic]

If I'm following things correctly, it looks like ClipboardHelper is only used in SelectionHandler now. If this is true, we just stick it in a lazy-loaded script that we load in SelectionHandler.js. Less stuff in browser.js FTW!

Comment 1

[:Margaret Leibovic]

In fact, a lot of the methods on ClipboardHelper, like all the different fooContext methods, are only used once in SelectionHandler, so we might as well rip those out and put them directly in SelectionHandler.

Comment 2

[Mark Capella [:capella]]

I'm curious if you're still active on this? Interested in passing it off?

Comment 3

[Mark Capella [:capella]]

Ping?

Comment 4

[Mark Capella [:capella]]

Attachment: bug951374.diff (v0)

First review starts with wes for the actionbar impacts...

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8400030 [details] [diff] [review]
bug951374.diff (v0)

Review of attachment 8400030 [details] [diff] [review]:
-----------------------------------------------------------------

Yay! Some nits. I can re-review if you want, but I'll leave it up to you. Our testing on this code is awful though (I haven't landed my few tests for this, but I don't think they'd catch this anyway...)

::: mobile/android/chrome/content/SelectionHandler.js
@@ +440,5 @@
>  
>      return obj[name];
>    },
>  
> +  _sendMessage: function(aMsgType, handles) {

Just leave this "type" to match "handles".

@@ +499,3 @@
>        order: 5,
> +      selector: {
> +        matches: function(aElement) {

I'd dump all the 'a' prefixes here as well.

@@ +520,5 @@
>        },
>        order: 4,
> +      selector: {
> +        matches: function(aElement) {
> +          if (NativeWindow.contextmenus.textContext.matches(aElement)) {

Does anyone else use the textContext stuff? Can we move it into SelectHandler as well?

@@ +556,5 @@
>        action: function(aElement) {
> +        if (aElement && (aElement instanceof Ci.nsIDOMNSEditableElement)) {
> +          let target = aElement.QueryInterface(Ci.nsIDOMNSEditableElement);
> +          target.editor.paste(Ci.nsIClipboard.kGlobalClipboard);
> +          target.focus();  

trailing ws.

@@ +565,5 @@
> +      selector: {
> +        matches: function(aElement) {
> +          if (NativeWindow.contextmenus.textContext.matches(aElement)) {
> +            let flavors = ["text/unicode"];
> +            let clipboard = Cc["@mozilla.org/widget/clipboard;1"].getService(Ci.nsIClipboard);

We should start using Services.clipboard.

@@ +614,5 @@
>          SelectionHandler.callSelection();
>        },
>        order: 1,
>        selector: {
> +        matches: function isPhoneNumber() {

No need to name this.

Comment 6

[Mark Capella [:capella]]

Attachment: bug951374.diff (v1)

Let's do a quick re-review ... I cleaned up a bit in SELECT_ALL's |action| that was superflous ... also fixed up the nits / Services.clipboard etc ...

I fixed back the aMsgType to msgType, since it makes that routine consistent, but I didn't refactor any others, as most of the module still uses the |aParm| style.

Finally, I fixed up the textContext stuff ... had originally avoided it as it's public ---> contextmenus ---> NativeWindow and I wasn't sure of add-ons exposure...

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8402544 [details] [diff] [review]
bug951374.diff (v1)

Review of attachment 8402544 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ -2042,5 @@
>          return false;
>        }
>      },
>  
> -    textContext: {

You're probably right about add-ons and this. Lets break it and see what happens. We haven't ever documented these before, but maybe the MDN guys have ways to communicate these things (or maybe we should document these...)

Comment 8

[Wesley Johnston (:wesj)]

doc's people, these "Selectors" aren't documented right now, but do we have ways to document this in release notes or something at least? i.e. "Add-on breaking changes in FF31"

Comment 9

[Mark Capella [:capella]]

Try push
https://tbpl.mozilla.org/?tree=Try&rev=9e3877f80e71

Comment 10

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/aa43193830a1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa43193830a1