Backout the patch for bug 887515 and its dependencies from Firefox

VERIFIED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Firefox
Menus
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26+ verified, firefox27+ verified, firefox28 verified, firefox29 verified, b2g-v1.2 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

26.06 KB, patch
ttaubert
: review+
jaws
: checkin+
Details | Diff | Splinter Review
15.56 KB, application/zip
Details
31.95 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
31.93 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
30.98 KB, patch
jaws
: review+
bsmedberg
: feedback+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #914258 +++

The patch for bug 896291 didn't get uplifted to Aurora in time for the mozilla-aurora -> mozilla-beta merge, and there is also an add-on compat issue that is being tracked in bug 895436.

As such, I'd like to back out the patches for bug 887515 and its dependencies from Fx26 and Fx27 (removing it from non-mc branches). This will give me the full Fx28 Nightly cycle to make the necessary API changes.
Created attachment 823535 [details] [diff] [review]
Backout from Fx26

Backout of the patches, similar to bug 914258. I put in a new UUID for nsISessionStore.idl as we did in bug 914258.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Didn't get bug 895436 fixed for Fx26 (will likely introduce an API change) and didn't get the patch for bug 896291 uplifted to Fx26.
User impact if declined: Data loss if the user accidentally closes too many tabs since bug 896291 wasn't uplifted and potential issues with add-ons due to bug 895436.
Testing completed (on m-c, etc.): locally, ran it through mochitest-browser suite.
Risk to taking this patch (and alternatives if risky): minor risk, as it was not an automated backout, although I did not have to do any major changes, just had to manually fix some of the backout hunk failures.
String or IDL/UUID changes made by this patch: string removals, but no new strings and the strings that will get referenced after this patch is applied were never removed and aren't new, so there should be net no string changes.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #823535 - Flags: review?(ttaubert)
Attachment #823535 - Flags: approval-mozilla-beta?
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: --- → +
tracking-firefox27: --- → +
fwiw, you'll want to ask for mozilla-approval uplift here too now that 27 is on Aurora.
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #2)
> fwiw, you'll want to ask for mozilla-approval uplift here too now that 27 is
> on Aurora.

I believe you meant 'mozilla-aurora approval'. Yes, I intend to do so, I still need to put together the backout patch for 27.
Comment on attachment 823535 [details] [diff] [review]
Backout from Fx26

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

Tests are succeeding and the patch looks good!
Attachment #823535 - Flags: review?(ttaubert) → review+
Attachment #823535 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: verifyme
This makes IDL changes. Can't land without ba+ which I don't see explicitly granted here.
Flags: needinfo?(jaws)
Jorge, can I get your approval to make this IDL change on beta?
Flags: needinfo?(jaws) → needinfo?(jorge)
Looks good to me. Which beta will this ship with?
Flags: needinfo?(jorge)
Beta 2 :)

https://hg.mozilla.org/releases/mozilla-beta/rev/2e7ce68c821f
status-firefox26: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2e7ce68c821f
status-b2g-v1.2: --- → fixed
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed in 26 beta 1 (buildID: 20131104182142).
status-firefox26: fixed → verified
Created attachment 8344725 [details]
Zip of separate patches for backout from m-c
Created attachment 8344731 [details] [diff] [review]
Folded patch of all the backout changesets from Fx29 (m-c)

This is the backout patch for 887515 and the followups. It also includes backouts of the patches from bug 910167 and bug 898732 because they touched the same code. Those patches can be rewritten and relanded before this feature is reimplemented.
Attachment #8344731 - Flags: review?(ttaubert)
Summary: Backout the patch for bug 887515 and its dependencies from Firefox 26 and Firefox 27 → Backout the patch for bug 887515 and its dependencies from Firefox 26, Firefox 27, Firefox 28
Attachment #8344731 - Attachment description: Folded patch of all the backout changesets from Fx28 (m-c) → Folded patch of all the backout changesets from Fx28/Fx29 (m-c)
Created attachment 8344756 [details] [diff] [review]
Folded patch of all the backout changesets from Fx28 (Aurora)
Attachment #8344756 - Flags: review?(ttaubert)
Attachment #8344756 - Attachment description: Folded patch of all the backout changesets from Fx28 (Holly) → Folded patch of all the backout changesets from Fx28 (Aurora)
Attachment #8344731 - Attachment description: Folded patch of all the backout changesets from Fx28/Fx29 (m-c) → Folded patch of all the backout changesets from Fx29 (m-c)
Comment on attachment 8344731 [details] [diff] [review]
Folded patch of all the backout changesets from Fx29 (m-c)

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

The patch looks good to me but I don't agree that we should backout bug 910167 and bug 898732 even though it might make the backout easier. I think we can use the patch here and just replace all nsISessionStore usages again. That's a very low-risk change.

::: browser/base/content/browser-places.js
@@ +481,5 @@
>    // browser.js to halt on "PlacesMenu is not defined" error.
>    this.__proto__.__proto__ = PlacesMenu.prototype;
> +  XPCOMUtils.defineLazyServiceGetter(this, "_ss",
> +                                     "@mozilla.org/browser/sessionstore;1",
> +                                     "nsISessionStore");

We could also keep the changes from bug 910167 here.

::: browser/base/content/browser-tabview.js
@@ +72,2 @@
>  
> +      let data = sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);

I think we could really keep using SessionStore.jsm instead of nsISessionStore here.

::: browser/base/content/browser.js
@@ +6077,5 @@
>  
> +  var tab = null;
> +  var ss = Cc["@mozilla.org/browser/sessionstore;1"].
> +           getService(Ci.nsISessionStore);
> +  if (ss.getClosedTabCount(window) > (aIndex || 0)) {

Let's keep SessionStore.jsm here and in the rest of the file.

::: browser/base/content/tabbrowser.xml
@@ +2189,5 @@
>              // and apply it to our tab.
>              if (isPending) {
> +              let ss = Cc["@mozilla.org/browser/sessionstore;1"]
> +                         .getService(Ci.nsISessionStore)
> +              ss.setTabState(aOurTab, ss.getTabState(aOtherTab));

Let's keep SessionStore here.

@@ +2620,5 @@
>          <body>
>            <![CDATA[
> +            return Cc["@mozilla.org/browser/sessionstore;1"]
> +                     .getService(Ci.nsISessionStore)
> +                     .duplicateTab(window, aTab);

And here.
Attachment #8344731 - Flags: review?(ttaubert) → feedback+
Comment on attachment 8344756 [details] [diff] [review]
Folded patch of all the backout changesets from Fx28 (Aurora)

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

I would do the same here and keep the SessionStore.jsm usages. It shouldn't be too hard to merge those conflicts, is it?
Attachment #8344756 - Flags: review?(ttaubert) → feedback+
This missed 3 trains, shouldn't we just back it out of central?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #16)
> This missed 3 trains, shouldn't we just back it out of central?

Yes, that is the plan. See attachment #8344731 [details] [diff] [review].
Summary: Backout the patch for bug 887515 and its dependencies from Firefox 26, Firefox 27, Firefox 28 → Backout the patch for bug 887515 and its dependencies from Firefox
Created attachment 8346793 [details] [diff] [review]
Folded patch of the backout csets from m-c (Fx29)

This patch includes the SessionStore changes reintroduced on top of the backout.
Attachment #8344731 - Attachment is obsolete: true
Attachment #8346793 - Flags: review?(ttaubert)
Created attachment 8346809 [details] [diff] [review]
Folded patch of the backout csets from Aurora (Fx28)
Attachment #8344756 - Attachment is obsolete: true
Attachment #8346809 - Flags: review?(ttaubert)
Created attachment 8346813 [details] [diff] [review]
Backout from Fx27 Beta
Attachment #8346813 - Flags: review?(ttaubert)
Attachment #8346793 - Flags: review?(ttaubert) → review+
Attachment #8346809 - Flags: review?(ttaubert) → review+
Attachment #823535 - Flags: checkin+
Attachment #8346813 - Flags: review?(ttaubert) → review+
Comment on attachment 8346813 [details] [diff] [review]
Backout from Fx27 Beta

Jorge, can I get binary-approval from you for landing this on beta? It changes the nsISessionStore.idl file.
Attachment #8346813 - Flags: review?(jorge)
Since we've already shipped beta1, can we avoid the IDL change by just leaving the methods there and making them throw NS_ERROR_NOT_IMPLEMENTED?
we can even make it [noscript] without changing the IDL so that it won't show up as available to JS extensions.
Does this make it harder for JS extensions to find out that their extension may be broken?
I don't think so, with the [noscript]
Pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/1566f020e34e

Pushed to holly:
https://hg.mozilla.org/projects/holly/rev/af8f2d2baab8
status-firefox28: --- → affected
status-firefox29: --- → fixed
Comment on attachment 8346809 [details] [diff] [review]
Folded patch of the backout csets from Aurora (Fx28)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 887515 never had all of its deps fixed
User impact if declined: issues with add-ons that track tab behavior
Testing completed (on m-c, etc.): locally, just pushed to fx-team
Risk to taking this patch (and alternatives if risky): tab tracking issues, but this is a pretty straightforward backout of the tree of patches
String or IDL/UUID changes made by this patch: IID change in nsISessionStore.idl
Attachment #8346809 - Flags: approval-mozilla-aurora?
Created attachment 8347412 [details] [diff] [review]
Backout from Fx27 Beta with no IID change

Carrying forward r+ from ttaubert. Flagging bsmedberg to make sure that I made the IDL and JSM changes that he was describing so it won't need ba+ for Beta.
Attachment #8346813 - Attachment is obsolete: true
Attachment #8346813 - Flags: review?(jorge)
Attachment #8347412 - Flags: review+
Attachment #8347412 - Flags: feedback?(benjamin)
Attachment #8347412 - Flags: feedback?(benjamin) → feedback+
Comment on attachment 8347412 [details] [diff] [review]
Backout from Fx27 Beta with no IID change

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not all deps for bug 887515 were fixed
User impact if declined: potential issues with add-ons that focus on tab state/restoring
Testing completed (on m-c, etc.): locally, just landed on fx-team
Risk to taking this patch (and alternatives if risky): tab-related issues, but it was a pretty straightforward backout of the patch tree from 887515.
String or IDL/UUID changes made by this patch: IID remained the same by changing implementation of functions to throw a NOT_IMPLEMENTED exception
Attachment #8347412 - Flags: approval-mozilla-beta?
Backed out on fx-team due to mochitest-bc orange. It looks like some tests were expecting that there would be no warning when closing multiple tabs.

https://hg.mozilla.org/integration/fx-team/rev/b41e80b8b109
Backed out on Holly for same bc orange, https://hg.mozilla.org/projects/holly/rev/fe17a76ae37a
Repushed to fx-team and holly with the test fixed:
https://hg.mozilla.org/integration/fx-team/rev/4e1073ed5389
https://hg.mozilla.org/projects/holly/rev/ff302db28367
Attachment #8346809 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8347412 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8346809 [details] [diff] [review]
Folded patch of the backout csets from Aurora (Fx28)

drats, i was premature - will wait for successful landing on central.
Attachment #8346809 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Attachment #8347412 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/4e1073ed5389
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Attachment #8346809 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8347412 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7e5a0bf9500
https://hg.mozilla.org/releases/mozilla-beta/rev/5e31e19fb269

To please the commit hook, I used ba=bsmedberg on the beta patch per the comments in the bug.
status-firefox27: affected → fixed
status-firefox28: affected → fixed
Backed out from Aurora for mochitest-bc failures and re-landed with the test fix.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6895de3528c9
For the record, it would have been much nicer to backout the functionality without touching strings on Aurora and Beta (obsolete/unused string would have been removed riding the train from central).
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0

Verified that the fixes from bug 887515 in 27 beta 2 (buildID: 20131216183647).
status-firefox27: fixed → verified
Verified this bug on FF 28 build id:20131217004003 using the following environments:

Ubuntu 12.04 x32
Windows xp x86
Os X 10.9
status-firefox28: fixed → verified

Comment 40

3 years ago
Verified as fixed on Firefox 29.0 beta 7, on Windows 7 64bit, Mac OS X 10.9.2 and Ubuntu 13.04 32bit.

I did notice an issue here but I'm not sure if it's related to this fix or not, or if it's already tracked anywhere:

When I uncheck "Warn me when closing multiple tabs", then use "Close Other Tabs", I still get the warning. This doesn't happen on builds before the backout here (e.g. Firefox 27), but it does happen on builds before that fix (e.g. Firefox 24).
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.