Closed Bug 914258 Opened 6 years ago Closed 6 years ago

Backout the patch for bug 887515 and its dependencies from Firefox 25

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox25 --- verified
firefox26 --- unaffected

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The amount of outstanding regressions based on the feature introduced in bug 887515 is too high and some of the fixes may be best if a new API was introduced (bug 895436).

This bug is to backout the patch for bug 887515 and its follow-up bugs from Firefox 25.
I left the uuid in nsISessionStore.idl unchanged from mozilla-aurora tip since there have been other changes to the file that are not being backed out.
Attachment #801998 - Flags: review?(ttaubert)
Attachment #801998 - Flags: review?(ttaubert) → review+
Comment on attachment 801995 [details] [diff] [review]
backout-35934f1b4a45

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 887515 has too many regressions and not enough time to fix them all on Aurora
User impact if declined: Various bugs with undoing closing tabs.
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): none expected, simple backout
String or IDL/UUID changes made by this patch: these patches had introduced a new string, but never removed the old one. Since this is a backout, there is string removal but the strings that are removed shouldn't mess up localization since the string that will be used in its place was and will still be around.
Attachment #801995 - Flags: approval-mozilla-aurora?
Comment on attachment 801997 [details] [diff] [review]
backout-fdb2be03094d

See above aurora-approval request.
Attachment #801997 - Flags: approval-mozilla-aurora?
Comment on attachment 801998 [details] [diff] [review]
backout-b94553178dfe

See above aurora-approval request.
Attachment #801998 - Flags: approval-mozilla-aurora?
Attachment #801995 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #801997 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #801998 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Jared, is there something QA needs to test here?
Flags: needinfo?(jaws)
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #8)
> Jared, is there something QA needs to test here?

Basically, "Undo Close Tab" should work the same in Firefox 25 as it does in Firefox 24. That is:
1) close a tab and then right-click on another tab and choose "Undo Close Tab". The most recently closed tab should be reopened.
2) right-click on a tab and choose "Close Other Tabs". Then right-click on the tab and choose "Undo Close Tab", and only one of the recently closed tabs should reopen. This menuitem will have to be clicked multiple times (once for each tab that got closed) to reopen all the closed tabs.
Flags: needinfo?(jaws)
Thanks Jared, flagging for verification.
Keywords: verifyme
(In reply to Jared Wein [:jaws] from comment #3)
> I left the uuid in nsISessionStore.idl unchanged from mozilla-aurora tip
> since there have been other changes to the file that are not being backed
> out.

In general, this isn't a good idea. Any binary-incompatible changes (e.g. removing methods) to an IDL must be paired with a change to the IID, otherwise clients can end up crashing.

If the interface definition ends up matching a previous state, it's OK to re-use that previous state's IID, but that doesn't appear to be the case here. So we should bump the IID for nsISessionStore on Aurora, I think.

(It may not matter in practice, I doubt somewhat that there are binary callers to this interface. But it's relatively low cost, so I think worth doing anyways.)
Flags: needinfo?(jaws)
Good call on the backout, btw; good that we didn't lose track of this.
Attachment #805450 - Flags: review?(gavin.sharp) → review+
Comment on attachment 805450 [details] [diff] [review]
Followup patch to rev the UUID for Fx25

[Approval Request Comment]
Bug caused by (feature/regressing bug #): followup for this bug
User impact if declined: potential binary incompatibility due to incorrect UUID
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

This should have been done with the aurora-uplifted patches, but those have now made their way to beta so this will need to be uplifted to beta.
Attachment #805450 - Flags: approval-mozilla-beta?
Too bad we missed this for beta 1, I guess I should have approved for Aurora when I r+ed. We should take this now on beta for b2, I think.
Comment on attachment 805450 [details] [diff] [review]
Followup patch to rev the UUID for Fx25

Because we're UUID frozen on beta, and there likely weren't any major clients of the new UUID yet, we're going to leave it as is.
Attachment #805450 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed on Firefox 25 beta 3 (buildID: 20130926170421).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.