Last Comment Bug 697110 - Support doorhangers for popup blocking
: Support doorhangers for popup blocking
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: ---
Assigned To: Gian-Carlo Pascutto [:gcp]
:
:
Mentors:
Depends on: 699772
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 07:53 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2016-07-29 14:20 PDT (History)
4 users (show)
camelia.urian: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch 1. Add the PopupBlockerObserver service and use NativeWindow.doorhanger (5.65 KB, patch)
2011-11-03 12:52 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review+
Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2011-10-25 07:53:36 PDT
Modify the popup blockers to use the new Java doorhangers.
Comment 1 Gian-Carlo Pascutto [:gcp] 2011-11-03 12:52:41 PDT
Created attachment 571749 [details] [diff] [review]
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?) 2011-11-03 13:13:08 PDT
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] 2011-11-04 00:12:56 PDT
Pushed with comments addressed.

http://hg.mozilla.org/projects/birch/rev/f3eea1384f14
Comment 4 Aaron Train [:aaronmt] 2011-11-04 06:37:33 PDT
bug 695448 also affected with these changes
Comment 5 Aaron Train [:aaronmt] 2011-11-04 06:39:51 PDT
20111104040406
http://hg.mozilla.org/projects/birch/rev/f3eea1384f14
Samsung Galaxy SII
Comment 6 Aaron Train [:aaronmt] 2011-11-04 06:40:27 PDT
and by comment #4, I meant bug 698845
Comment 7 Wesley Johnston (:wesj) 2011-11-10 10:26:40 PST
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?) 2011-11-11 08:46:04 PST
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Comment 9 Aaron Train [:aaronmt] 2011-11-14 06:45:48 PST
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Comment 10 Camelia Urian 2011-12-07 02:51:24 PST
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

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