Use NativeWindow.doorhanger in ContentPermissionPrompt.js

VERIFIED FIXED

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
6 years ago
11 months ago

People

(Reporter: fabrice, Assigned: gcp)

Tracking

unspecified
All
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=696520#c7 for background
Assignee: nobody → fabrice
Priority: -- → P2
(Assignee)

Updated

6 years ago
Assignee: fabrice → gpascutto
(Assignee)

Comment 1

6 years ago
Created attachment 571027 [details] [diff] [review]
Patch 1. Share doorhanger code
Attachment #571027 - Flags: review?(mark.finkle)
(Assignee)

Comment 2

6 years ago
Created attachment 571043 [details] [diff] [review]
Patch 2. Don't let loop runtime increase with app runtime

The new code is a bit more compact but it introduces a loop whose runtime will increase with Fennec runtime. Unsure if it has much practical impact but this looks like the kind of thing that may be a pain to debug later down the road, so this changes it back to the original.
Attachment #571043 - Flags: review?(mark.finkle)
Comment on attachment 571027 [details] [diff] [review]
Patch 1. Share doorhanger code


>diff --git a/mobile/components/ContentPermissionPrompt.js b/mobile/components/ContentPermissionPrompt.js
>-  getTabForRequest: function getTabForRequest(request) {
>+  getChromeForRequest: function getBrowserForRequest(request) {

Change the function name too

    getChromeForRequest: function getChromeForRequest(request) {

>+  getTabForRequest: function getTabForRequest(request) {
>+    let chromeWin = this.getChromeForRequest(request);
>+    if (request.window) {
>       let windowID = chromeWin.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;

I don't think we use the windowID

>       let browser = chromeWin.BrowserApp.getBrowserForWindow(request.window);
>       let tabID = chromeWin.BrowserApp.getTabForBrowser(browser).id;
>       return tabID;
>     }
>-    let chromeWin = request.element.ownerDocument.defaultView;
>     let browser = chromeWin.Browser;
>     let tabID = chromeWin.BrowserApp.getTabForBrowser(browser).id;

browser is actually | request.element |

It's hard to test this code since we are not using e10s. In fact, | chromeWin.Browser | will fail since there is no "Browser" object in the chromeWin anymore. Maybe we should just remove this code and return null ?

At the very least use:

      let tabID = chromeWin.BrowserApp.getTabForBrowser(request.element).id;

r+, but fix up nits and the non-e10s part of getTabForRequest. Returning null seems fine to me, for now.
Attachment #571027 - Flags: review?(mark.finkle) → review+
Comment on attachment 571043 [details] [diff] [review]
Patch 2. Don't let loop runtime increase with app runtime

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

Hmm. I think we could keep the original code if we switched to use doorhanger.callbacks as an object instead of an array.

>   doorhanger: {
>     _callbacks: [],

_callbacks: {},

>   observe: function(aSubject, aTopic, aData) {

>       let id = parseInt(aData);

Let's stop doing this. Leave it as a string. That allows us to use "namespaced" callback IDs if needed.

>-      if (this.doorhanger._callbacks[id]) {
>-        let prompt = this.doorhanger._callbacks[id].prompt;
>-        this.doorhanger._callbacks[id].cb();

>-        for (let i = 0; i < this._callbacksId; i++) {
>-          if (this._callbacks[i] && this._callbacks[i].prompt == prompt)
>-            delete this._callbacks[i];

          for (callback in this.doorhanger.callbacks) {
            if (callback.prompt == prompt)
              delete callback;

This should do the same thing but use the object instead of an array. GCP, does it do what you wanted? Fabrice, does this make sense?
Attachment #571043 - Flags: review?(mark.finkle)
Attachment #571043 - Flags: review?(fabrice)
Attachment #571043 - Flags: review-
(Reporter)

Comment 5

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #4)
>           for (callback in this.doorhanger.callbacks) {
>             if (callback.prompt == prompt)
>               delete callback;
> 
> This should do the same thing but use the object instead of an array. GCP,
> does it do what you wanted? Fabrice, does this make sense?

 That loks ok to me. GCP, can you attach a new patch?
(Assignee)

Comment 6

6 years ago
Created attachment 571101 [details] [diff] [review]
Patch 1. v2 Share doorhanger code

Implemented review comments.
Attachment #571027 - Attachment is obsolete: true
Attachment #571101 - Flags: review+
(Assignee)

Comment 7

6 years ago
Created attachment 571102 [details] [diff] [review]
Patch 2. v2 Don't let loop runtime increase with app runtime
Attachment #571043 - Attachment is obsolete: true
Attachment #571043 - Flags: review?(fabrice)
Attachment #571102 - Flags: review?(mark.finkle)
Comment on attachment 571102 [details] [diff] [review]
Patch 2. v2 Don't let loop runtime increase with app runtime


>+        for (var callback in this.doorhanger._callbacks) {

nit:
          for (let callback in this.doorhanger._callbacks) {
Attachment #571102 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/projects/birch/rev/a0e94d288ba0
http://hg.mozilla.org/projects/birch/rev/e532cb6dd56e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
20111102040257
http://hg.mozilla.org/projects/birch/rev/2b9de9749c1e
Samsung Nexus S (Android 2.3.6)
Status: RESOLVED → VERIFIED
(Assignee)

Comment 11

6 years ago
>          for (callback in this.doorhanger.callbacks) {

That should have been a "for each" or written differently. Oops! 

We're leaking memory. Will fix in bug 697110.
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.