Closed
Bug 994664
Opened 10 years ago
Closed 10 years ago
Improve SelectionHandler UI performance
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: capella, Assigned: capella)
Details
Attachments
(1 file)
9.36 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
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 1•10 years ago
|
||
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).
Assignee | ||
Comment 2•10 years ago
|
||
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
Comment 3•10 years ago
|
||
Sent to you :)
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fd0c925fe29a
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cf95a5315f6f
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cf95a5315f6f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•