Closed Bug 721211 Opened 8 years ago Closed Last year

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: Gavin, Assigned: Pike)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Attached patch updated patch (obsolete) — Splinter Review
Assignee: nobody → gavin.sharp
Attachment #591673 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Whiteboard: p=0
Iteration: 36.1 → ---
Attached patch rebased patch (obsolete) — Splinter Review
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?
Attachment #8489120 - Attachment is obsolete: true
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.
(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
Flags: needinfo?(dolske)
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.
Assignee: gavin.sharp → nobody
Status: ASSIGNED → NEW
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.
Flags: needinfo?(dolske)
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.
Assignee: nobody → l10n
Flags: needinfo?(mconnor)
Attachment #8708394 - Attachment is obsolete: true
Attachment #8708394 - Flags: review?(dolske)
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.
Attachment #8708395 - Flags: review?(hskupin) → review+
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.
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().
Attached patch refactored patchSplinter Review
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.
Attachment #8706413 - Attachment is obsolete: true
Flags: needinfo?(mconnor)
Attachment #8715778 - Flags: review?(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?
Attachment #8715778 - Flags: review?(dolske) → review-
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;
(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.
Flags: needinfo?(robert.strong.bugs)
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.
Flags: needinfo?(robert.strong.bugs)
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, '.')
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.
Flags: needinfo?(dolske)
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.
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.
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.
(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.
Flags: needinfo?(dolske)
Duplicate of this bug: 1463544
Attachment #8708395 - Attachment is obsolete: true
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 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. :)
Attachment #8983012 - Flags: review?(dolske) → review?(jaws)
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?
Attachment #8983012 - Flags: review?(jaws) → review+
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
Note, on my try push I also triggered zh-CN builds, and verified that the homepage customization in there still works.
Pushed by axel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb08bc2f2956
consolidate home page retrieval code, make it support non-localized prefs, r=jaws
https://hg.mozilla.org/mozilla-central/rev/bb08bc2f2956
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1490339
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
You need to log in before you can comment on or make changes to this bug.