Closed Bug 1409054 Opened 2 years ago Closed 2 years ago
Properly get rid of old about:home
59 bytes, text/x-review-board-request
114.41 KB, image/png
Bug 1400326 made some changes to about:home without Activity Stream, but in an effort to keep the patch small so we could uplift to Beta 57, we just hid the unused elements instead of removing them. We should properly remove the unused elements from the launcher, and the messaging code/tests that go with it.
Summary: Properly get rid of unused about:home functionality → Properly get rid of old about:home
Is this actionable? If so, it would be nice to make this a good-first-bug.
(In reply to Tim Nguyen :ntim from comment #2) > Is this actionable? If so, it would be nice to make this a good-first-bug. I don't think this is a good first bug, there might be tons of complications like unexpected breakage or regressions. I'm all for unshipping, but I think someone with a little investment and responsibility in the project should handle it :)
I agree with Johann about not being good first bug. There's some logic in AboutRedirector.cpp that involves redirecting about:home, as well as removing unused files, making sure the disabled tests are removed etc...
I wonder if it even makes sense to remove the AboutRedirector parts at all. It seems reasonable to keep around the old about:home URL people are used to and it sounds like we'd have less trouble dealing with things like the home button etc... OTOH I'm sure there are also a lot of extra hacks around having multiple entry points to AS/about:newtab that we might want to get rid of. Does it make sense to remove the browser/base/content/abouthome parts first and have a separate discussion about keeping the about:home URL separately?
(Im working on this)
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Version: 57 Branch → Trunk
Comment on attachment 8979306 [details] Bug 1409054 - Remove code from the old about:home. https://reviewboard.mozilla.org/r/245496/#review251508 ::: browser/modules/moz.build (Diff revision 1) > 'test/browser/formValidation/browser.ini', > ] > XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini'] > > EXTRA_JS_MODULES += [ > - 'AboutHome.jsm', Still looking, but quickly, AboutHome.jsm file should be removable now and not just un-packaged?
Ah right, forgot to actually remove it
flod, any suggestions on where to move abouthome.pageTitle? Looks like it's specially used for the home button to show "Firefox Nightly Start Page": https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/base/content/browser.xul#771 So we can't just directly remove aboutHome.dtd. I'm checking with design if the button should just say "Firefox Home" to match about:preferences, although that string only exists in preferences.ftl, so we'll probably need to find a different file to stash a new "Firefox Home" anyway.. browser.properties ? felipe, this ^^ seems a bit much for this bug, as it might involve changing aboutHomeOverrideTooltip behavior to get a properties string instead of using the dtd entity, so we can split out that work to a separate bug. Not sure if it should block this bug though, so this bug can just cleanly delete the aboutHome.dtd
(In reply to Ed Lee :Mardak from comment #11) > flod, any suggestions on where to move abouthome.pageTitle? Looks like it's > specially used for the home button to show "Firefox Nightly Start Page": > > https://searchfox.org/mozilla-central/rev/ > 8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/base/content/browser.xul#771 > > So we can't just directly remove aboutHome.dtd. > > I'm checking with design if the button should just say "Firefox Home" to > match about:preferences, although that string only exists in > preferences.ftl, so we'll probably need to find a different file to stash a > new "Firefox Home" anyway.. browser.properties ? browser.dtd (or browser.properties) are probably a good place for that kind of string. Having said that, I'm looking at the patch and I don't see any removal of strings? There is also aboutHome.properties
Indeed, the strings aren't removed in this patch, but most could be removed from aboutHome.dtd if not for bug 1396839. aboutHome.properties seems to be for android.
Ah, it wasn't(In reply to Ed Lee :Mardak from comment #11) > felipe, this ^^ seems a bit much for this bug, as it might involve changing > aboutHomeOverrideTooltip behavior to get a properties string instead of > using the dtd entity, so we can split out that work to a separate bug. Not > sure if it should block this bug though, so this bug can just cleanly delete > the aboutHome.dtd Ah I had forgotten about those strings. I'll remove the unused strings, but still leave aboutHome.dtd in place with abouthome.pageTitle. Then I'll file a separate bug to move that string to a new place and finally delete the file. What do you think?
or, I just saw that you assigned yourself to bug 1396839, so I can wait for that to land and fully remove this file.
There's one other problem that showed up on a push to try which I'm not sure yet what to do about it: The test browser_all_files_referenced.js complains that the file firstname.lastname@example.org becomes unreferenced. All the branding flavors of the About Dialog pages uses that file, except the "unofficial" one, which is used on try or local builds. Compare  and  Now, there's the about-logo.png (the "1x" version), but the test doesn't complain about that one because it is used in Activity Stream  So, I could: A - just delete the email@example.com file from the unbranded folder B - add an exception for it in browser_all_files_referenced.js C - figure out how to make AS use that file Since this is not packaged in official builds anyway (this is just a prob in unofficial builds), I'm leaning towards B. What do you think?  https://searchfox.org/mozilla-central/source/browser/branding/nightly/content/aboutDialog.css  https://searchfox.org/mozilla-central/source/browser/branding/unofficial/content/aboutDialog.css  https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/extensions/activity-stream/css/activity-stream-linux.css#963
Bug 1448918 only just recently added the reference to chrome://branding/content/about-logo.png https://github.com/mozilla/activity-stream/pull/4090#issuecomment-381240397 I guess we don't need the 2x version for its usage for about:welcome as it's used as 90x90 where it's normally 192 and 384 for 2x. There's also chrome://branding/content/icon64.png but I guess that's too small for about:welcome… ? I think we should just delete unbranded about-logo@2x. If someone wants it, it can be restored ;)
So… the removing `pref("browser.rights.version", 3)` and related got me digging into where they came from, and looks like about:rights message moved from a notification bar to a snippet in bug 819493. I've attached a screenshot of what it looks like in Firefox 59 when one could still disable activity stream to get old about:home. mpopova, should we be showing a "rights" snippet on first run? Apparently we haven't been showing it to new users since activity stream became the default in 57… felipe, I suppose this particular decision / bug relating to about:rights shouldn't block the landing of this change…
(In reply to Ed Lee :Mardak from comment #19) > felipe, I suppose this particular decision / bug relating to about:rights > shouldn't block the landing of this change… Yeah, I don't think it blocks this, since it's being unused right now. I guess I could avoid removing some of these prefs? Or they can be re-added depending on the decision from bug 1463277. Up to you on how you think it's best for me to proceed here
Comment on attachment 8979306 [details] Bug 1409054 - Remove code from the old about:home. https://reviewboard.mozilla.org/r/245496/#review251620 Deleting ~200KB (images) and 1.6k lines and none added. :) There's some more lines to remove, and this should be good to go! ::: browser/base/content/tab-content.js (Diff revision 3) > - addMessageListener("AboutHome:Update", this); > - addEventListener("click", this, true); > - addEventListener("pagehide", this, true); > - > - sendAsyncMessage("AboutHome:MaybeShowMigrateMessage"); > - sendAsyncMessage("AboutHome:RequestUpdate"); nsBrowserGlue won't need to listen for these 2 messages anymore ::: browser/base/content/test/about/browser.ini:13 (Diff revision 3) > [browser_aboutCertError.js] > support-files = > dummy_page.html > -[browser_aboutHome_imitate.js] > [browser_aboutHome_input.js] > skip-if = true # Bug 1409054 to remove; previously skipped for intermittents, e.g., Bug 1399648 Looks like this test can be removed now.. it was testing the various buttons on the page, which you were initially trying to get rid of! :D ::: browser/base/jar.mn (Diff revision 3) > content/browser/aboutRestartRequired.js (content/aboutRestartRequired.js) > content/browser/aboutRestartRequired.xhtml (content/aboutRestartRequired.xhtml) > content/browser/aboutRobots.xhtml (content/aboutRobots.xhtml) > content/browser/aboutRobots.js (content/aboutRobots.js) > content/browser/aboutRobots.css (content/aboutRobots.css) > -* content/browser/abouthome/aboutHome.xhtml (content/abouthome/aboutHome.xhtml) There's a comment in aboutNewTabService.js that was keeping this file referenced, so the comment can be removed now. ::: browser/components/nsBrowserGlue.js:148 (Diff revision 3) > > /* global AboutHome:false, ContentPrefServiceParent:false, ContentSearch:false, > UpdateListener:false, webrtcUI:false */ > > /** > * IF YOU ADD OR REMOVE FROM THIS LIST, PLEASE UPDATE THE LIST ABOVE AS WELL. There's this big scary warning ;) I think it's just removing the "global" eslint comment.
Attachment #8979306 - Flags: review?(edilee) → review+
Thanks for the thorough review to catch these things I had missed. I updated the patch with these final changes and will give it a final try-server push before landing (In reply to Ed Lee :Mardak from comment #21) > Deleting ~200KB (images) and 1.6k lines and none added. :) There's some more > lines to remove, and this should be good to go! \o/
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/81367b23ee5f Remove code from the old about:home. r=Mardak
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.