1392324 - Set about:home to activity-stream

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Andrei Oprea [:andreio]]

We want to make Activity Stream the default experience for about:home as well.

Comment 1

[Andrei Oprea [:andreio]]

Attachment: Bug 1392324 - Add pref to enable Activity Stream on about:home.

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

Comment 2

[Andrei Oprea [:andreio]]

This patch adds a new pref "browser.newtabpage.activity-stream.disableHome" that when true will load Activity Stream on the about:home page.

Comment 3

[Ed Lee :Mardak]

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.

Comment 4

[Ed Lee :Mardak]

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… ?

Comment 5

[Ed Lee :Mardak]

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

Comment 6

[Kate Hudson :k88hudson]

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

Comment 7

[Kate Hudson :k88hudson]

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)

Comment 8

[Ed Lee :Mardak]

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

Comment 9

[Andrei Oprea [:andreio]]

Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

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

Comment 10

[Ed Lee :Mardak]

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

Comment 11

[Ed Lee :Mardak]

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 12

[Ed Lee :Mardak]

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 13

[Andrei Oprea [:andreio]]

Comment on attachment 8900191 [details]
Bug 1392324 - Add pref to enable Activity Stream on about:home.

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

Comment 14

[Ed Lee :Mardak]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae745e100aa
Add pref to enable Activity Stream on about:home. r=Mardak

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ae745e100aa

Comment 17

[[github robot]]

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)

Comment 18

[Md. Majedul islam]

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

Comment 19

[Md. Rahimul Islam]

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]

Comment 20

[Md. Rahimul Islam]

As per Comment 18 and Comment 19, I am marking this bug as verified fixed.