Closed Bug 1462281 Opened 4 years ago Closed 4 years ago

Remove obsolete getNotificationBox function form browser.js

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

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 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 on attachment 8976467 [details]
Bug 1462281 - Remove obsolete getNotificationBox function form browser.js.

https://reviewboard.mozilla.org/r/244604/#review250948
Attachment #8976467 - Flags: review?(dothayer)
(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.
Attachment #8976467 - Flags: review?(gijskruitbosch+bugs)
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. :-\
Attachment #8976467 - Flags: review?(gijskruitbosch+bugs)
(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...
Flags: needinfo?(nalexander)
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.
Attachment #8976467 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8976467 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df66a68e78da
Remove obsolete getNotificationBox function form browser.js. r=Gijs
(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...
https://hg.mozilla.org/mozilla-central/rev/df66a68e78da
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(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.
Flags: needinfo?(nalexander)
You need to log in before you can comment on or make changes to this bug.