Remove notification-inner which is now unused in Firefox itself

RESOLVED FIXED in Firefox 59

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: Gijs, Assigned: dao)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
See bug 1162635 comment 9.

We'd need to doublecheck, but at least within default themes it seems to now serve no purpose.

Then there's these:

https://mxr.mozilla.org/addons/search?string=notification-inner&find=css

that might need to update their CSS. A bunch of add-ons from Axel Grude, and then these:

I couldn't find authors for:

https://addons.mozilla.org/en-US/firefox/addon/yandexbar/

https://addons.mozilla.org/en-US/mobile/addon/classic-grey-theme/ (fennec 1-4, so I think that no longer works at all anyway)

https://addons.mozilla.org/en-US/seamonkey/addon/gnomerunnergtk-for-seamonkey-2/ (seamonkey 2-2.1, so ditto)

https://mxr.mozilla.org/addons/source/622954/skin/extensions-gnome.css#11 <-- dunno what this is, can't find it on AMO.

https://mxr.mozilla.org/addons/source/622956/skin/extensions-gnome.css#11 ditto

(I left out a number that just reset borders which would simply have no effect, rather than break because an element was now missing)


Then there are a few hits for XML:

https://mxr.mozilla.org/addons/search?string=notification-inner&find=xml

https://addons.mozilla.org/en-US/firefox/addon/socialite-fixed/ and the older version (couldn't find the newer version's author, assuming the old one is defunct)

https://addons.mozilla.org/en-GB/seamonkey/addon/xsidebar/ -- done by Philip Chee

https://addons.mozilla.org/en-US/firefox/addon/mozilla-labs-snowl/ dead, AFAICT

https://addons.mozilla.org/en-US/firefox/addon/geolocater/

https://addons.mozilla.org/en-US/firefox/addon/foxage2ch/ (authors for this and the previous add-on do both seem to have bugzilla accounts)

(not all of these would be necessarily affected by us removing the notification-inner thing from the internal code, but you could potentially be affected by the style changes in bug 1162635 as well, and it'd be good to get the third-party binding copies in line with what we do here, if anything.)


... and apparently this will even bite a few JS usecases

https://mxr.mozilla.org/addons/source/722/chrome/content/noscript/noscriptOverlay.js#1630 

https://mxr.mozilla.org/addons/source/249344/chrome/content/bmsnote.js#755 which is
https://addons.mozilla.org/en-US/firefox/addon/book-discovery/ (author does seem to have a bugzilla account)

https://mxr.mozilla.org/addons/search?string=notification-inner&find=\.js lists 2 other hits which aren't public on AMO.


This was kind of more than I expected, but I still think we can basically do this, but we should let it ride the trains normally. That'll give everybody at least 12 weeks to update their code. Dão, does that sound OK? I can write and test a patch, do a trypush to be safe, and CC all the authors for headsup. Unless you think it's not worth it considering the above?
Flags: needinfo?(dao)
(Assignee)

Comment 1

3 years ago
The CSS hits seem trivial enough that I wouldn't worry about them too much. The ones setting a binding for notification-inner are interesting. That's a pretty wild hack that we never supported explicitly.

The XML hits don't seem directly relevant for whether we keep notification-inner in our binding, as you noted.

> ... and apparently this will even bite a few JS usecases
> 
> https://mxr.mozilla.org/addons/source/722/chrome/content/noscript/
> noscriptOverlay.js#1630 

This is already broken with the "outset" class removal.

> https://mxr.mozilla.org/addons/source/249344/chrome/content/bmsnote.js#755
> which is
> https://addons.mozilla.org/en-US/firefox/addon/book-discovery/ (author does
> seem to have a bugzilla account)

Last updated 2011? Does this still work? Well, it only has 141 users anyway.
Flags: needinfo?(dao)
(Assignee)

Comment 2

3 years ago
Affected files:

http://mxr.mozilla.org/mozilla-central/search?string=notification-inner
QA Contact: dao
Summary: Consider removing notification-inner which is now unused in Firefox itself → Remove notification-inner which is now unused in Firefox itself
Whiteboard: [good first bug][lang=xul]
(Assignee)

Comment 3

10 months ago
(In reply to Dão Gottwald [::dao] from comment #2)
> Affected files:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=notification-inner

https://dxr.mozilla.org/mozilla-central/search?q=notification-inner
Blocks: 1387013
Mentor: dao+bmo
Keywords: addon-compat → good-first-bug
QA Contact: dao+bmo
Hello,

I would like to work on this bug. Is it still available? Might I have a little assistance with getting started?
(Assignee)

Comment 5

10 months ago
(In reply to Joshua Smith [:joshua-s] from comment #4)
> Hello,
> 
> I would like to work on this bug. Is it still available?

Yep.

> Might I have a little assistance with getting started?

Do you have the source code? Have you built Firefox yet? Let me know where in the process you currently are so I can help you effectively :)
Dao:

Thanks for responding and I apologize for the delay.

I have built Firefox from mozilla-central before. I need to finish setting it up on my current workstation, but that part of the process is good.
(Assignee)

Updated

8 months ago
Assignee: nobody → dao+bmo
Mentor: dao+bmo
Keywords: good-first-bug
Whiteboard: [good first bug][lang=xul]
Comment hidden (mozreview-request)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8927308 [details]
Bug 1215335 - Remove unused notification-inner container.

https://reviewboard.mozilla.org/r/198632/#review203794

Also checked to make sure htere wasn't any css that selected on the inherited [type] attribute and couldn't find anything
Attachment #8927308 - Flags: review?(bgrinstead) → review+

Comment 9

8 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2302ac35732f
Remove unused notification-inner container. r=bgrins
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Flags: needinfo?(dao+bmo)

Comment 12

8 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49d46f8e5f47
Remove unused notification-inner container. r=bgrins
Backed out for failing browser-chrome's browser/modules/test/browser/browser_UnsubmittedCrashHandler.js:

https://hg.mozilla.org/integration/autoland/rev/f872d28315f03697c0742db23a8ad8cc8484900b

Push with failures, only care about the bc ones: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=49d46f8e5f474fa46cf726ab9bcc6d5c0812ce41&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=144605634&repo=autoland

[task 2017-11-14T16:33:00.459Z] 16:33:00     INFO - Entering test bound test_other_ignored
[task 2017-11-14T16:33:00.463Z] 16:33:00     INFO - TEST-PASS | browser/modules/test/browser/browser_UnsubmittedCrashHandler.js | There should be a notification - {"eventCallback":"(eventType) => {\n      if (eventType == \"dismissed\") {\n        // The user intentionally dismissed the not == true - 
[task 2017-11-14T16:33:00.467Z] 16:33:00     INFO - Buffered messages finished
[task 2017-11-14T16:33:00.472Z] 16:33:00     INFO - TEST-UNEXPECTED-FAIL | browser/modules/test/browser/browser_UnsubmittedCrashHandler.js | Uncaught exception - at chrome://mochitests/content/browser/browser/modules/test/browser/browser_UnsubmittedCrashHandler.js:289 - TypeError: closeButton is null
[task 2017-11-14T16:33:00.476Z] 16:33:00     INFO - Stack trace:
[task 2017-11-14T16:33:00.477Z] 16:33:00     INFO -     test_other_ignored@chrome://mochitests/content/browser/browser/modules/test/browser/browser_UnsubmittedCrashHandler.js:289:3
[task 2017-11-14T16:33:00.478Z] 16:33:00     INFO -     Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1065:21
[task 2017-11-14T16:33:00.478Z] 16:33:00     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:1056:9
[task 2017-11-14T16:33:00.480Z] 16:33:00     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:956:9
[task 2017-11-14T16:33:00.481Z] 16:33:00     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-11-14T16:33:00.483Z] 16:33:00     INFO - Leaving test bound test_other_ignored
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

8 months ago
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)

Comment 15

8 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52eeb888317f
Remove unused notification-inner container. r=bgrins

Comment 16

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52eeb888317f
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.