1462281 - Remove obsolete getNotificationBox function form browser.js

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Dão Gottwald [:dao]]

This function isn't e10s-ready (it takes a content window in a chrome context) and appears to be used only in test_privbrowsing_perwindowpb.html (via notification_common.js) which has been dead for a few years (bug 1173337).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c1dd46d41fc1142a65f1dbfba064fb88c33e4cb

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

Review commit: https://reviewboard.mozilla.org/r/244604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244604/

Comment 2

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8976467 [details]
Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

https://reviewboard.mozilla.org/r/244604/#review250946

Hmm, it looks like it's used right here? https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/mobile/android/components/BlocklistPrompt.js#24

I'm not familiar enough with that to know at a glance if it's dead or not, but it looks like it's referenced from a few places.

Comment 3

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8976467 [details]
Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

https://reviewboard.mozilla.org/r/244604/#review250948

Comment 4

[Dão Gottwald [:dao]]

(In reply to Doug Thayer [:dthayer] (PTO on May 17) from comment #2)
> Comment on attachment 8976467 [details]
> Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.
> 
> https://reviewboard.mozilla.org/r/244604/#review250946
> 
> Hmm, it looks like it's used right here?
> https://searchfox.org/mozilla-central/rev/
> da499aac682d0bbda5829327b60a865cbc491611/mobile/android/components/
> BlocklistPrompt.js#24
> 
> I'm not familiar enough with that to know at a glance if it's dead or not,
> but it looks like it's referenced from a few places.

That's Firefox for Android, completely separate from Firefox desktop's browser.js.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8976467 [details]
Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

https://reviewboard.mozilla.org/r/244604/#review250970

https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#1480 is in toolkit though...

FWIW, I also don't see a definition for the mobile version. I bet the blocklist prompt is broken on Android because I imagine there's no tests. :-\

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #5)
> FWIW, I also don't see a definition for the mobile version. I bet the
> blocklist prompt is broken on Android because I imagine there's no tests. :-\

Nick, can you check if I'm missing something, and if not file a mobile bug? It feels like maybe on mobile this is java-provided or something, but I don't see it...

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8976467 [details]
Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

(In reply to :Gijs (he/him) from comment #5)
> Comment on attachment 8976467 [details]
> Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.
> 
> https://reviewboard.mozilla.org/r/244604/#review250970
> 
> https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/
> nsLoginManagerPrompter.js#1480 is in toolkit though...

Still not needed in desktop Firefox because we implement PopupNotifications. _getNotifyBox is only the legacy fallback for _getPopupNote.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8976467 [details]
Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

https://reviewboard.mozilla.org/r/244604/#review250972

Please leave a note on the disabled test bug to indicate this bit will need to be fixed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df66a68e78da
Remove obsolete getNotificationBox function form browser.js. r=Gijs

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to :Gijs (he/him) from comment #6)
> (In reply to :Gijs (he/him) from comment #5)
> > FWIW, I also don't see a definition for the mobile version. I bet the
> > blocklist prompt is broken on Android because I imagine there's no tests. :-\
> 
> Nick, can you check if I'm missing something, and if not file a mobile bug?
> It feels like maybe on mobile this is java-provided or something, but I
> don't see it...

Yeah, this looks busted: https://bugzilla.mozilla.org/show_bug.cgi?id=1462796.  I wonder what getNotificationBox in devtools hopes to achieve...

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/df66a68e78da

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #10)
> (In reply to :Gijs (he/him) from comment #6)
> > (In reply to :Gijs (he/him) from comment #5)
> > > FWIW, I also don't see a definition for the mobile version. I bet the
> > > blocklist prompt is broken on Android because I imagine there's no tests. :-\
> > 
> > Nick, can you check if I'm missing something, and if not file a mobile bug?
> > It feels like maybe on mobile this is java-provided or something, but I
> > don't see it...
> 
> Yeah, this looks busted:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1462796.  I wonder what
> getNotificationBox in devtools hopes to achieve...

Looks like this is obsolete and will be addressed by the Add-ons/Web Extensions team; clearing my NI here.