699573 - expose needHomePageOverride logic (ability to detect upgrade/firstrun) in a more useful way

Status: NEW

(Firefox :: General, defect)

Description

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

Currently the needHomePageOverride function in nsBrowserContentHandler runs every time someone gets defaultArgs, which includes e.g. opening a new window. This is unnecessary, and means that we have various unrelated things shoved into this function for convenience (like the AboutHomeUtils stuff), since it's the only place we do checks for upgrade/first run scenarios.

I propose creating a new module that will track this state specifically, and allow it to be queried from elsewhere so that code that wants to handle upgrade/new profile scenarios can live wherever it wants.

Comment 1

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

Attachment: patch

Comment 2

[:Margaret Leibovic]

Comment on attachment 571779 [details] [diff] [review]
patch

Looks good to me. Like I said on IRC, you probably just want to run browser_bug538331.js to make sure nothing got unintentionally busted.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 571779 [details] [diff] [review]
patch

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

::: browser/modules/RunState.jsm
@@ +41,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +let EXPORTED_SYMBOLS = ["RunState"];
> +
> +var RunState = {

Nit: Use "let" for consistency?

Comment 4

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

(In reply to Margaret Leibovic [:margaret] from comment #2)
> Looks good to me. Like I said on IRC, you probably just want to run
> browser_bug538331.js to make sure nothing got unintentionally busted.

Turns out the move to making .state a lazy getter did bust that test, so I've reverted that for now until we find a different solution to that test (my future startup refactorings will probably impact this).

Comment 5

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #4)
> Turns out the move to making .state a lazy getter did bust that test, so
> I've reverted that for now

... except that that's going to mess up the use of this module by other consumers, so that isn't a suitable solution. I'm going to have to fix the test now, I guess.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Any update on this bug? I am working on bug 532150, which seems to depend on this one.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.