Set about:home to activity-stream

VERIFIED FIXED in Firefox 57

Status

()

enhancement
VERIFIED FIXED
2 years ago
24 days ago

People

(Reporter: andreio, Assigned: andreio)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

We want to make Activity Stream the default experience for about:home as well.
Assignee: nobody → andrei.br92
This patch adds a new pref "browser.newtabpage.activity-stream.disableHome" that when true will load Activity Stream on the about:home page.
Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

https://reviewboard.mozilla.org/r/171574/#review176902

::: browser/components/about/AboutRedirector.cpp:161
(Diff revision 1)
>            do_GetService("@mozilla.org/browser/aboutnewtab-service;1", &rv);
>          NS_ENSURE_SUCCESS(rv, rv);
>          rv = aboutNewTabService->GetDefaultURL(url);
>          NS_ENSURE_SUCCESS(rv, rv);
> +      } else if (path.EqualsLiteral("home")) {
> +        if (sActivityStreamDisableHome) { // Override about:home and set it to Activity Stream.

I intended the "disable home" to mean that activity stream on about:home would be disabled when true, so clearly the pref is poorly named if you implemented the opposite.
k88hudson, maybe we should just name the pref something like

browser.newtabpage.activity-stream.alsoEnableOnAboutHome

? This is to try to be clearer that it should only make about:home load activity stream if activity-stream.enabled is also true.

And this would also allow us to turn it on/off from the add-on without setting the pref in firefox.js… ?
Flags: needinfo?(khudson)
Actually, if we make it a pref that the add-on controls / sets defaults, then it won't be set until the add-on initializes. That might lead to some odd issues...
I think name-spacing it under browser.newtabpage.activity-stream makes sense, but like you said, unfortunately I think we probably do want it declared next to browser.newtabpage.activity-stream.enabled in firefox.js to avoid race conditions
Flags: needinfo?(khudson)
Also personally I'd go with an affirmative pref (browser.newtabpage.activity-stream.alsoEnableOnAboutHome or browser.newtabpage.activity-stream.aboutHome.enabled sounds fine) rather than negative (disableHome)
(In reply to Kate Hudson :k88hudson from comment #7)
> browser.newtabpage.activity-stream.aboutHome.enabled sounds fine
I suppose it's quite likely we'll have some aboutHome specific prefs.

Not sure if it's that important to imply

browser.newtabpage.activity-stream.aboutHome.alsoEnabled

vs just

browser.newtabpage.activity-stream.aboutHome.enabled

andreio, at least in the code, it should only replace about:home if both activity-stream.enabled and activity-stream.aboutHome. enabled are true.
Attachment #8900191 - Flags: review?(edilee)
Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

https://reviewboard.mozilla.org/r/171574/#review176946

::: browser/components/about/AboutRedirector.cpp:162
(Diff revisions 1 - 2)
>          NS_ENSURE_SUCCESS(rv, rv);
>          rv = aboutNewTabService->GetDefaultURL(url);
>          NS_ENSURE_SUCCESS(rv, rv);
>        } else if (path.EqualsLiteral("home")) {
> -        if (sActivityStreamDisableHome) { // Override about:home and set it to Activity Stream.
> +        // Override about:home and set it to Activity Stream.
> +        if (sActivityStreamEnabled && sActivityStreamAboutHomeEnabled) {

`sActivityStreamEnabled` is only inited via `GetURIFlags` while `sActivityStreamAboutHomeEnabled` is inited during `NewChannel`.

I don't recall whether one or the other always happens first, but given that these are public APIs, we shouldn't rely on a particular ordering.

I believe the fix here is to also init `sActivityStreamEnabled`.
Attachment #8900191 - Flags: review-
Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

https://reviewboard.mozilla.org/r/171574/#review176952

::: browser/components/about/AboutRedirector.cpp:163
(Diff revision 2)
>          rv = aboutNewTabService->GetDefaultURL(url);
>          NS_ENSURE_SUCCESS(rv, rv);
> +      } else if (path.EqualsLiteral("home")) {
> +        // Override about:home and set it to Activity Stream.
> +        if (sActivityStreamEnabled && sActivityStreamAboutHomeEnabled) {
> +          url.AssignASCII("resource://activity-stream/data/content/activity-stream.html");

Also, we probably sohuld just use aboutNewTabService to get the resource url just like the above `if` block.
Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

https://reviewboard.mozilla.org/r/171574/#review176946

> `sActivityStreamEnabled` is only inited via `GetURIFlags` while `sActivityStreamAboutHomeEnabled` is inited during `NewChannel`.
> 
> I don't recall whether one or the other always happens first, but given that these are public APIs, we shouldn't rely on a particular ordering.
> 
> I believe the fix here is to also init `sActivityStreamEnabled`.

Oh and speaking of `GetURIFlags`, we should set the same flags for :home as :newtab
Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

https://reviewboard.mozilla.org/r/171574/#review177524

We'll need to backport the activity-stream changes
Attachment #8900191 - Flags: review?(edilee) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0ae745e100aa
Add pref to enable Activity Stream on about:home. r=Mardak
https://hg.mozilla.org/mozilla-central/rev/0ae745e100aa
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/7e665d74229a2d8d4f5dbb0cd728769bc301d024
chore(channel): Backport bug 1392324 enable AS behind pref on about:home (#3237)
I can see this feature implemented on latest nightly 57.0a1 (2017-08-29) in Ubuntu 16.04, 64 bit 

Build ID 	20170829100404
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170830]
I can see this feature implemented on latest nightly 57.0a1 (2017-08-29) in windows 10 (32bit) 

Build ID 	20170829100404
Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170830]
As per Comment 18 and Comment 19, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Blocks: 1394533
Blocks: 1274705
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.