Closed Bug 1195743 Opened 5 years ago Closed 5 years ago

Remove old migration step removing bookmarks-menu-button's persisted class attribute

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: dao, Assigned: dao)

Details

Attachments

(1 file)

We're doing this again in step 31.

The step 16 change is just a drive-by fix.
Attachment #8649261 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8649261 [details] [diff] [review]
bookmarks-menu-button.diff

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

::: browser/components/nsBrowserGlue.js
@@ -1983,5 @@
>        // Remove persisted collapsed state from TabsToolbar.
>        xulStore.removeValue(BROWSER_DOCURL, "TabsToolbar", "collapsed");
>      }
>  
> -    if (currentUIVersion < 21) {

Please leave a comment noting 21 was made obsolete by bug <whatever>, see migration <whichever> - doing archaeology on removed code is annoying, and the missing numbers will be confusing otherwise. :-)
Attachment #8649261 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #1)
> Please leave a comment noting 21 was made obsolete by bug <whatever>, see
> migration <whichever> - doing archaeology on removed code is annoying, and
> the missing numbers will be confusing otherwise. :-)

It really shouldn't be confusing. There are lots of other holes and that's expected since old migration steps make sense only so much time. I don't think we should add comments for removed steps.
(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Please leave a comment noting 21 was made obsolete by bug <whatever>, see
> > migration <whichever> - doing archaeology on removed code is annoying, and
> > the missing numbers will be confusing otherwise. :-)
> 
> It really shouldn't be confusing. There are lots of other holes and that's
> expected since old migration steps make sense only so much time. I don't
> think we should add comments for removed steps.

Alright, then ship as-is, I guess.

(I still would love to kill this whole thing for new profiles once upon a time... but not today, I guess)
https://hg.mozilla.org/mozilla-central/rev/13bb29917965
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.