Closed Bug 1434167 Opened 3 years ago Closed 2 years ago

Remove getSourceHomePageURL from migrator code

Categories

(Firefox :: Migration, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: dthayer, Assigned: Gijs)

References

Details

Attachments

(2 files)

This is a follow-up for Bug 1433024.

Verdi, do you know if there's any reason to keep the logic where we import the user's homepage during migration? It only occurs under some weird, branding-specific circumstances. Right now there is an IE implementation, a Safari implementation, and a Chrome implementation, and as far as I can tell, the Chrome implementation is broken since Chrome home pages don't work the same anymore. So that leaves Safari and IE. Do we know how people generally use their custom homepages and whether the use they have for their custom home pages appropriately transfers to FF?
Flags: needinfo?(mverdi)
Also note that when we added that migration code, our own homepage was far less useful.
(In reply to Doug Thayer [:dthayer] from comment #0)
> This is a follow-up for Bug 1433024.
> 
> Verdi, do you know if there's any reason to keep the logic where we import
> the user's homepage during migration? 

I don’t believe so. We removed this from auto migration because we didn’t think it was useful anymore.
Flags: needinfo?(mverdi)
I'm not sure why this is marked as critical (possibly related to bug 1433024) but marking as P3 for now. Feel free to adjust.
Priority: -- → P3
Woops! I missed that copying it over from bug 1433024.
Severity: critical → minor
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8948936 [details]
Bug 1434167 - remove home page code from the migration wizard,

https://reviewboard.mozilla.org/r/218362/#review224116

::: browser/components/migration/content/migration.js:380
(Diff revision 1)
>            // We're done now.
>            this._wiz.canAdvance = true;
>            this._wiz.advance();
>  
>            setTimeout(close, 5000);
>          } else {
>            this._wiz.canAdvance = true;
>            var nextButton = this._wiz.getButton("next");
>            nextButton.click();

The only reason I'm asking you rather than Doug for review here, Matt, is whether you (given history etc.) have any idea why these two blocks are different. As far as I can tell they do the same thing minus the setTimeout (ie I should think clicking the next button is equivalent to `this._wiz.advance()`), so we could just:

```js
this._wiz.canAdvance = true;
this._wiz.advance();
if (this._autoMigrate) {
  setTimeout(close, 5000);
}
```

and simplify a bit further.

I tried looking at (CVS...) history but AFAICT this has been like this since Ben Goodger wholesale landed this pile of imported code some 12-15 years ago. Perhaps I'm missing something, though equally perhaps I'm trying to find logic where there simply is none.
Comment on attachment 8948935 [details]
Bug 1434167 - remove getSourceHomePageURL implementations and callsites,

https://reviewboard.mozilla.org/r/218360/#review224118

::: browser/components/migration/content/migration.js:330
(Diff revision 1)
>      var appName = MigrationUtils.getBrowserName(this._source);
>  
>      // semi-wallpaper for crash when multiple profiles exist, since we haven't initialized mSourceProfile in places
>      this.spinResolve(this._migrator.getMigrateData(this._selectedProfile, this._autoMigrate));
>  
> -    var oldHomePageURL = this.spinResolve(this._migrator.getSourceHomePageURL());
> +    var oldHomePageURL = null;

I tidy up the rest of this in the second patch. I can unify them before landing, I just figured it would be easier to review if I separated the two things.
Oops, missed the telemetry histogram removal. Adding that to the second cset...
Comment on attachment 8948935 [details]
Bug 1434167 - remove getSourceHomePageURL implementations and callsites,

https://reviewboard.mozilla.org/r/218360/#review224268
Attachment #8948935 - Flags: review?(dothayer) → review+
Comment on attachment 8948936 [details]
Bug 1434167 - remove home page code from the migration wizard,

https://reviewboard.mozilla.org/r/218362/#review224116

> The only reason I'm asking you rather than Doug for review here, Matt, is whether you (given history etc.) have any idea why these two blocks are different. As far as I can tell they do the same thing minus the setTimeout (ie I should think clicking the next button is equivalent to `this._wiz.advance()`), so we could just:
> 
> ```js
> this._wiz.canAdvance = true;
> this._wiz.advance();
> if (this._autoMigrate) {
>   setTimeout(close, 5000);
> }
> ```
> 
> and simplify a bit further.
> 
> I tried looking at (CVS...) history but AFAICT this has been like this since Ben Goodger wholesale landed this pile of imported code some 12-15 years ago. Perhaps I'm missing something, though equally perhaps I'm trying to find logic where there simply is none.

Hmm… I don't have enough experience with <wizard>s to know for sure that there is no difference in the two branches… if you manually test and see no difference then I'm fine with simplifying but on the other hand I'm fine leaving it as-is with what "works" rather than potentially regressing something. Up to you.
Comment on attachment 8948936 [details]
Bug 1434167 - remove home page code from the migration wizard,

https://reviewboard.mozilla.org/r/218362/#review224372
Attachment #8948936 - Flags: review?(MattN+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9a70d4fe4a06
remove getSourceHomePageURL implementations and callsites, r=dthayer
https://hg.mozilla.org/integration/autoland/rev/5f7d0a8085a8
remove home page code from the migration wizard, r=MattN
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #12)
> Hmm… I don't have enough experience with <wizard>s to know for sure that
> there is no difference in the two branches… if you manually test and see no
> difference then I'm fine with simplifying but on the other hand I'm fine
> leaving it as-is with what "works" rather than potentially regressing
> something. Up to you.

Leaving this for now, then. Thanks!
Blocks: 475068
Blocks: 354967
Blocks: 769466
Blocks: 1200849
Blocks: 735183
Blocks: 1210401
Blocks: 718189
https://hg.mozilla.org/mozilla-central/rev/9a70d4fe4a06
https://hg.mozilla.org/mozilla-central/rev/5f7d0a8085a8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.