Enhance SelectionHandler lazy load

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 40
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted 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-
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.