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

NEW
Unassigned

Status

()

defect
8 years ago
10 months ago

People

(Reporter: Gavin, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Depends on: 699575
Posted patch patchSplinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #571779 - Flags: review?(mnoorenberghe+bmo)
Attachment #571779 - Flags: review?(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.
Attachment #571779 - Flags: review?(margaret.leibovic) → review+
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?
Attachment #571779 - Flags: review?(mnoorenberghe+bmo) → review+
(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).
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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.
Any update on this bug? I am working on bug 532150, which seems to depend on this one.
Assignee: gavin.sharp → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.