1160732 - Remove about:sync-progress

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

about:sync-progress should use the new in-content style-sheet to be able to remove the old inContentUI.css.

It looks, this is from old sync and will be removed in the future but until then we should still change the style-sheet.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: SyncProgress-old.png

Comment 2

[Richard Marti (:Paenglab)]

Attachment: SyncProgress-new.png

How about:sync-progress would look with new style-sheet.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: sync-progress.patch

I've moved the file to shared as it's on all platforms the same.

Comment 4

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

Comment on attachment 8600572 [details] [diff] [review]
sync-progress.patch

I don't think this file is used anymore, so we can probably just remove it.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8600572 [details] [diff] [review]
sync-progress.patch

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

Yeah, let's just nuke it completely.

Comment 6

[Richard Marti (:Paenglab)]

It's used by progress.xhtml and this is called by https://dxr.mozilla.org/mozilla-central/source/browser/base/content/sync/utils.js#90. Should I use only the css file or also progress.xhtml and all references to it (https://dxr.mozilla.org/mozilla-central/search?q=sync-progress&case=false)?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

None of those call-sites are actually hit, so I think the latter (css, xhtml and all references to it). Most of utils.js can go too, but I don't think we need to go that far.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: sync-progress.patch

Remove it.

I'm not so good in programming. I hope it's correct to remove the whole browser_aboutSyncProgress.js test. If not, which part should be removed from this test?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Looks about right to me!  I've pushed a try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=20c3bb8d7de5

Richard, just a heads-up, incase you have a good reason not to remove this?

Comment 10

[Richard Marti (:Paenglab)]

I forgot to add also my try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1dcb0616af9

Errors on other tests, then it shouldn't be caused by this patch.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Yeah, that try looks good. Let's just wait a couple of days to hear from Richard and if he has no objections (which I doubt he will), I'll r+ then - thanks!

Comment 12

[Richard Newman [:rnewman]]

This is part of the Old Sync setup flow, which I think includes pairing existing devices. A very very small minority of users can still encounter this.

Removing this line:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js#561

will cause the wizard to simply end, without triggering the progress page, and you can then clean up the rest of the code.

Comment 13

[Richard Marti (:Paenglab)]

Thank you, Richard. This line is already removed in my patch.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8603158 [details] [diff] [review]
sync-progress.patch

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

Yeah, that change is already there, so I think this looks good! Thanks!

Comment 15

[Richard Marti (:Paenglab)]

Try runs are in comment 9 and 10.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3fe949180c2c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fe949180c2c