697549 - Use NativeWindow.doorhanger in ContentPermissionPrompt.js

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[[:fabrice] Fabrice Desré]

See https://bugzilla.mozilla.org/show_bug.cgi?id=696520#c7 for background

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Share doorhanger code

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Attachment: 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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 5

[[:fabrice] Fabrice Desré]

(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?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Share doorhanger code

Implemented review comments.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. v2 Don't let loop runtime increase with app runtime

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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) {

Comment 9

[Gian-Carlo Pascutto [:gcp]]

http://hg.mozilla.org/projects/birch/rev/a0e94d288ba0
http://hg.mozilla.org/projects/birch/rev/e532cb6dd56e

Comment 10

[Aaron Train [:aaronmt]]

20111102040257
http://hg.mozilla.org/projects/birch/rev/2b9de9749c1e
Samsung Nexus S (Android 2.3.6)

Comment 11

[Gian-Carlo Pascutto [:gcp]]

>          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.