Last Comment Bug 697549 - Use NativeWindow.doorhanger in ContentPermissionPrompt.js
: Use NativeWindow.doorhanger in ContentPermissionPrompt.js
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P2 normal (vote)
: ---
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-26 13:30 PDT by [:fabrice] Fabrice Desré
Modified: 2016-07-29 14:20 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch 1. Share doorhanger code (6.19 KB, patch)
2011-11-01 09:12 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review+
Details | Diff | Splinter Review
Patch 2. Don't let loop runtime increase with app runtime (2.51 KB, patch)
2011-11-01 09:54 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review-
Details | Diff | Splinter Review
Patch 1. v2 Share doorhanger code (6.25 KB, patch)
2011-11-01 12:24 PDT, Gian-Carlo Pascutto [:gcp]
gpascutto: review+
Details | Diff | Splinter Review
Patch 2. v2 Don't let loop runtime increase with app runtime (1.94 KB, patch)
2011-11-01 12:24 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review+
Details | Diff | Splinter Review

Description [:fabrice] Fabrice Desré 2011-10-26 13:30:02 PDT
See https://bugzilla.mozilla.org/show_bug.cgi?id=696520#c7 for background
Comment 1 Gian-Carlo Pascutto [:gcp] 2011-11-01 09:12:15 PDT
Created attachment 571027 [details] [diff] [review]
Patch 1. Share doorhanger code
Comment 2 Gian-Carlo Pascutto [:gcp] 2011-11-01 09:54:39 PDT
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.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-01 11:06:35 PDT
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?) 2011-11-01 11:18:10 PDT
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é 2011-11-01 11:36:28 PDT
(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] 2011-11-01 12:24:02 PDT
Created attachment 571101 [details] [diff] [review]
Patch 1. v2 Share doorhanger code

Implemented review comments.
Comment 7 Gian-Carlo Pascutto [:gcp] 2011-11-01 12:24:43 PDT
Created attachment 571102 [details] [diff] [review]
Patch 2. v2 Don't let loop runtime increase with app runtime
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-01 13:01:53 PDT
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 10 Aaron Train [:aaronmt] 2011-11-02 07:10:50 PDT
20111102040257
http://hg.mozilla.org/projects/birch/rev/2b9de9749c1e
Samsung Nexus S (Android 2.3.6)
Comment 11 Gian-Carlo Pascutto [:gcp] 2011-11-03 12:25:56 PDT
>          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.

Note You need to log in before you can comment on or make changes to this bug.