899167 - Code Cleanup new SelectionHandler messages for Find-In-Page processing.

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Recent changes for bug 896764 introduced two new messages "SelectedText:Get", and "SelectedText:Data". We'd like to change these to "TextSelection:Get" and "TextSelection:Data".

Additionally, the messages are being caught and processed in browser.js, but should be moved into SelectionHandler.js which is doing the actual work.

Comment 1

[Adrian May [:adrianmay]]

Sounds trivial. Gimme a while to set up my environment and I'll post a patch. Do I assign it to myself or does somebody else do that for me? I have a Samsung Note and an old HTC Desire for testing. I'm more into C++ than JS, so any pointers as to what would be a good next bug for me would be appreciated.

Comment 2

[Mark Capella [:capella]]

New contributors aren't automatically allowed to assign bugs to themselves so I'll do it for you.

This particular bug is in the Firefox mobile / Android area, which is almost all Java and JS. For C++ stuff, you can look into Bugsahoy: http://www.joshmatthews.net/bugsahoy/ and ask around the mozilla #introduction channel.

I'll see if I can spot anything C++ in the meantime, but there's other good Mobile bugs we can use help with ;-)

Comment 3

[Adrian May [:adrianmay]]

Hi Mark,

It just looks like a search/replace job to me. Am I missing something? I was gonna grep the whole codebase, not just the 896764 patch. I'm not totally clueless about Java/JS: I managed to write an OpenGL wallpaper which involves a lot of Java mantra. I once did a Java programming job too. I just mean that C++ is my strongpoint.

Comment 4

[Adrian May [:adrianmay]]

Attachment: TextSelection_899167.patch

I don't know of a test case for this, but the feature (populating the find-in-page bar with any selected text on the page) still works on my phone.

Comment 5

[Mark Capella [:capella]]

This looks great! Part of the change is to preserve a bit of existing behavior where if you go to a page and click the menu item for |Find in Page| without first selecting a search string, the softKBD opens up for convenient user input.

The original bug talks about this, and basically details the answer starting somewhere around bug 896764 comment 14.

mark

Comment 6

[Mark Capella [:capella]]

Forgot to mention, usually it's better to do a more exhaustive search of the entire codebase than just the original patch involved, as you suggested. You can quickly scan mozilla-central using the tool here: http://mxr.mozilla.org/mozilla-central/search

Comment 7

[Adrian May [:adrianmay]]

Attachment: TextSelection_899167_2.patch

Hmm. Not so trivial after all. I guess I should learn to RTFB before ploughing in :-)

Now I put SelectionHandler in that lazy loader list and it pops up the keyboard only if there's no selection. Also, if sometime earlier the user had hit Find with something selected, but now hits find with nothing selected, the earlier text will be suggested and highlighted and the keyboard will appear. I think that's the desired behaviour, isn't it?

Would you like any other tweaks?

Comment 8

[Mark Capella [:capella]]

Comment on attachment 785476 [details] [diff] [review]
TextSelection_899167_2.patch

Your observed behaviours are correct. I'll ask you to make one small change I forgot about, then if you test out the patch all right you can go ahead and re-post it and flag margaret for the final review.

( She's thorough, but I don't think she'll find anything earth shattering :-D )

In my first patch I had to call _getSelectedText() in SelectionHandler.js from browser.js. The function had previously been fully private to SelectionHandler.js hence it started with |_|, a mozilla standard, and I renamed references to it to be the public style without the |_|.

Now that it's fully private again (search to be sure?) we can rename it back. That's it !

Comment 9

[Adrian May [:adrianmay]]

Attachment: TextSelection_899167_3.patch

Seems to be purely private, and the features still work on my phone. So does copy.

But I'm noticing bugs about highlighting the selected text on the page. Basically, the wedge-shaped handles are always correct, but to get the background of the selcted text to turn orange, I often need to force a redraw by scrolling, zooming, or the like. Did I just break that or is it already known?

Comment 10

[Mark Capella [:capella]]

Not sure what you're seeing there ... maybe grab me in #IRC and explain?

Also, just noticed your |Services.obs.removeObserver(this, "TextSelection:Get", false);| statement has an extraneous parameter |false|, and in your final patch after approval and before pushing you'll need to add a |r=margaret| to the commit comment in your patch.

Comment 11

[:Margaret Leibovic]

Comment on attachment 785486 [details] [diff] [review]
TextSelection_899167_3.patch

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

Thanks for the patch! (And thanks for the mentoring, Capella!)

This is on the right track, but there are a few issues with adding/removing the observers, so we'll need to update the patch before landing it.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +51,5 @@
>      Services.obs.addObserver(this, "Tab:Selected", false);
>      Services.obs.addObserver(this, "after-viewport-change", false);
>      Services.obs.addObserver(this, "TextSelection:Move", false);
>      Services.obs.addObserver(this, "TextSelection:Position", false);
> +    Services.obs.addObserver(this, "TextSelection:Get", false);

This isn't going to work right... we don't want to be adding/removing this observer in _addObservers/_removeObservers, since those are called when selection starts/ends. Instead, we want to always be able to observe this event, regardless of whether or not there is an active selection.

Luckily, LazyNotificationGetter takes care of this for us, so you can just get rid of these addObserver/removeObserver calls in here.

::: mobile/android/chrome/content/browser.js
@@ +99,5 @@
>    ["FindHelper", ["FindInPage:Find", "FindInPage:Prev", "FindInPage:Next", "FindInPage:Closed", "Tab:Selected"], "chrome://browser/content/FindHelper.js"],
>    ["PermissionsHelper", ["Permissions:Get", "Permissions:Clear"], "chrome://browser/content/PermissionsHelper.js"],
>    ["FeedHandler", ["Feeds:Subscribe"], "chrome://browser/content/FeedHandler.js"],
>    ["Feedback", ["Feedback:Show"], "chrome://browser/content/Feedback.js"],
> +  ["SelectionHandler", ["TextSelection:Get"], "chrome://browser/content/SelectionHandler.js"],

We are already including SelectionHandler as a normal lazy-loaded script up above. Since we now want to observe this notification, this is correct, but you should remove the line up above where we currently include SelectionHandler.

Comment 12

[:Margaret Leibovic]

(In reply to Adrian May [:adrianmay] from comment #9)

> But I'm noticing bugs about highlighting the selected text on the page.
> Basically, the wedge-shaped handles are always correct, but to get the
> background of the selcted text to turn orange, I often need to force a
> redraw by scrolling, zooming, or the like. Did I just break that or is it
> already known?

Can you reproduce this issue without your patch applied? If so, could you file a bug with steps to reproduce? If not, we should figure out what caused this to break and fix it before landing your patch :)

Comment 13

[Adrian May [:adrianmay]]

Attachment: TextSelection_899167_4.patch

Hi Margaret,

So I took out three lines: the add/removeObserver calls, and the old entry in the top list for SelectionHandler. I think that's what you meant and it seems to work. But I'm not sure how the SelectionHandler get's wired up to all the other events it's interested in, like TextSelection:Move/Position, Window:Resize, Tab:Selected, Gesture:SingleTap and after-viewport-change. I guess they worked before though.

As for the not-orange bug, I only saw it once and can't seem to reproduce it with or without my changes, despite trying extensively, but it's not necessarily related to my changes. I guess people are normally working on something or other when they see bugs, but that doesn't mean there's a connection. It wasn't my imagination, so I guess it's a random multithreading thing. If you point me at the code that handles that orangeness I might just spot a race condition and then figure out how to provoke it. It's not really a show-stopper anyway.

For the next bug, I could find another mentored one, or you might like to point me at something urgent. I'll leave that up to you.

Adrian.

Comment 14

[:Margaret Leibovic]

(In reply to Adrian May [:adrianmay] from comment #13)

> So I took out three lines: the add/removeObserver calls, and the old entry
> in the top list for SelectionHandler. I think that's what you meant and it
> seems to work. But I'm not sure how the SelectionHandler get's wired up to
> all the other events it's interested in, like TextSelection:Move/Position,
> Window:Resize, Tab:Selected, Gesture:SingleTap and after-viewport-change. I
> guess they worked before though.

If you look through the logic in startSelection, you'll see that the observers get added there (SelectionHandler.startSelection is called directly from browser.js).

> As for the not-orange bug, I only saw it once and can't seem to reproduce it
> with or without my changes, despite trying extensively, but it's not
> necessarily related to my changes. I guess people are normally working on
> something or other when they see bugs, but that doesn't mean there's a
> connection. It wasn't my imagination, so I guess it's a random
> multithreading thing. If you point me at the code that handles that
> orangeness I might just spot a race condition and then figure out how to
> provoke it. It's not really a show-stopper anyway.

Was it that the highlight color was black instead of orange? I've seen that before when the content isn't focused. The selection itself is handled by platform code, so it's probably not something you want to start digging into :) In any case, if you do see that happen again, you should file a bug.

> For the next bug, I could find another mentored one, or you might like to
> point me at something urgent. I'll leave that up to you.

Feel free to explore more mentor bugs, it's good to work on small bugs in different parts of the code to get familiar with the project. There are also plenty more text selection bugs if you just want to look through the open bugs in the Text Selection component :)

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=prod%3Aandroid%20comp%3Atext&list_id=7506552

Comment 15

[:Margaret Leibovic]

I'm adding the checkin-needed keyword to this bug, which means someone should come along and land it for you in the next day or so.

Great work!

Comment 16

[Mark Capella [:capella]]

In progress...

Comment 17

[Mark Capella [:capella]]

ftw... :-D

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

Comment 18

[Adrian May [:adrianmay]]

Cool! Thanks for all the support. I've got a couple open for the gfx team right now so I'd best not overcommit, but I'll probably pick up another of these in a few days.

:-)

Comment 19

[Ed Morley [:emorley]]

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