853691 - Some SelectionHandler cleanup

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Attachment: Only set _activeType = TYPE_NONE in one place

Doing some housekeeping before major surgery!

In this first patch, we really only need to be setting _activeType = TYPE_NONE in one place. The assignment in endSelection() looks like it was just a hack to bail out when ending up in a recursive loop from notififySelectionChanged. Removing the listener earlier fixes this issue. And the second assignment in hideThumb is unnecessary because we always call _cleanUp() right after.

Comment 1

[:Margaret Leibovic]

Attachment: Get rid of hideThumb

We never want to keep certain handles visible, so sending specific handles along with the "TextSelection:HideHandles" message feels like unnecessary overhead. And this lets our _cleanUp() function be _activeType-agnostic.

Comment 2

[:Margaret Leibovic]

Attachment: Reorganize SelectionHandler cleanup code paths

I decided to fold these patches together because when I wrote a third patch, it just changed what I had put in these first two patches :)

In most of the places we call endSelection() right now, we really just want to end the selection. Factoring out the copy/share code paths makes this more straightforward. Since what's left after that is basically _cleanUp, we can just have one method to handle shutting down SelectionHandler. I decided to name this _closeSelection, since that's consistent with the name for this function in metro's SelectionHandler, but I'm open to changing that (_cleanUp feels kind of like the wrong name now that this also clears the selection).

Comment 3

[:Margaret Leibovic]

Comment on attachment 729121 [details] [diff] [review]
Reorganize SelectionHandler cleanup code paths

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

Some review comments for myself...

::: mobile/android/chrome/content/SelectionHandler.js
@@ +78,5 @@
>          }
>          break;
>        }
>        case "Tab:Selected":
>          if (this._activeType == this.TYPE_CURSOR) {

This check seems unnecessary, since we fall through for the TYPE_SELECTION case. We could just call _closeSelection() here then break.

@@ +149,3 @@
>        case "compositionend":
>          // If the handles are displayed during user input, hide them.
>          if (this._activeType == this.TYPE_CURSOR) {

And this check is redundant, since we only add these listeners in showThumb().

Comment 4

[:Margaret Leibovic]

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

> @@ +149,3 @@
> >        case "compositionend":
> >          // If the handles are displayed during user input, hide them.
> >          if (this._activeType == this.TYPE_CURSOR) {
> 
> And this check is redundant, since we only add these listeners in
> showThumb().

Nevermind, this was wrong.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 729121 [details] [diff] [review]
Reorganize SelectionHandler cleanup code paths

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +84,3 @@
>          }
>          // fall through
>        case "Window:Resize": {

Yeah, since Tab:Selected and Window:Resize do the same thing, I would just have Tab:Selected fall through and remove the selection type check from Window:Resize.

Comment 6

[:Margaret Leibovic]

Comment on attachment 729121 [details] [diff] [review]
Reorganize SelectionHandler cleanup code paths

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +399,2 @@
>  
>      this._activeType = this.TYPE_NONE;

I also just found a problem here. I shouldn't be setting _activeType right before checking if it's TYPE_SELECTION, so I need to move this below the if statement.

Comment 7

[:Margaret Leibovic]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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