Closed Bug 1409054 Opened 2 years ago Closed 2 years ago

Properly get rid of old about:home

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Iteration:
62.2 - Jun 4
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox62 --- fixed

People

(Reporter: ursula, Assigned: Felipe)

References

(Blocks 4 open bugs)

Details

(Keywords: polish, Whiteboard: [AS61MVP])

Attachments

(2 files)

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.
Severity: normal → enhancement
Keywords: polish
Priority: -- → P3
Blocks: 1433133
Iteration: --- → 60.4 - Mar 12
Priority: P3 → P2
Whiteboard: [AS60MVP]
Depends on: 1433324
Summary: Properly get rid of unused about:home functionality → Properly get rid of old about:home
Blocks: 1356461
Blocks: 1437659
Iteration: 60.4 - Mar 12 → ---
Priority: P2 → P3
Iteration: --- → 61.1 - Mar 26
Whiteboard: [AS60MVP] → [AS61MVP]
Iteration: 61.1 - Mar 26 → ---
Iteration: --- → 61.2 - Apr 9
Priority: P3 → P2
Iteration: 61.2 - Apr 9 → 61.4 - May 7
Duplicate of this bug: 1450570
Blocks: png-cleanup
Is this actionable? If so, it would be nice to make this a good-first-bug.
Flags: needinfo?(usarracini)
(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...
Flags: needinfo?(usarracini)
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?
Iteration: 61.4 - May 7 → ---
Iteration: --- → 62.1 - May 21
Priority: P2 → P3
Iteration: 62.1 - May 21 → 62.2 - Jun 4
(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
Flags: needinfo?(francesco.lodolo)
Depends on: 1396839
(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
Flags: needinfo?(francesco.lodolo)
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 about-logo@2x.png 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 about-logo@2x.png 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
Flags: needinfo?(edilee)
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 ;)
Flags: needinfo?(edilee)
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…
Flags: needinfo?(mpopova)
See Also: → 1463277
(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 felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/81367b23ee5f
Remove code from the old about:home. r=Mardak
https://hg.mozilla.org/mozilla-central/rev/81367b23ee5f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Blocks: 1395602
Blocks: 1281088, 1406524
Blocks: 1329955
Flags: needinfo?(mpopova)
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.