Lazy load ClipboardHelper

RESOLVED FIXED in Firefox 31

Status

()

Firefox for Android
Text Selection
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: capella)

Tracking

({dev-doc-needed})

Trunk
Firefox 31
ARM
Android
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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!
(Reporter)

Comment 1

4 years ago
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.
Assignee: nobody → mozbugs.retornam
(Assignee)

Comment 2

4 years ago
I'm curious if you're still active on this? Interested in passing it off?
Flags: needinfo?(mozbugs.retornam)
(Assignee)

Comment 3

4 years ago
Ping?
(Assignee)

Updated

4 years ago
Assignee: mozbugs.retornam → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8400030 [details] [diff] [review]
bug951374.diff (v0)

First review starts with wes for the actionbar impacts...
Attachment #8400030 - Flags: review?(wjohnston)
(Assignee)

Updated

4 years ago
Flags: needinfo?(mozbugs.retornam)
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.
Attachment #8400030 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 8402544 [details] [diff] [review]
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...
Attachment #8402544 - Flags: review?(wjohnston)
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...)
Attachment #8402544 - Flags: review?(wjohnston) → review+
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"
Keywords: dev-doc-needed
(Assignee)

Comment 9

4 years ago
Try push
https://tbpl.mozilla.org/?tree=Try&rev=9e3877f80e71
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/aa43193830a1
https://hg.mozilla.org/mozilla-central/rev/aa43193830a1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.