Closed Bug 697110 Opened 13 years ago Closed 13 years ago

Support doorhangers for popup blocking

Categories

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

ARM
Android
defect

Tracking

(firefox11 fixed, fennec11+)

VERIFIED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: gcp, Assigned: gcp)

References

Details

Attachments

(1 file)

Modify the popup blockers to use the new Java doorhangers.
Assignee: nobody → gpascutto
Priority: -- → P2
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+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 699772
bug 695448 also affected with these changes
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 → ---
Status: REOPENED → RESOLVED
Closed: 13 years ago13 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+]
Flags: in-litmus?(fennec) → in-litmus+
Whiteboard: [QA+]
tracking-fennec: --- → 11+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: