1409054 - Properly get rid of old about:home

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P3)

Description

[Ursula Sarracini (:ursula)]

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.

Comment 2

[Tim Nguyen :ntim]

Is this actionable? If so, it would be nice to make this a good-first-bug.

Comment 3

[Johann Hofmann [:johannh]]

(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 :)

Comment 4

[Ursula Sarracini (:ursula)]

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...

Comment 5

[Johann Hofmann [:johannh]]

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?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(Im working on this)

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1409054 - Remove code from the old about:home.

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

Comment 8

[Ed Lee :Mardak]

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?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Ah right, forgot to actually remove it

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8979306 [details]
Bug 1409054 - Remove code from the old about:home.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245496/diff/1-2/

Comment 11

[Ed Lee :Mardak]

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

Comment 12

[Francesco Lodolo [:flod]]

(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

Comment 13

[Ed Lee :Mardak]

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.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 15

[:Felipe Gomes (needinfo for replies!)]

or, I just saw that you assigned yourself to bug 1396839, so I can wait for that to land and fully remove this file.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

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 [e-mail] 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 [1] and [2]

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 [3]

So, I could:

A - just delete the [e-mail] 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?

[1] https://searchfox.org/mozilla-central/source/browser/branding/nightly/content/aboutDialog.css
[2] https://searchfox.org/mozilla-central/source/browser/branding/unofficial/content/aboutDialog.css
[3] https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/extensions/activity-stream/css/activity-stream-linux.css#963

Comment 17

[Ed Lee :Mardak]

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 ;)

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8979306 [details]
Bug 1409054 - Remove code from the old about:home.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245496/diff/2-3/

Comment 19

[Ed Lee :Mardak]

Attachment: about:rights snippet in 59 with activity stream disabled

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…

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(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 21

[Ed Lee :Mardak]

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.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8979306 [details]
Bug 1409054 - Remove code from the old about:home.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245496/diff/3-4/

Comment 23

[:Felipe Gomes (needinfo for replies!)]

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/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81367b23ee5f
Remove code from the old about:home. r=Mardak

Comment 25

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/81367b23ee5f