828254 - Provide ability to search for highlighted text

Status: RESOLVED FIXED

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

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130108 Firefox/21.0
Build ID: 20130108033457

Steps to reproduce:

You can share but not search for text. This is one of desktops' most basic features. Mobile should have parity.

Comment 1

[Aaron Train [:aaronmt]]

Clarification: Chrome on Android uses the ActionBar, and the current selection is searched (to Google) via a magnifying glass icon. I think this would be pretty trivial to add (even a mentor bug) to a context-menu for us in browser.js in initContextMenu() and adding a context. Would one want a new tab or re-use the existing?

Comment 2

[Paul [pwd]]

A new tab would follow the UX that we've established on desktop.

Comment 3

[:Margaret Leibovic]

Ian, what's your opinion on this? I feel like we're often wary of adding more context menus, so maybe we should just do this as part of the action bar if/when we fix bug 768667.

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #3)
> Ian, what's your opinion on this? I feel like we're often wary of adding
> more context menus, so maybe we should just do this as part of the action
> bar if/when we fix bug 768667.

That would have been my suggestion, too. We don't even have any context menu / action bar appear yet when you select text, so we would need to address that first. Preferably with an action bar.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > Ian, what's your opinion on this? I feel like we're often wary of adding
> > more context menus, so maybe we should just do this as part of the action
> > bar if/when we fix bug 768667.
> 
> That would have been my suggestion, too. We don't even have any context menu
> / action bar appear yet when you select text, so we would need to address
> that first. Preferably with an action bar.

Actually, we do have a specific menu for selected text:
1. Select some text
2. Long press on the selection
3. Enjoy the selected text menu

There is room on the menu for "Search using Xxx"

Comment 6

[Ian Barlow (:ibarlow)]

Heh, well there you go -- learn something new every day ;)

Comment 7

[:Margaret Leibovic]

I think in inputs there may be even more items than that, which is why I worry about adding another context menu item. But we can test to see how it looks

If we do want to go ahead with the context menu item, this would be a good intern starter bug.

Comment 8

[:Margaret Leibovic]

Attachment: screenshot of context menu for selected text in an input

There are already a lot of context menu items for selected text in an input. Perhaps we should do something to have the selected text context menu replace the input context menu, instead of merging the items like this.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

I might not be typical, but I seem to use the desktop "Search for " context menu when selecting web content, not text in inputs. I see the desktop supports both (which I didn't even realize), but maybe we could only focus on content as a first step.

Aslo, I see that for inputs Chrome, which uses the action bar - a different bug, only supports "Select All", "Cut" and "Copy". We could reduce our long input context menu by removing "Copy All" and "Select Word". Just food for thought.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Patch adding context menu item to search using the selected text as the search terms.

Here`s a patch to fix this particular bug. The discussion seems to have brought up a couple of additional things that need changing in this area - how exactly should we proceed? Mark`s suggestion for how to shorten the long context menu seems sensible. Should the patch be modified such that this new feature does not exist for input fields? (Currently it works both on input fields and ordinary content)

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Patch adding context menu item to search using the selected text as the search terms. Code style tweaks.

Applying style corrections that got another of my patches rejected...

Comment 12

[:Margaret Leibovic]

Comment on attachment 768418 [details] [diff] [review]
Patch adding context menu item to search using the selected text as the search terms. Code style tweaks.

There seems to be something wrong with the formatting of this patch... splinter review isn't showing all the hunks, and it's failing to apply for me locally. Maybe something went wrong while updating the patch?

Comment 13

[:Margaret Leibovic]

I'm curious what ibarlow thinks the string for this menu item should be. I think we can come up with something better than "Search with Selection".

On desktop the context menu item is:

Search Google for "the selected text..."

I feel like we don't have a lot of room for showing a long context menu item, but I do think we should put the search provider in there. Maybe "Search using Google" like mfinkle suggested in comment 5 would be good. I also like his idea to just restrict this to static text content.

For adding the engine name to the context menu item, you'll want to use string bundle's getFormattedString method. The desktop code is a handy example:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1361

Comment 14

[Paul [pwd]]

I'd suggest "Search selected text", "Search for..." or simply "Search". If we're planning to provide more than one search option as per https://addons.mozilla.org/en-US/android/addon/long-tap-search/ I'd suggest agree with comment 5 "Search using XXX"

Comment 15

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #13)
> I'm curious what ibarlow thinks the string for this menu item should be. I
> think we can come up with something better than "Search with Selection".
> 
> On desktop the context menu item is:
> 
> Search Google for "the selected text..."
> 
> I feel like we don't have a lot of room for showing a long context menu
> item, but I do think we should put the search provider in there. Maybe
> "Search using Google" like mfinkle suggested in comment 5 would be good. I
> also like his idea to just restrict this to static text content.
> 
> For adding the engine name to the context menu item, you'll want to use
> string bundle's getFormattedString method. The desktop code is a handy
> example:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> nsContextMenu.js#1361

Maybe "Searchprovider search"?

So the menu item could read "Google search" or "Duckduckgo search", or whatever their default provider has been set to

Comment 16

[:Margaret Leibovic]

Comment on attachment 768418 [details] [diff] [review]
Patch adding context menu item to search using the selected text as the search terms. Code style tweaks.

Setting this to r-, since we need to fix the formatting issue here, and we should update the menu item string to follow ibarlow's suggestion.

Comment 17

[Chris Kitching [:ckitching]]

Attachment: Bug 769262 - Adding context menu item to search with selection. r=margaret.leibovic

Added Ian´s suggestion and hopefully managed not to fail in exporting the patch this time...

Comment 18

[:Margaret Leibovic]

Comment on attachment 769262 [details] [diff] [review]
Bug 769262 - Adding context menu item to search with selection. r=margaret.leibovic

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

::: mobile/android/chrome/content/browser.js
@@ +5840,5 @@
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.selectAll"), ClipboardHelper.selectAllContext, ClipboardHelper.selectAll.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.share"), ClipboardHelper.shareContext, ClipboardHelper.share.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.paste"), ClipboardHelper.pasteContext, ClipboardHelper.paste.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.changeInputMethod"), NativeWindow.contextmenus.textContext, ClipboardHelper.inputMethod.bind(ClipboardHelper));
> +    NativeWindow.contextmenus.add(Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1), ClipboardHelper.searchWithContext, ClipboardHelper.searchWith.bind(ClipboardHelper));

If the user changes the default search engine, this string doesn't update itself. To fix this, you should observe the "browser-search-engine-modified" notification, and update the context menu when appropriate. It looks like we can even just watch for the specific case when the default engine changes, since the string "engine-default" will be included in the data of the notification:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3554

I don't see an API to update a contextmenu item label, so you may just have to remove/re-add with the new string.

It looks like contextmenus.add returns an id:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1704

Which you can keep track of and call contextmenus.remove, followed by contextmenus.add again.

You'll probably want to factor this logic out into a helper method.

@@ +5879,5 @@
>    },
>  
> +  searchWith: function(aElement) {
> +    let string = SelectionHandler._getSelectedText();
> +    if(string) {

Nit: Space between if and paren.

@@ +5882,5 @@
> +    let string = SelectionHandler._getSelectedText();
> +    if(string) {
> +      let req = Services.search.defaultEngine.getSubmission(string);
> +      BrowserApp.selectOrOpenTab(req.uri.spec);
> +    }

I'd prefer to move this all into a helper method on SelectionHandler, something like SelectionHandler.searchSelection(). This would be more consistent with the other methods used around here, and it would also let us avoid calling _getSelectedText, which is currently only used within SelectionHandler (prepending an underscore to method names is a JS convention for considering the method private).

Comment 19

[Chris Kitching [:ckitching]]

Attachment: V3 - Adding default change event tracking.

Here is a revised patch - I hadn't realised the thing about underscores in JS - that's handy to know - thanks!

Comment 20

[:Margaret Leibovic]

Comment on attachment 771732 [details] [diff] [review]
V3 - Adding default change event tracking.

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

This is looking good, but I have enough comments that I'd like to see a new version of the patch. And please remember to use 2-space indentation in JS files.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +274,5 @@
> +      let string = SelectionHandler._getSelectedText();
> +      if (string) {
> +          let req = Services.search.defaultEngine.getSubmission(string);
> +          BrowserApp.selectOrOpenTab(req.uri.spec);
> +      }

Looking at shareSelection and copySelection, I think you should also put a call to this._closeSelection() at the end of this function. I would also prefer if you placed this method down next to those other two methods.

::: mobile/android/chrome/content/browser.js
@@ +5813,5 @@
>    }
>  };
>  
>  var ClipboardHelper = {
> +  _searchMenuItem: -1, //Recorded so it can be removed/replaced when default engine changed.

Nit: Place comment above property, and also put a space between the "//" and the text of the comment.

@@ +5823,5 @@
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.selectAll"), ClipboardHelper.selectAllContext, ClipboardHelper.selectAll.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.share"), ClipboardHelper.shareContext, ClipboardHelper.share.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.paste"), ClipboardHelper.pasteContext, ClipboardHelper.paste.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.changeInputMethod"), NativeWindow.contextmenus.textContext, ClipboardHelper.inputMethod.bind(ClipboardHelper));
> +    this._searchMenuItem = NativeWindow.contextmenus.add(Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1), ClipboardHelper.searchWithContext, ClipboardHelper.searchWith.bind(ClipboardHelper));

Nit: Put a blank line above this line to make this more readable.

@@ +5824,5 @@
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.share"), ClipboardHelper.shareContext, ClipboardHelper.share.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.paste"), ClipboardHelper.pasteContext, ClipboardHelper.paste.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.changeInputMethod"), NativeWindow.contextmenus.textContext, ClipboardHelper.inputMethod.bind(ClipboardHelper));
> +    this._searchMenuItem = NativeWindow.contextmenus.add(Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1), ClipboardHelper.searchWithContext, ClipboardHelper.searchWith.bind(ClipboardHelper));
> +    Services.obs.addObserver(this, "browser-search-engine-modified", false); //Watch for changes to the default search engine so the above label can be tweaked.

Same nit about the comment.

You should also add an uninit method that removes this observer at shutdown. See BrowserApp.shutdown for examples of other objects that have uninit methods.

@@ +5830,5 @@
> +
> +  observe: function observe(aSubject, aTopic, aData) {
> +      if (aTopic == "browser-search-engine-modified" && aData == "engine-default") {
> +          NativeWindow.contextmenus.remove(this._searchMenuItem); //Update search with context menu element...
> +          this._searchMenuItem = NativeWindow.contextmenus.add(Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1), ClipboardHelper.searchWithContext, ClipboardHelper.searchWith.bind(ClipboardHelper));

I know this is just one line, but let's factor this out into a helper method, since this exactly the same as up above and relatively complicated (you may also want to factor the label out into a local variable to make it this more readable. Something like _addSearchMenuItem would be a good name for that helper method.

@@ +5870,5 @@
>      SelectionHandler.selectAll(aElement, aX, aY);
>    },
>  
> +  searchWith: function(aElement) {
> +    SelectionHandler.searchSelection(aElement);

Nice, this is much cleaner (and more consistent with the neighboring code).

@@ +5943,5 @@
>      }
>    },
>  
> +  searchWithContext: {
> +      matches: function (aElement, aX, aY) {

Nit: Give this function a name (makes console messages better in the unlikely event of an error).

Comment 21

[Chris Kitching [:ckitching]]

Attachment: V4 - More denitting.

Denitting applied.

Comment 22

[:Margaret Leibovic]

Comment on attachment 772525 [details] [diff] [review]
V4 - More denitting.

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

This looks good, but you should address these additional comments before landing.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +388,5 @@
>      this._closeSelection();
>    },
>  
> +  searchSelection: function sh_searchSelection() {
> +    let string = SelectionHandler._getSelectedText();

Oops, sorry I didn't notice this in the last patch, this should just be this._getSelectedText().

@@ +392,5 @@
> +    let string = SelectionHandler._getSelectedText();
> +    if (string) {
> +      let req = Services.search.defaultEngine.getSubmission(string);
> +      BrowserApp.selectOrOpenTab(req.uri.spec);
> +      this._closeSelection();

copySelection/shareSelection both close the selection regardless of whether or not there was a valid selection string, so we should probably do that here as well. Also, I feel like I would like us to follow the same variable naming convention as in those methods (selectedText instead of string). Sorry for being so nit-picky! I feel like this has become an exercise is matching neighboring code style :)

::: mobile/android/chrome/content/browser.js
@@ +5831,5 @@
> +    Services.obs.addObserver(this, "browser-search-engine-modified", false);
> +  },
> +
> +  uninit: function uninit() {
> +    Services.obs.removeObserver(this, "browser-search-engine-modified", false);

removeObserver only takes two parameters:
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService.idl#50

Comment 23

[Chris Kitching [:ckitching]]

Attachment: Patch providing this feature.. V5: Return of the nits.

I really did manage to pack an impressive number of nits into such a small patch.

Comment 24

[:Margaret Leibovic]

Comment on attachment 772783 [details] [diff] [review]
Patch providing this feature.. V5: Return of the nits.

Nice :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5720a99657f

Comment 26

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

Backed out for causing oranges:
https://hg.mozilla.org/integration/mozilla-inbound/rev/441eb0b1eb85

Comment 27

[:Margaret Leibovic]

Chris pushed this to try to see if it was indeed the offending changeset in that orange push:
https://tbpl.mozilla.org/?tree=Try&rev=d5096ac6e9d6

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Btw, areweslimyet.com/mobile reported a startup memory spike recently that went away a few pushes later. The regression range [1] and fix range [2] seem to implicate this patch.

[1] http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=468b35185c44&tochange=86e71d868f45
[2] http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=faa78d7c3c42&tochange=441eb0b1eb85

Once the test failures are sorted out we should also check for memory regressions before re-landing.

Comment 29

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #27)
> Chris pushed this to try to see if it was indeed the offending changeset in
> that orange push:
> https://tbpl.mozilla.org/?tree=Try&rev=d5096ac6e9d6

Looks like this patch is to blame after all...

Chris, can you try to get robocop tests running locally and see if you can reproduce this failure locally? I was the one who wrote that test, so I can look at it more closely.

The first thing to experiment with would be getting rid of the observer (that might have caused the memory spike kats saw). However, I also wonder if the call to Services.search.defaultEngine in searchWithContext is causing the search service to now be initialized before our distribution is set up, tickling some race condition. I actually don't think we need that check, so we could get rid of it if necessary, but it would be unfortunate for that race condition to exist.

Comment 30

[:Margaret Leibovic]

Gavin tells me it's actually bad to be calling Services.search.defaultEngine like this because it will synchronously initialize the search service, which is a bad thing to be doing during startup regardless of whether or not it causes a test failure. So let's get rid of that, and hopefully that will fix the test issue.

Comment 31

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #30)
> Gavin tells me it's actually bad to be calling Services.search.defaultEngine
> like this because it will synchronously initialize the search service, which
> is a bad thing to be doing during startup regardless of whether or not it
> causes a test failure. So let's get rid of that, and hopefully that will fix
> the test issue.

I pushed to try to test out this theory:
https://tbpl.mozilla.org/?tree=Try&rev=25bda688e1f7

Comment 32

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #31)
> (In reply to :Margaret Leibovic from comment #30)
> > Gavin tells me it's actually bad to be calling Services.search.defaultEngine
> > like this because it will synchronously initialize the search service, which
> > is a bad thing to be doing during startup regardless of whether or not it
> > causes a test failure. So let's get rid of that, and hopefully that will fix
> > the test issue.
> 
> I pushed to try to test out this theory:
> https://tbpl.mozilla.org/?tree=Try&rev=25bda688e1f7

That didn't work :(

But I noticed we get the search engine name in _setSearchMenuItem. I made another push to try to see if this is the problem:
https://tbpl.mozilla.org/?tree=Try&rev=0fbcd12406c4

If it is, maybe we can avoid calling _setSearchMenuItem in init, and just wait for the observer to be notified (which it should be whenever the search service gets initialized).

Comment 33

[Chris Kitching [:ckitching]]

Yes, it seems we could lazily set the string when the contextmenu is being build - that might be better for startup performance, anyway.

Comment 34

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #32)
> (In reply to :Margaret Leibovic from comment #31)
> > (In reply to :Margaret Leibovic from comment #30)
> > > Gavin tells me it's actually bad to be calling Services.search.defaultEngine
> > > like this because it will synchronously initialize the search service, which
> > > is a bad thing to be doing during startup regardless of whether or not it
> > > causes a test failure. So let's get rid of that, and hopefully that will fix
> > > the test issue.
> > 
> > I pushed to try to test out this theory:
> > https://tbpl.mozilla.org/?tree=Try&rev=25bda688e1f7
> 
> That didn't work :(
> 
> But I noticed we get the search engine name in _setSearchMenuItem. I made
> another push to try to see if this is the problem:
> https://tbpl.mozilla.org/?tree=Try&rev=0fbcd12406c4
> 
> If it is, maybe we can avoid calling _setSearchMenuItem in init, and just
> wait for the observer to be notified (which it should be whenever the search
> service gets initialized).

Aha, this is green. So let's just try a patch that follows this suggestion (initializing the contextmenu item only when we observe the notification).

Comment 35

[Chris Kitching [:ckitching]]

Pushed such a patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=615625e78e26

Comment 36

[Chris Kitching [:ckitching]]

Attachment: V6 - No more oranges.

A patch that doesn't break Firefox.

Comment 37

[:Margaret Leibovic]

Comment on attachment 778641 [details] [diff] [review]
V6 - No more oranges.

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

This patch is different than what I had in mind... is there a reason you got rid of the observer? This will remove/add the menuitem every time _sendToContent is called, which seems like overkill. It also feels un-modular to be calling ClipboardHelper._setSearchMenuItem in _sendToContent.

My suggestion from comment 32 was just to avoid calling _setSearchMenuItem in init, and instead only do it when we observe the notification. Did you try that?

Comment 38

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 778641 [details] [diff] [review]
V6 - No more oranges.

>+  _setSearchMenuItem: function setSearchMenuItem() {
>+    if (this._searchMenuItem) {
>+      NativeWindow.contextmenus.remove(this._searchMenuItem);
>+    }
>+    this._searchMenuItem = NativeWindow.contextmenus.add(Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1), ClipboardHelper.searchWithContext, ClipboardHelper.searchWith.bind(ClipboardHelper));
>   },

drive-by:
* rename to addSearchMenu ?
* no need for the '_' since it's used publicly

Comment 39

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #38)
> Comment on attachment 778641 [details] [diff] [review]
> V6 - No more oranges.
> 
> >+  _setSearchMenuItem: function setSearchMenuItem() {
> >+    if (this._searchMenuItem) {
> >+      NativeWindow.contextmenus.remove(this._searchMenuItem);
> >+    }
> >+    this._searchMenuItem = NativeWindow.contextmenus.add(Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1), ClipboardHelper.searchWithContext, ClipboardHelper.searchWith.bind(ClipboardHelper));
> >   },
> 
> drive-by:
> * rename to addSearchMenu ?
> * no need for the '_' since it's used publicly

mfinkle, do you like this new approach of removing/adding the menuitem in _sendToContent, as opposed to when a default search engine is set? I feel like it's more elegant to just do it when the search engine is set.

Comment 40

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #39)

> mfinkle, do you like this new approach of removing/adding the menuitem in
> _sendToContent, as opposed to when a default search engine is set? I feel
> like it's more elegant to just do it when the search engine is set.

Honestly never looked at it. Looking now...

Comment 41

[Chris Kitching [:ckitching]]

Doing this when the menu is created is a tad overkill, but doing it just when search engine changed events come in leads to it never getting set until you go into the preferences and change your search engine, I believe - this is why I added it to init in the first place.

But now we've determined that doing this in init isn't good it's not hugely clear how to proceed. I guess we could check for existence when the menu is about to be shown and if the item does not exist then create it, as well as listening for search engine modified events to alter it at that point (So effectively the first call to show the menu becomes a sort of init) - but this seems somewhat ugly.

Comment 42

[:Margaret Leibovic]

Comment on attachment 778641 [details] [diff] [review]
V6 - No more oranges.

Following discussion on IRC, let's have _sendToContent fire a notification that ClipboardHelper can listen for to set up the menuitem, instead of directly calling into ClipboardHelper from _sendToContent. We could name it something like "prepare-context-menu" (not sure if I like that, I bet mfinkle will chime in with a naming suggestion).

As a follow-up, it could be nice to look into using a notification like that as a different way for people to register context menu items, as opposed to using context matching on a potentially large list of context items.

Comment 43

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #41)
> Doing this when the menu is created is a tad overkill, but doing it just
> when search engine changed events come in leads to it never getting set
> until you go into the preferences and change your search engine, I believe -
> this is why I added it to init in the first place.
> 
> But now we've determined that doing this in init isn't good it's not hugely
> clear how to proceed. I guess we could check for existence when the menu is
> about to be shown and if the item does not exist then create it, as well as
> listening for search engine modified events to alter it at that point (So
> effectively the first call to show the menu becomes a sort of init) - but
> this seems somewhat ugly.

Sorry, didn't see this last comment. We can just do the add/remove business when _sendToContent is called like your current patch does (all those calls to .matches are probably more of a perf bottleneck in that function anyway). But let's just do it with a notification/observer instead of directly calling into ClipboardHelper.

Comment 44

[Chris Kitching [:ckitching]]

Attachment: V7 - Notify-observer used.

Comment 45

[:Margaret Leibovic]

Comment on attachment 781796 [details] [diff] [review]
V7 - Notify-observer used.

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

This is looking good, but let's land a patch with a better notification topic name.

::: mobile/android/chrome/content/browser.js
@@ +2004,5 @@
>  
>        this.menuitems = [];
>        let menuitemsSet = false;
>  
> +      // It's time to set the search menu item - we avoided doing it on startup so we can get away without the search service for longer.

This notification isn't specifically about the search menuitem, so this comment shouldn't talk about it.

@@ +2005,5 @@
>        this.menuitems = [];
>        let menuitemsSet = false;
>  
> +      // It's time to set the search menu item - we avoided doing it on startup so we can get away without the search service for longer.
> +      Services.obs.notifyObservers(null, "ContextMenu:Open", "");

I would prefer if we used a topic that better describes exactly when this notification is fired, which is before we prepare the list of items to show. In comment 42 I suggested "prepare-context-menu". But maybe "before-build-contextmenu" would be better. We can bikeshed on names, but I think we should pick something better than "ContextMenu:Open", since that's not really what this notification represents.

Also, as a convention, notifications that are just on the gecko side use foo-bar formatting, instead of Foo:Bar formatting. It's not really a big deal, but it's nice to be consistent.

@@ +5923,5 @@
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.selectAll"), ClipboardHelper.selectAllContext, ClipboardHelper.selectAll.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.share"), ClipboardHelper.shareContext, ClipboardHelper.share.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.paste"), ClipboardHelper.pasteContext, ClipboardHelper.paste.bind(ClipboardHelper));
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.changeInputMethod"), NativeWindow.contextmenus.textContext, ClipboardHelper.inputMethod.bind(ClipboardHelper));
> +    Services.obs.addObserver(this, "ContextMenu:Open", false);

Please put a blank line above this, and add a comment about why we're adding this. Something similar to that comment you currently have up in _sendToContent would be good.

@@ +5927,5 @@
> +    Services.obs.addObserver(this, "ContextMenu:Open", false);
> +  },
> +
> +  uninit: function ch_uninit() {
> +    Services.obs.removeObserver(this, "ContextMenu:Open", false);

removeObserver only takes two parameters, get rid of the third.

Comment 46

[Chris Kitching [:ckitching]]

Attachment: V8 - Better names and even more nits.

Round we go...

Comment 47

[:Margaret Leibovic]

Comment on attachment 781831 [details] [diff] [review]
V8 - Better names and even more nits.

This looks great, thanks!

Let's push to try before landing this, just to double check that things are still green with these changes.

Comment 48

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=655498beff2c

Comment 49

[Chris Kitching [:ckitching]]

Looks shiny and green.. ish. Should I be worried about the rc1 orange that went green on restart? I guess this is an intermittent bug in the test, not my patch?

Comment 50

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

(In reply to Chris Kitching [:ckitching] from comment #49)
> Looks shiny and green.. ish. Should I be worried about the rc1 orange that
> went green on restart? I guess this is an intermittent bug in the test, not
> my patch?

Yeah, that's an intermittent failure. (Bug 851861)

Comment 51

[Chris Kitching [:ckitching]]

Splendid. Let's land this.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22c60e4b3242

Comment 54

[Francesco Lodolo [:flod]]

When you add strings with variables, please add a localization comment explaining what the variable is. I'll open a follow-up bug for that.