Support doorhangers for popup blocking

VERIFIED FIXED

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

unspecified
ARM
Android
Points:
---
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Modify the popup blockers to use the new Java doorhangers.
(Assignee)

Updated

6 years ago
Assignee: nobody → gpascutto

Updated

6 years ago
Priority: -- → P2
(Assignee)

Comment 1

6 years ago
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.
Attachment #571749 - Flags: review?(mark.finkle)
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)
Attachment #571749 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 3

6 years ago
Pushed with comments addressed.

http://hg.mozilla.org/projects/birch/rev/f3eea1384f14
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 699772
bug 695448 also affected with these changes
20111104040406
http://hg.mozilla.org/projects/birch/rev/f3eea1384f14
Samsung Galaxy SII
Status: RESOLVED → VERIFIED
and by comment #4, I meant bug 698845
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
Flags: in-litmus?(fennec)
Whiteboard: [QA+]

Comment 10

6 years ago
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
Flags: in-litmus?(fennec) → in-litmus+
Whiteboard: [QA+]
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.