Closed Bug 1433324 Opened 6 years ago Closed 6 years ago

Remove browser.newtabpage.activity-stream.enabled and browser.newtabpage.activity-stream.aboutHome.enabled prefs

Categories

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

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Iteration:
60.3 - Feb 26
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: Mardak, Assigned: Mardak)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [AS60MVP] )

Attachments

(3 files)

Bug 1433324 - Part 1. Assume true for browser.newtabpage.activity-stream.aboutHome.enabled.
59 bytes, text/x-review-board-request
ursula
: review+
Details
Bug 1433324 - Part 3. Assume true for browser.newtabpage.activity-stream.enabled.
59 bytes, text/x-review-board-request
ursula
: review+
Details
Bug 1433324 - Part 2. Get auto migration tests running with activity stream enabled.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
There's various code paths and tests that check and/or set these prefs, e.g., aboutNewTabService, AboutRedirector, activity stream's bootstrap.js. Let's simplify the code by assuming it's true and remove the pref. This will lead to a bunch of dead code that is not directly in the above files, and hopefully those can be simply removed in bug 1409054 for home and bug 1433133 for tiles without needing to change any logic.
Assignee: nobody → edilee
Comment on attachment 8950889 [details]
Bug 1433324 - Part 3. Assume true for browser.newtabpage.activity-stream.enabled.

emtwo, I hardcoded that AS_ENABLED ping is always true. I'm guessing that there's additional metadata being sent as part of the ping, so it shouldn't be completely removed?

https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/components/nsBrowserGlue.js#369-380
Attachment #8950889 - Flags: feedback?(msamuel)
ursula, browser_newtab_overrides.js used to run with activity stream disabled, i.e., tiles, which ran in chrome privileges. I flipped the test to check that it's *not* system principal. Not entirely sure how it affects bug 1385306.
See Also: → 1385306
Comment on attachment 8950889 [details]
Bug 1433324 - Part 3. Assume true for browser.newtabpage.activity-stream.enabled.

gijs, I ended up skipping auto migration tests as they were only running for Tiles about:newtab. Should auto migration stuff have a bug to remove its logic and tests, etc? I can reference it from https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/components/migration/tests/browser/browser.ini#3
Attachment #8950889 - Flags: feedback?(gijskruitbosch+bugs)
gijs, also, I noticed MAKE_LINKABLE was only for about:home but not about:newtab, but they're the same, so I made sure the flags were the same with `static const uint32_t ACTIVITY_STREAM_FLAGS = …`. Should be okay?
(In reply to Ed Lee :Mardak from comment #6)
> gijs, also, I noticed MAKE_LINKABLE was only for about:home but not
> about:newtab, but they're the same, so I made sure the flags were the same
> with `static const uint32_t ACTIVITY_STREAM_FLAGS = …`. Should be okay?

Activity Stream doesn't need make_linkable, so it shouldn't have it. And no, it's not OK - it means the web can link (open) that about: page. The only reason it's on about:home is because about:home used to need it to work for snippets, and I and bz have been too busy to fix bug 1228118 and clean it all up. Seems like you're removing non-AS about:home here so you can just drop the flag on the floor.
(In reply to Ed Lee :Mardak from comment #5)
> Comment on attachment 8950889 [details]
> Bug 1433324 - Part 2. Assume true for
> browser.newtabpage.activity-stream.enabled.
> 
> gijs, I ended up skipping auto migration tests as they were only running for
> Tiles about:newtab. Should auto migration stuff have a bug to remove its
> logic and tests, etc? I can reference it from
> https://searchfox.org/mozilla-central/rev/
> d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/components/migration/tests/
> browser/browser.ini#3

No, we're still wanting this stuff. Doug Thayer knows more. I don't remember why it got disabled for AS, but it was to do with not wanting to put in the work to make the code work or some of AS interfering with automigration. The right solution now is not "remove the thing we didn't want to bother fixing at the time".
(In reply to :Gijs from comment #9)
> (In reply to Ed Lee :Mardak from comment #5)
> > Comment on attachment 8950889 [details]
> > Bug 1433324 - Part 2. Assume true for
> > browser.newtabpage.activity-stream.enabled.
> > 
> > gijs, I ended up skipping auto migration tests as they were only running for
> > Tiles about:newtab. Should auto migration stuff have a bug to remove its
> > logic and tests, etc? I can reference it from
> > https://searchfox.org/mozilla-central/rev/
> > d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/components/migration/tests/
> > browser/browser.ini#3
> 
> No, we're still wanting this stuff. Doug Thayer knows more. I don't remember
> why it got disabled for AS, but it was to do with not wanting to put in the
> work to make the code work or some of AS interfering with automigration. The
> right solution now is not "remove the thing we didn't want to bother fixing
> at the time".

See in particular bug 1395305 comment 3 .

Sorry if my phrasing in comment 9 was harsh. But we do need to actually fix this now, or if we really don't want to do it here, then we should perma-disable the test and have a follow-up bug that needs some resourcing.
(In reply to Ed Lee :Mardak from comment #4)
> ursula, browser_newtab_overrides.js used to run with activity stream
> disabled, i.e., tiles, which ran in chrome privileges. I flipped the test to
> check that it's *not* system principal. Not entirely sure how it affects bug
> 1385306.

Yeah that's fine, I'll just have to wait for this to land and rebase the patch for bug 1385306 on it
Comment on attachment 8950888 [details]
Bug 1433324 - Part 1. Assume true for browser.newtabpage.activity-stream.aboutHome.enabled.

https://reviewboard.mozilla.org/r/220144/#review226064

Thanks for doing this work Ed! Just a nit, and this patch looks good

::: browser/base/content/test/about/browser.ini:23
(Diff revision 1)
>  [browser_aboutHome_search_searchbar.js]
>  [browser_aboutHome_search_suggestion.js]
>  skip-if = os == "mac" || (os == "linux" && (!debug || bits == 64)) # Bug 1399648, bug 1402502
>  [browser_aboutHome_search_telemetry.js]
>  [browser_aboutHome_snippets.js]
> +skip-if = true # Bug 1409054 to remove

I know these tests (_input.js, _wrapsCorrectly.js, and _snippets.js)  are skipped so it doesn't matter too much, but they do try to set "browser.newtabpage.activity-stream.aboutHome.enabled" to false, so maybe just also remove the reference to the pref within the tests themselves too?
Attachment #8950888 - Flags: review?(usarracini) → review+
Comment on attachment 8950889 [details]
Bug 1433324 - Part 3. Assume true for browser.newtabpage.activity-stream.enabled.

https://reviewboard.mozilla.org/r/220146/#review226096

In terms of the parts of the patch for removing the pref and making sure the default is that Activity Stream is on, this looks good. I can't speak for the migration tests and the telemetry stuff though. Is there a try run available to make sure we didn't miss anything here?

::: browser/components/newtab/aboutNewTabService.js:46
(Diff revision 1)
>    if (!IS_RELEASE_OR_BETA) {
>      Services.prefs.addObserver(PREF_ACTIVITY_STREAM_DEBUG, this);
>    }
>  
>    // More initialization happens here
> -  this.toggleActivityStream();
> +  this.toggleActivityStream(true);

Do we need to call this with `true` if we can just set `_activityStreamEnabled: true` as the default value?
I feel like the entire `toggleAcivityStream` function is kind of irrelevant now since there's no way to do that anymore. Maybe we can set all the values to default to true and simplify aboutNewTabService even more
Attachment #8950889 - Flags: review?(usarracini) → review+
Blocks: 1438305
Blocks: 1438311
Comment on attachment 8950889 [details]
Bug 1433324 - Part 3. Assume true for browser.newtabpage.activity-stream.enabled.

https://reviewboard.mozilla.org/r/220146/#review226096

I filed followups for migration stuff and telemetry. There's a try run from yesterday that seems good https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5d84ad050c9ddac5b97952626949b93ee51013b and I triggered a new one rebased with additional changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0c54a5f4dae7ac19a890f2c5f87317ad1ace0e1

> Do we need to call this with `true` if we can just set `_activityStreamEnabled: true` as the default value?
> I feel like the entire `toggleAcivityStream` function is kind of irrelevant now since there's no way to do that anymore. Maybe we can set all the values to default to true and simplify aboutNewTabService even more

I agree that the code could probably be cleaned up more, but there is a code path that calls `toggleActivityStream(false)`, e.g., via web extensions. Whether aboutNewTabService should still treat that as activity stream enabled or not.. unclear especially with the idl exposing activityStreamEnabled. I'll file a followup to clean it up more.
Blocks: 1438316
(In reply to :Gijs from comment #8)
> Activity Stream doesn't need make_linkable, so it shouldn't have it.
Up until bug 1425454, mozilla.org was directly navigating to about:home. Although for that bug, I made it call a UITour method to navigate to about:newtab. Unclear if any other pages were depending on being able to navigate to about:home.

I don't quite understand the original needing MAKE_LINKABLE for about:home, but bug 1228118 that was in the comment seems to indicate it's for IndexedDB's use, which activity stream does need for snippets (although maybe other uses too in the future). I'll double check to see if removing the flag breaks snippets, etc.

(In reply to :Gijs from comment #9)
> The right solution now is not "remove the thing we didn't want to bother fixing at the time".
I filed bug 1438305 as it's more complicated than just showing auto migration as there's already the manual migration messaging.
Looks like removing MAKE_LINKABLE causes test failures as it's trying to navigate to about:home:

https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js#14

Console message: Security Error: Content at moz-nullprincipal:{3f279e7d-c9e1-459e-a397-e646f512c647} may not load or link to about:home.

I guess I'll point it to about:rights. ?
(In reply to Ed Lee :Mardak from comment #19)
> Looks like removing MAKE_LINKABLE causes test failures as it's trying to
> navigate to about:home:
> 
> https://searchfox.org/mozilla-central/source/browser/base/content/test/
> general/browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js#14
> 
> Console message: Security Error: Content at
> moz-nullprincipal:{3f279e7d-c9e1-459e-a397-e646f512c647} may not load or
> link to about:home.
> 
> I guess I'll point it to about:rights. ?

Please us BrowserTestUtils.registerAboutPage instead to have a page that's specific to the test so that we stop relying on various characteristics of other about: pages which are subject to change.
(In reply to Ed Lee :Mardak from comment #18)
> (In reply to :Gijs from comment #8)
> > Activity Stream doesn't need make_linkable, so it shouldn't have it.
> Up until bug 1425454, mozilla.org was directly navigating to about:home.
> Although for that bug, I made it call a UITour method to navigate to
> about:newtab. Unclear if any other pages were depending on being able to
> navigate to about:home.

I sure hope not... It's a security liability, tbh.

> I don't quite understand the original needing MAKE_LINKABLE for about:home,
> but bug 1228118 that was in the comment seems to indicate it's for
> IndexedDB's use, which activity stream does need for snippets (although
> maybe other uses too in the future). I'll double check to see if removing
> the flag breaks snippets, etc.

Yeah... I think this was why, effectively the innermost URL changes if MAKE_LINKABLE gets changed (was moz-safe-about:home, will be about:home) which changes the origin for the indexeddb database, which then means things are... unhappy.

I hope that the way AS runs snippets means this doesn't matter for about:home-as-activity-stream, but I wasn't involved with the snippets implementation for AS, and the about:home vs. nested URI stuff was a while back so I don't remember too many details.

about:newtab + AS doesn't have this problem because the indexeddb usage for snippets has never lived at moz-safe-about:newtab at all, because about:newtab has never been linkable. That said, if you changed that to be linkable then presumably the snippets there would have issues...

> (In reply to :Gijs from comment #9)
> > The right solution now is not "remove the thing we didn't want to bother fixing at the time".
> I filed bug 1438305 as it's more complicated than just showing auto
> migration as there's already the manual migration messaging.

This is preffed off by default, so I don't think that just moving it across to keep the integration test coverage would be *that* bad an idea, but meh. A follow-up will do.
Blocks: 1438367
Gijs, is this good enough to run all 3 migration mochitests everywhere except multiple_dismissal for debug linuxes? The more I poked at it, the less it seemed worthwhile to keeping the notification-bar-specific stuff as most likely activity stream will be doing some in-content messaging, e.g., https://mozilla.michaelverdi.com/public/am6.framer/

Here's a try that had ~50% linux debug failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e16cae919d5534590afb5bf8c3e4f21cc1da467&filter-searchStr=(bc

And skipping the one test for linux debug got it all green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d227edc7eb04f3062c7194bb707320262cf8d6f&filter-searchStr=(bc
Comment on attachment 8951161 [details]
Bug 1433324 - Part 2. Get auto migration tests running with activity stream enabled.

https://reviewboard.mozilla.org/r/220428/#review226420

Yep, this looks OK to me. Thanks!
Attachment #8951161 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ad6392e366d4
Part 1. Assume true for browser.newtabpage.activity-stream.aboutHome.enabled. r=ursula
https://hg.mozilla.org/integration/autoland/rev/f3069763fab6
Part 2. Get auto migration tests running with activity stream enabled. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/088e727e5cf7
Part 3. Assume true for browser.newtabpage.activity-stream.enabled. r=ursula
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/f32b1b0dee155cad90ff1aa89edfa344db7ec9f5
chore(mc): Port Bug 1433324 - Part 3. Assume true for browser.newtabpage.activity-stream.enabled. r=ursula
Depends on: 1384094
So, I am assuming that there is no going back now? No way to get the older big-preview-thumbnail'ed new tab page?
It's intolerable that you force users to use some add-on just to have tiles with an acceptable size. Moreover, the new tab provided by add-ons is not preloaded so the experience is not nice.

However, I managed to make activity stream behave closer to the old page:

 1. Disable all the useless things, i.e. only leave the "Top sites" and possibly "Search".
 2. Check "Show two rows"
 3. In your Firefox profile, create chrome/userContent.css with this code:

@-moz-document url(about:newtab), url(about:home) {
  .top-sites-list .top-site-outer .tile,
  .top-sites-list .top-site-outer .title {
    width: 280px !important;
  }
  .top-sites-list .top-site-outer .tile {
    height: 190px !important;
  }
  .wide-layout-enabled main {
    width: 85% !important;
  }
  .top-sites-list {
    max-height: 695px !important;
    overflow-y: hidden !important;
    text-align: center !important;
    padding-top: 15px !important;
  }
}

Could be improved with media queries, but works reasonably for me.
Restart Firefox to see the changes.
I have reproduced this bug on nightly 60.0a1 (2018-01-25) on windows 10, 64 bit.

The bug's fix is now verified on Latest Nightly 
Build ID 	20180217100053
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0

[testday-20180216]
(In reply to Oriol Brufau [:Oriol] from comment #33)
>  2. Check "Show two rows"
If you want more rows, you can set browser.newtabpage.activity-stream.topSitesRows to some larger number. The about:preferences UI will expose a dropdown to select up to 4 soon in bug 1400536.
(In reply to Ed Lee :Mardak from comment #35)
> If you want more rows, you can set
> browser.newtabpage.activity-stream.topSitesRows to some larger number. The
> about:preferences UI will expose a dropdown to select up to 4 soon in bug
> 1400536.

Thanks, but 16 tiles (or 5x3 in my code above) are enough. I just want them to be bigger. Otherwise I can't see what's in the image. Using a 200% zoom is also somewhat acceptable, but looks much better with my CSS above.
I've verified that the two prefs no longer appear by default on clean new profile, or after you update to the latest Nightly from a build that still had the prefs.

However, if the prefs were changed to false, after you update to the latest Nightly you can still find them in about:config. I'm not sure if this is intended because they are user changed prefs or if they should not longer be shown because they were taken out. 

Ed, should the prefs still exist in this case after update or not?
Flags: needinfo?(edilee)
Blocks: 1370930
about:config will show all default prefs and user set prefs, and the way activity-stream.enabled would have had a user set value is if the user went to about:config, so probably not really a priority to clean it up…
Flags: needinfo?(edilee)
Marking this as Verified based on the first part of comment 37. 
I've logged bug 1439933 as a follow-up for the clean up.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Depends on: 1438763
Summary: Remove browser.newtabpage.activity-stream.enabled and .aboutHome.enabled prefs → Remove browser.newtabpage.activity-stream.enabled and browser.newtabpage.activity-stream.aboutHome.enabled prefs
Blocks: 1458933
Depends on: 1509084
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: