721211 - consolidate home page retrieval code, and add getCharPref fallback to allow simple overriddes in prefs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 642106 comment 21.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

Comment 3

[Axel Hecht [:Pike]]

Attachment: rebased patch

We came across this bug again because the original hack is a problem for the Chinese community and using pontoon.

I rebased the patch by gavin for now.

Gavin, do you remember where you left off on this patch?

Comment 4

[Axel Hecht [:Pike]]

I found a few things where I need info on driving this forward:

Dolske, mconnor: after this patch, browserconfig.properties are all empty. Should they just be removed?

I'm in particular wondering if any of this is used by partner repacks or should be.

Henrik, I see that you're relying on browserconfig.properties for testing, and for testing testing. Thanks to dxr finding those now :-). Testing testing would need a new fix, and possibly the homepage tests should use the jsm? Also not sure where to land those fixes and how to get them into the tree.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Axel Hecht [:Pike] from comment #4)
> Henrik, I see that you're relying on browserconfig.properties for testing,
> and for testing testing. Thanks to dxr finding those now :-). Testing
> testing would need a new fix, and possibly the homepage tests should use the
> jsm? Also not sure where to land those fixes and how to get them into the
> tree.

If you have to make changes to firefox ui tests (i think that you talk about those) you best create the patch against the github repository. That way you get Travis feedback, which you wont get when landing the changes in tree right now (currently i work on check-in based test execution in TC). Best file a bug under Testing / Firefox ui tests

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I don't think there's any reason to leave browserconfig.properties around. Not sure why my earlier patch didn't delete them.

I can't recall any specific reasons for not going ahead with this.

Comment 7

[Justin Dolske [:Dolske]]

Removing browserconfig.properties makes sense to me too. No idea if partner repacks are involved with that, I'd ping mconnor to see if they need to do anything.

Comment 8

[Axel Hecht [:Pike]]

Attachment: MozReview Request: Bug 721211: consolidate home page retrieval code, make it support non-localized prefs, r?dolske

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

Comment 9

[Axel Hecht [:Pike]]

Attachment: MozReview Request: bug 721211, fix up UI tests now that homepage is a regular pref, r?whimboo

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

Comment 10

[Axel Hecht [:Pike]]

Taking, also pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b2363ccd76e

needinfo on mconnor for the partner build question from comment 4.

Comment 11

[Axel Hecht [:Pike]]

OK, the browser patch fails on try, there are many more places that set and get the homepage pref right now, need to go through a few more.

The testing patch shouldn't be affecte much.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8708395 [details]
MozReview Request: bug 721211, fix up UI tests now that homepage is a regular pref, r?whimboo

https://reviewboard.mozilla.org/r/31035/#review27895

Looks pretty straight forward. Given that we do not use this pref in our update tests, no fallback code is necessary. 

Given that we are in a transition phase we still have our canonical repository under https://github.com/mozilla/firefox-ui-tests. If you don't mind to also create a patch for the mozilla-central branch there, it would be great. Otherwise I will port this over once it has been landed here.

Comment 13

[Axel Hecht [:Pike]]

I pushed the latest version of my patch to try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=43105181cedf. Only unrelated intermittent failures left.

reviewboard doesn't know about this try run, because it didn't let me trigger it, so ignore the warning over there.

I've ended up refactoring things quite a bit, and moved most interactions with the homepage go through HomePage.jsm. Some tests still do it directly, and I've not added an API to reset().

Comment 14

[Axel Hecht [:Pike]]

https://reviewboard.mozilla.org/r/31033/#review29171

Comment 15

[Axel Hecht [:Pike]]

Attachment: refactored patch

Taking this off mozreview, 'cause that crashes right now.

This patch turned out to be more intensive that gavin's original, so switching the ownership of this patch to me.

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 8715778 [details] [diff] [review]
refactored patch

Review of attachment 8715778 [details] [diff] [review]:
-----------------------------------------------------------------

r-: Mostly there, just a number of small changes.

I assume we also need an L10N bug, dependent on this one, to switch the CN homepage override from browserconfig.properties to the localized prefs file?

General comment: I think the long term goal is to eventually make this more difficult to hijack (bug 1051082), such as was done with the new-tab URL (bug 1118285) and search (part of bug 1203167?). So eventually this shouldn't be nsIPref at all. That work is clearly out of scope for this bug, which is an incremental improvement to untangling the current state of affairs.

::: browser/base/content/browser.js
@@ +2984,3 @@
>   */
>  function getDefaultHomePage() {
>    // Get the start page from the *default* pref branch, not the user's

nit: I think this comment can just go away.

@@ +2984,4 @@
>   */
>  function getDefaultHomePage() {
>    // Get the start page from the *default* pref branch, not the user's
> +  return HomePage.getDefault();

I think the "If url is a pipe-delimited set of pages, just take the first one" code you removed needs to stay here. Good practice, and it's possible some other build is using it.

@@ +5113,5 @@
>      }
>    },
>  
> +  getHomePage: function () {
> +    return HomePage.get();

This API (gHomeButton.getHomePage()) is kind of pointless now. I suppose we should keep it, because I see addons using it. :(

Bonus points for removing its use in our own code (in favor of HomePage.get directly)? I see 1 call in SessionStore.jsm, and 2 in browser.js.

::: browser/components/nsBrowserGlue.js
@@ +2020,5 @@
>        // or google.com/firefox.
>        const HOMEPAGE_PREF = "browser.startup.homepage";
>        if (Services.prefs.prefHasUserValue(HOMEPAGE_PREF)) {
> +        const DEFAULT = HomePage.getDefault();
> +        let value = HomePage.get();

The .getDefault() is right, but I think the rest of this code should actually keep setting the pref directly, as in the old code, and not use the HomePage module.

I know, that sounds weird! But it helps future proof if/when we move HomePage to use a non-prefs backing store, and prevent lurking bugs if this code no longer actually updates the pref value.

(Or to look at it a different way, the code here mixes use of HomePage and the pref service. It's a layering violation that only works right now because HomePage happens to use prefs.)

::: browser/modules/HomePage.jsm
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const prefName = "browser.startup.homepage";
> +
> +function getHome(useDefault) {

tiny bikeshed: Instead of "getHome" -- "getPrefValue" or "getHomepagePref" or ...

@@ +20,5 @@
> +  }
> +  try {
> +    homePage = prefs.getComplexValue(prefName,
> +                                     Ci.nsIPrefLocalizedString).data;
> +  } catch (ex) {}

Oh, I see, you're retaining support for someone still using a .properties file.

Probably worth a comment briefly noting the history (ie, why this code is there). Although... Once we remove our own CN usage of this, maybe we should just relnote this and keep things simple. In fact, I think I'd prefer that unless there's a reason to keep supporting it.

@@ +37,5 @@
> +
> +  return homePage;
> +}
> +
> +let HomePage = {

I'd like to see HomePage use a similar API to NewTabURL.jsm.

I think that's mostly just s/set/override/.

Adding overridden() and reset() should also be fairly trivial: via prefHasUserValue() and clearUserPref(). And at least the general prefs page should use these for the homepage "Restore to Default" button.

Ideally we wouldn't need getDefault(), but it looks like we do this in a number of spots because we're afraid your current homepage has been hijacked. Sigh!

One complexity with the current pref value is that it actually supports multiple URLs separated by "|"... I think it would likely be saner to handle this in the API, for callers that want / know how to deal with multiple URLs. EG, have getMulti() and overrideMulti(). But would be fair to just file a followup and not do that here.

@@ +38,5 @@
> +  return homePage;
> +}
> +
> +let HomePage = {
> +  get: function HomePage_get() {

The function names (HomePage_get / HomePage_getDefault / HomePage_set) are not needed.

::: toolkit/mozapps/installer/upload-files.mk
@@ -303,4 @@
>    ua-update.json \
>    platform.ini \
>    greprefs.js \
> -  browserconfig.properties \

Hmmm... Is this a special file that should also be added to removed-files.in? rstrong?

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8715778 [details] [diff] [review]
refactored patch

Review of attachment 8715778 [details] [diff] [review]:
-----------------------------------------------------------------

I'm assuming HomePage.jsm will become an API recommended for add-ons. Can we ensure it's covered by automated tests?

::: browser/modules/HomePage.jsm
@@ +5,5 @@
> +let EXPORTED_SYMBOLS = ["HomePage"];
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

nit: New code often has this on a single line these days:
const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

@@ +9,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const prefName = "browser.startup.homepage";

nit: this const should have a name making it explicit that it's not a variable, eg. kPrefName or PREF_NAME.

@@ +13,5 @@
> +const prefName = "browser.startup.homepage";
> +
> +function getHome(useDefault) {
> +  var homePage;
> +  var prefs = Services.prefs;

nit: 'let' for new code.

@@ +46,5 @@
> +    return getHome(true);
> +  },
> +  set: function HomePage_set(value) {
> +    let str = Cc["@mozilla.org/supports-string;1"]
> +      .createInstance(Ci.nsISupportsString);

This is missing a line here:
  str.data = value;

Comment 18

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #16)

> ::: toolkit/mozapps/installer/upload-files.mk
> @@ -303,4 @@
> >    ua-update.json \
> >    platform.ini \
> >    greprefs.js \
> > -  browserconfig.properties \
> 
> Hmmm... Is this a special file that should also be added to
> removed-files.in? rstrong?

Oops, meant to actually NI rstrong on this bit.

Comment 19

[Robert Strong (they/them - no direct email)]

It depends on where the file lives. Comment from removed-files.in

# When to use removed-files.in file to remove files and directories:
# * Files and directories located in the installation's "distribution/" and
#   "extensions/" directories that were added before Firefox 27. Files and
#   directories located in these directories were not included in the
#   application update file removals for a complete update prior to Firefox 27.
# * Empty directories that were accidentally added to the installation
#   directory.
# * Third party files and directories that were added to the installation
#   directory. Under normal circumstances this should only be done after release
#   drivers have approved the removal of these third party files.

Comment 20

[Axel Hecht [:Pike]]

Comment on attachment 8715778 [details] [diff] [review]
refactored patch

Review of attachment 8715778 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at review comments made me realize....

::: browser/components/nsBrowserGlue.js
@@ +2025,3 @@
>          let updated =
> +          value.replace(/https?:\/\/start\.mozilla\.org[^|]*/i, DEFAULT)
> +                replace(/https?:\/\/(www\.)?google\.[a-z.]+\/firefox[^|]*/i,

err, right, like this is gonna work.

I butchered this one pretty badly, obviously there are no tests for this code at all :-(

(hint, '.')

Comment 21

[Axel Hecht [:Pike]]

Got a new try-run up, https://treeherder.mozilla.org/#/jobs?repo=try&revision=25bffd271f5f.

Didn't rename .set() to .override(), I feel like changing one's homepage would be popular enough to just .set(). Don't care really, though, it's more that I didn't actually do that yet.

I found another problem: There's code that's using the observer service to watch for the pref, notably the gHomeButton does. Dolske, any suggestion for that?

I also fixed two tests, but I'm not sure if I'm fond of that. See https://hg.mozilla.org/try/rev/79213437e0b8#l2.1 for the current state.

For the gHomeButton.getHomePage(), I added a Deprecated.warning(), also for BrowserContentHander.startPage. Not sure if the second one is needed?

Lastly, I don't think I fix the lack of testing of the migration code. The current phase of this patch is already a bit over my head. Just so that I can stretch, but I'm reaching a point where someone more frequently interacting with this code would have a much easier time.

Also got some answers:

We don't need removed-files.in, the browserconfig.properties is in browser/omni.ja.
I'm keeping the localized pref around, because I guess that there are either distributions, or customizations, that make use of this.
Also, it means that we can land this without breaking chinese, and then fix chinese as a follow-up.

Comment 22

[Axel Hecht [:Pike]]

PS: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25bffd271f5f is full of green.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

The nsBrowserGlue homepage resetting code should probably just be removed now, since it has presumably been run for the majority of users for which it could ever be run. Or if not, at least note in bug 1051082 that the nsBrowserGlue code should be removed/reimplemented when that bug deals with migration.

Comment 24

[Axel Hecht [:Pike]]

I was wondering about the migration code, but figured "this is how it should be".

https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1812 has migrations starting from UI version 2, the migration I'm changing is for 24.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Some of the migrations are still there because they are useful/needed for new users (who start off at currentUIVersion=0). Any others are just left around either because there's still a small chance that someone will be upgrading from a build before it was introduced, or because removing them isn't a high priority.

Since this migration is not particularly important (i.e. missing it won't cause any data loss), is not relevant to new users, and was added a few releases ago (Firefox 36, though admittedly I don't know what percentage of the Firefox user base is currently on 36+), my opinion is that removing it now would be fine.

Comment 26

[Justin Dolske [:Dolske]]

(In reply to Axel Hecht [:Pike] from comment #21)

> I found another problem: There's code that's using the observer service to
> watch for the pref, notably the gHomeButton does. Dolske, any suggestion for
> that?

I'd probably just replace it with a static string, e.g. "Open your home page". If someone needs the tooltip they're probably more interested in what the button does, not where it goes.

Comment 28

[Axel Hecht [:Pike]]

Attachment: Bug 721211: consolidate home page retrieval code, make it support non-localized prefs,

Also changes the tooltip on the home button to be independent of the URLs
it opens, per dolske.

Some tests explicitly set browser.startup.homepage, but only through
SpecialPowers.putPrefEnv. That's a good compromise, given the extra
functionality there.

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

Comment 29

[Axel Hecht [:Pike]]

Taking this back up after a while.

compared to the old patch in https://reviewboard.mozilla.org/r/31033/diff/1#index_header (which I don't know why mozreview didn't pick up to build on), this is a bit more code, but also a bit more removals.

In particular, I stopped trying to show Deprecated notices for unexposed APIs, given that they're not exposed to xul addons two years later.

I'm totally puzzled why https://treeherder.mozilla.org/#/jobs?repo=try&revision=a777164af10f00753c3b2641d369795ca2ccb054&selectedJob=181683221 fails, the rest seems intermittent on try.

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 8983012 [details]
Bug 721211: consolidate home page retrieval code, make it support non-localized prefs,

I'm probably not the best person to review this now. I don't even remember the previous review. :)

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8983012 [details]
Bug 721211: consolidate home page retrieval code, make it support non-localized prefs,

https://reviewboard.mozilla.org/r/248868/#review262624

::: browser/modules/HomePage.jsm:31
(Diff revision 1)
> +  if (!homePage)
> +    homePage = prefs.getStringPref(kPrefName);

Please wrap this with curly braces.

::: browser/modules/HomePage.jsm:59
(Diff revision 1)
> +    return Services.prefs.prefHasUserValue(kPrefName);
> +  },
> +
> +  set(value) {
> +    Services.prefs.setStringPref(kPrefName, value);
> +

Please remove this empty line.

::: browser/modules/test/unit/test_HomePage.js:12
(Diff revision 1)
> +
> +ChromeUtils.import("resource:///modules/HomePage.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +add_task(function* () {
> +  Assert.ok(!HomePage.overridden, "Homepage should not be overriden by default.");

Can you add a test case that uses a complex value?

Comment 32

[Axel Hecht [:Pike]]

Comment on attachment 8983012 [details]
Bug 721211: consolidate home page retrieval code, make it support non-localized prefs,

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

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 8983012 [details]
Bug 721211: consolidate home page retrieval code, make it support non-localized prefs,

https://reviewboard.mozilla.org/r/248868/#review262624

> Please wrap this with curly braces.

Done

> Can you add a test case that uses a complex value?

I actually had to fix the test a bit as rebasing ended up getting the platform skips wrong for this test, and the attribution test.

Pushed to try, too. https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a547e925ddaa4d4bfdc8a316b71d626ca2c7cc

Comment 34

[Axel Hecht [:Pike]]

Note, on my try push I also triggered zh-CN builds, and verified that the homepage customization in there still works.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb08bc2f2956
consolidate home page retrieval code, make it support non-localized prefs, r=jaws

Comment 36

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bb08bc2f2956

Comment 37

[Hector Zhao [:hectorz]]

Should the nsIBrowserHandler.idl[1] be updated now that `.startPage` is gone?

[1]: https://searchfox.org/mozilla-central/rev/80ac71c1c54a/browser/components/nsIBrowserHandler.idl#12