Replace about:home with about:newtab as the default homepage for the feb 2017 funnelcake

VERIFIED FIXED in Firefox 51

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox51 verified, firefox52 verified, firefox53 verified)

Details

(Whiteboard: LocalizablePreferences pref: browser.startup.homepage = "about:newtab")

Attachments

(1 attachment)

Assignee

Description

3 years ago
This should be pref-off-able in some way so we can keep the normal behaviour for the funnelcake
Assignee

Comment 1

3 years ago
This might be harder than I thought it was going to be. browser.startup.homepage is a localized pref, with the default being in   https://dxr.mozilla.org/mozilla-central/source/browser/branding/official/locales/browserconfig.properties . From what I can tell from the implementation in nsPrefBranch ( https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/modules/libpref/nsPrefBranch.cpp#217 ) this basically means that for:
- default values it reads the value of the pref and then tries to read the value of the relevant string from the .properties file that the pref value points to (ie the pref value *must* point to a .properties file). There doesn't seem to be a fallback for the default value not being a pointer to a .properties file.
- non-default values, it just returns the (char) value of the pref.

Nick, for funnelcakes/repacks, can we change these values, ie can you override browserconfig.properties or similar? (The pref in question is browser.startup.homepage)
Flags: needinfo?(nthomas)
I don't know about the underlying code which handles distribution.ini, but here is an example of an old funnelcake which sets browser.startup.homepage:
 https://github.com/mozilla-partners/funnelcake/blob/master/old/funnelcake41/distribution/distribution.ini#L13
Flags: needinfo?(nthomas)
Assignee

Comment 4

3 years ago
Ah, so there's a [LocalizablePreferences] ini section. That works. I think we might still need to write some other code here. I'll look in more detail tomorrow. Thanks!
Assignee

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 6

3 years ago
I could be convinced to wontfix this bug instead of taking this patch, but if we do want to do something it feels like we should treat about:newtab the same as about:home in being the "Firefox start page" or whatever. For about:home, I explicitly check that it's the default value so that we don't confuse users who have explicitly set it to about:newtab on a "normal" build. I didn't want to just use the default value because then for partner builds where e.g. www.foo.com is the default, we shouldn't pretend that that's the "Firefox start page" and not tell them the URL.
Assignee

Comment 7

3 years ago
(In reply to :Gijs Kruitbosch from comment #6)
> For
> about:home, I explicitly check that it's the default value so that we don't
> confuse users who have explicitly set it to about:newtab on a "normal"
> build.

Err, that should have read "For about:newtab". Note that I didn't bother with the same logic for the home button tooltip where that distinction seemed less important (as the prefs have UI to reset, and the home button in the main browser doesn't).
Not to say that your patch is wrong, but here's another idea: How about we make about:home an alias to about:newtab at a lower level? Can we do that conditionally?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 9

3 years ago
mozreview-review
Comment on attachment 8819342 [details]
Bug 1322719 - treat about:newtab like about:home in our UI when it's the default,

https://reviewboard.mozilla.org/r/99164/#review99460

I suppose we'll need this patch either way.
Attachment #8819342 - Flags: review?(dao+bmo) → review+
Assignee

Comment 10

3 years ago
(In reply to Dão Gottwald [:dao] from comment #8)
> Not to say that your patch is wrong, but here's another idea: How about we
> make about:home an alias to about:newtab at a lower level? Can we do that
> conditionally?

Not easily... there are other conditions elsewhere in the code (for instance, what loads in the parent/non-parent process, what loads with chrome privileges and what without) that are different, and then there's code that depends on that (like the about: tabsprogresslistener code in browser.js that I didn't know existed until a few hours ago) that would need changing... so aliasing it seems scary in terms of side-effects/complexity. :-(

With the existing patch, I would assume we basically just land this patch + have the funnelcake set browser.startup.homepage, and AIUI that Should Just Work. Does that sound sane?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao+bmo)
Sounds okay.
Flags: needinfo?(dao+bmo)

Comment 12

3 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bd570e5a9b45
treat about:newtab like about:home in our UI when it's the default, r=dao

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd570e5a9b45
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Is this ready for uplift?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 15

3 years ago
(In reply to Dão Gottwald [:dao] from comment #14)
> Is this ready for uplift?

Waiting to hear from Dolske, going to clear ni for now.
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: LocalizablePreferences pref: browser.startup.homepage = "about:newtab"
Assignee

Comment 16

3 years ago
Comment on attachment 8819342 [details]
Bug 1322719 - treat about:newtab like about:home in our UI when it's the default,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1322718 comment #1
[User impact if declined]: not having this patch will affect the experience of users who run the funnelcake.
[Is this code covered by automated tests?]: No. This code only takes effect if the homepage pref's default is changed. I decided it was enough of an edgecase to not bother with an automated test.
[Has the fix been verified in Nightly?]: no, as it needs a custom build with a pref changed.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. We'll need a custom build set up. With the custom build, we should check that the tooltip of the 'home' button is "Mozilla Firefox start page", and ditto for the contents of the 'home page' setting in the preferences if the default homepage is set to about:newtab.
[List of other uplifts needed for the feature/fix]: this is related to all the other deps of bug 1322718, but the patch is standalone and can be uplifted independently.
[Is the change risky?]: no
[Why is the change risky/not risky?]: because it will mostly affect the funnelcake (ie it's behind a pref). The only effect on normal builds is that the tooltip of the homepage button will say "Mozilla Firefox Home Page" for users who have configured their homepage to be "about:newtab" manually (without these patches, the tooltip will say "about:newtab".
[String changes made/needed]: no.
Attachment #8819342 - Flags: approval-mozilla-beta?
Attachment #8819342 - Flags: approval-mozilla-aurora?
Assignee

Updated

3 years ago
Comment on attachment 8819342 [details]
Bug 1322719 - treat about:newtab like about:home in our UI when it's the default,

Small changes in newtab for the funnelcake onboarding experiment.
Thanks for explaining the behavior changes in details, Gijs!
Attachment #8819342 - Flags: approval-mozilla-beta?
Attachment #8819342 - Flags: approval-mozilla-beta+
Attachment #8819342 - Flags: approval-mozilla-aurora?
Attachment #8819342 - Flags: approval-mozilla-aurora+
Setting the flag for verification, instructions available in Comment 16.
Flags: qe-verify+
Flags: needinfo?(gwimberly)

Updated

3 years ago
Flags: needinfo?(gwimberly)

Updated

2 years ago
Status: RESOLVED → VERIFIED
Hi Gijs,
Could you possibly provide the custom build you mentioned in Comment #16? I want to verify the fix for 52 & 53. Thanks.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 22

2 years ago
(In reply to Alexandru Simonca, QA (:asimonca) from comment #21)
> Hi Gijs,
> Could you possibly provide the custom build you mentioned in Comment #16? I
> want to verify the fix for 52 & 53. Thanks.

53: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25f298024796f3d33a2892d4ecff5e2bed6ed98
52: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=736a9d8dec8febf77ebd346fb0ed2e1fe39bcc79
Flags: needinfo?(gijskruitbosch+bugs)
Based on Comment #16 and on the try builds Gijs provided I consider this issue to be VERIFIED FIXED.

The tooltip for the Home button in the try builds reads "Nightly Start Page" and the contents of the 'home page' setting in the preferences is the same while the default homepage is set to "Show my homepage". 
This applies to Windows 10 x64 and Mac OS X 10.11.6.
You need to log in before you can comment on or make changes to this bug.