697110 - Support doorhangers for popup blocking

Status: VERIFIED FIXED

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

Description

[Gian-Carlo Pascutto [:gcp]]

Modify the popup blockers to use the new Java doorhangers.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Add the PopupBlockerObserver service and use NativeWindow.doorhanger

Includes a fix to actually delete the callback objects in NativeWindow.

Comment 2

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

Comment on attachment 571749 [details] [diff] [review]
Patch 1. Add the PopupBlockerObserver service and use NativeWindow.doorhanger


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

>       let id = aData;
>       if (this.doorhanger._callbacks[id]) {
>         let prompt = this.doorhanger._callbacks[id].prompt;
>         this.doorhanger._callbacks[id].cb();
>-        for (let callback in this.doorhanger._callbacks) {
>-          if (callback.prompt == prompt) {
>-            delete callback;
>+        for (let id in this.doorhanger._callbacks) {
>+          if (this.doorhanger._callbacks[id].prompt == prompt) {
>+            delete this.doorhanger._callbacks[id];

Only a mild worry about using two variables named 'id'. You could use for each and keep 'callback'

>+    BrowserApp.deck.addEventListener("DOMUpdatePageReport",
>+                                     PopupBlockerObserver.onUpdatePageReport,
>+                                     false);

No wrap needed. Honest.


>+var PopupBlockerObserver = {
>+  onUpdatePageReport: function onUpdatePageReport(aEvent)
>+  {

nit: { on same line as 'function'

>+    var cBrowser = BrowserApp.selectedBrowser;

nit: can we switch to 'let' and 'browser'

>+
>+      if (Services.prefs.getBoolPref("privacy.popups.showBrowserMessage")) {
>+        var brandShortName = Strings.brand.GetStringFromName("brandShortName");
>+        var message;
>+        var popupCount = cBrowser.pageReport.length;

nit: use 'let'

>+        var buttons = [

nit: use 'let'

>+            accessKey: null,
>+            accessKey: null,
>+            accessKey: null,

Just remove these. We should be fine without them.

>+  allowPopupsForSite: function allowPopupsForSite(aAllow) {
>+    var currentURI = BrowserApp.selectedBrowser.currentURI;

nit: Use 'let'

>+  showPopupsForSite: function showPopupsForSite() {

>+        let newTab = BrowserApp.addTab(popupURIspec);
>+        newTab.active = true;
>+        BrowserApp.selectTab(newTab);

In XUL fennec we do not activate/select the new tab. This should be enough:

          BrowserApp.addTab(popupURIspec);

r+ with nits and removing the active/selectTab

(I know the 'let' things weren't yours. I should have changed those a long time ago)

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Pushed with comments addressed.

http://hg.mozilla.org/projects/birch/rev/f3eea1384f14

Comment 4

[Aaron Train [:aaronmt]]

bug 695448 also affected with these changes

Comment 5

[Aaron Train [:aaronmt]]

20111104040406
http://hg.mozilla.org/projects/birch/rev/f3eea1384f14
Samsung Galaxy SII

Comment 6

[Aaron Train [:aaronmt]]

and by comment #4, I meant bug 698845

Comment 7

[Wesley Johnston (:wesj)]

These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Comment 9

[Aaron Train [:aaronmt]]

20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)

Comment 10

[Camelia Urian]

Test cases updated: BFT - popup and annoyance blocking:

https://litmus.mozilla.org/show_test.cgi?id=33636
https://litmus.mozilla.org/show_test.cgi?id=33700