1434167 - Remove getSourceHomePageURL from migrator code

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

[Alex Thayer [:alexical] (she/her)]

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?

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Also note that when we added that migration code, our own homepage was far less useful.

Comment 2

[Verdi [:verdi] Best to slack me]

(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.

Comment 3

[Matthew N. [:MattN]]

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.

Comment 4

[Alex Thayer [:alexical] (she/her)]

Woops! I missed that copying it over from bug 1433024.

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1434167 - remove getSourceHomePageURL implementations and callsites,

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

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1434167 - remove home page code from the migration wizard,

This removes all the homepage-related code. All the cases where we advance the wizard straight to
the homepage wizardpage now go straight to the 'start migrating' page (the 'point' was skipping
earlier pages that allowed users to select items).
The brand bundle was only used by the homepage code so is also being removed.

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

Comment 7

[:Gijs (he/him)]

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 8

[:Gijs (he/him)]

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.

Comment 9

[:Gijs (he/him)]

Oops, missed the telemetry histogram removal. Adding that to the second cset...

Comment 10

[:Gijs (he/him)]

Comment on attachment 8948936 [details]
Bug 1434167 - remove home page code from the migration wizard,

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

Comment 11

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8948935 [details]
Bug 1434167 - remove getSourceHomePageURL implementations and callsites,

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

Comment 12

[Matthew N. [:MattN]]

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 13

[Matthew N. [:MattN]]

Comment on attachment 8948936 [details]
Bug 1434167 - remove home page code from the migration wizard,

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[:Gijs (he/him)]

(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!

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9a70d4fe4a06
https://hg.mozilla.org/mozilla-central/rev/5f7d0a8085a8