Closed
Bug 1160732
Opened 10 years ago
Closed 10 years ago
Remove about:sync-progress
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: Paenglab, Assigned: Paenglab)
References
Details
Attachments
(3 files, 1 obsolete file)
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
How about:sync-progress would look with new style-sheet.
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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 5•10 years ago
|
||
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-
Assignee | ||
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
Thank you, Richard. This line is already removed in my patch.
Comment 14•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 15•10 years ago
|
||
Try runs are in comment 9 and 10.
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
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.
Description
•