Closed Bug 1160352 Opened 9 years ago Closed 9 years ago

Enhance SelectionHandler lazy load

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file, 1 obsolete file)

Update browser.js to do a better job of lazy loading SelectionHandler, as identified during GeckoCarets implementation.

This should provide a start-up time boost for the duration.
Attached patch bug1160352.diff (obsolete) — Splinter Review
Is there a better/built-in way to check if the lazy load script has/has not yet been loaded?
Attachment #8600140 - Flags: review?(margaret.leibovic)
Comment on attachment 8600140 [details] [diff] [review]
bug1160352.diff

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

Let's not do this boolean flag to keep track of whether or not the script has been loaded. That code doesn't get called at startup, so we shouldn't worry about it, but if we do want to try to fix that, we should do it a different way.

::: mobile/android/chrome/content/SelectionHandler.js
@@ -615,5 @@
> -  removeAction: function(id) {
> -    // Update actions list and actionbar UI if active.
> -    delete this.actions[id];
> -    this._updateMenu();
> -  },

I think we should keep these as add-on APIs.

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/SelectionHandler

::: mobile/android/chrome/content/browser.js
@@ +5148,5 @@
> +
> +          // Possibly required after SelectionHandler lazy-loads.
> +          if (SelectionHandler_loaded) {
> +            SelectionHandler.subdocumentScrolled(this._scrollableElement);
> +          }

I don't think we should do this this way...

If we want to try to improve this logic, I think we should just decouple things to avoid calling SelectionHandler directly. Can we try replacing this with an observer service notification?
Attachment #8600140 - Flags: review?(margaret.leibovic) → review-
Attached patch bug1160352.diffSplinter Review
Short and simple then !
Attachment #8600140 - Attachment is obsolete: true
Attachment #8600261 - Flags: review?(margaret.leibovic)
Comment on attachment 8600261 [details] [diff] [review]
bug1160352.diff

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

::: mobile/android/chrome/content/browser.js
@@ -7069,5 @@
> -        // POST      *                                   YES
> -        // POST      application/x-www-form-urlencoded   YES
> -        // POST      text/plain                          NO ( a little tricky to do)
> -        // POST      multipart/form-data                 NO
> -        // POST      everything else                     YES

Please keep this comment.
Attachment #8600261 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/18347201f035
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.