Closed Bug 994664 Opened 10 years ago Closed 10 years ago

Improve SelectionHandler UI performance

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file)

This is an improved version of the patch originally provided as a solution to bug 988471, which was solved by a different patch.

This works hand-in-hand to improve handle/caret dragging performance, by avoiding sending actionbar update messages "TextSelection:Update" after every "TextSelection:PositionHandles" message (which in many situations are redundant), limiting to only when logic / state-change dictates.

(Note minor correction over previous version to accommodate the new phone# logic.)
Attachment #8404694 - Flags: review?(wjohnston)
Comment on attachment 8404694 [details] [diff] [review]
bugSelectionHandler.diff (v0)

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +451,5 @@
> +      throw "Action with id " + action.id + " already added";
> +
> +    // Update actions list and actionbar UI if active.
> +    this.actions[action.id] = action;
> +    this._updateMenu();

Good fixes :) thanks

@@ +1060,5 @@
>      });
> +
> +    // Text state transitions (text <--> no text) can affect selection context and actionbar display
> +    let currTargetElementHasText = (this._targetElement.textLength > 0);
> +    if (currTargetElementHasText != this._prevTargetElementHasText) {

This part scares me a little bit. I think I want to not do this at all... but I need to think about it. I had some WIP tests for this that I'd also like to get up to date and landed before we land this so we at least have SOME coverage. Do you want to grab those and finish them (otherwise, I'll bump them up on my priority list).
If you mean bug 980074, then I can finish it first ... I want us to have it, I have the time, and I re-wrote it once once after you first published so I could create a test to capture / prove bug 982608.

(This patch can wait a little longer)

Assign it to me if that's what you meant, and ignore my tail-wagging-eagerness if it's not :-D
Sent to you :)
I review-posted a tweaked out version of the testSelectionHandler patch (bug 980074), and I'm re-pinging on this one now.

If we can complete this one, I can also push bug 895463 (which is approved, but dependent on this patch).
Comment on attachment 8404694 [details] [diff] [review]
bugSelectionHandler.diff (v0)

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

OK. Lets try :)
Attachment #8404694 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/cf95a5315f6f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.