Remove about:sync-progress

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Firefox 41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Blocks: 1160731
Posted image SyncProgress-old.png
Posted image SyncProgress-new.png
How about:sync-progress would look with new style-sheet.
Posted patch sync-progress.patch (obsolete) — Splinter Review
I've moved the file to shared as it's on all platforms the same.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8600572 - Flags: review?(gavin.sharp)
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.
Attachment #8600572 - Flags: review?(gavin.sharp) → review?(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.
Attachment #8600572 - Flags: review?(mhammond) → review-
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)?
Flags: needinfo?(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.
Flags: needinfo?(mhammond)
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?
Attachment #8600572 - Attachment is obsolete: true
Attachment #8603158 - Flags: review?(markh)
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?
Flags: needinfo?(rnewman)
Summary: Let about:sync-progress use the new in-content style-sheet → Remove about:sync-progress
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.
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!
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.
Flags: needinfo?(rnewman)
Thank you, Richard. This line is already removed in my patch.
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!
Attachment #8603158 - Flags: review?(markh) → review+
Keywords: checkin-needed
Try runs are in comment 9 and 10.
https://hg.mozilla.org/integration/fx-team/rev/3fe949180c2c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3fe949180c2c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.