864041 - Remove Firefox 2+3 compat code from about:sessionrestore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Michael Kohler [:mkohler]]

There is Firefox 2 and Firefox 3 compat code in nsSessionStartup. I think we can remove this.

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 739989 [details] [diff] [review]
Patch v1

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

This looks good but we should do the same in browser/components/sessionstore/content/aboutSessionRestore.js. Also, we should remove the test for bug 581593 which is just about this backwards compatibility.

Comment 3

[Michael Kohler [:mkohler]]

Attachment: Patch v2

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 766359 [details] [diff] [review]
Patch v2

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

::: browser/components/sessionstore/content/aboutSessionRestore.js
@@ +24,5 @@
>      gStateObject = JSON.parse(sessionData.value);
>    }
>    catch (exJSON) {
>      var s = new Cu.Sandbox("about:blank", {sandboxName: 'aboutSessionRestore'});
> +    gStateObject = Cu.evalInSandbox(sessionData.value, s);

I think it would be sufficient if we just do "gStateObject = JSON.parse(sessionData.value);" without a try statement. If sessionData.value is invalid we should just throw an error and there is nothing to show on the page anyway.

Comment 5

[Michael Kohler [:mkohler]]

Attachment: Patch v3

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 771786 [details] [diff] [review]
Patch v3

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

Thanks!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 771786 [details] [diff] [review]
Patch v3

Sorry, revoking my r+ for now. It seems that this breaks browser/components/sessionstore/test/browser_586068-apptabs.js for some reason.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

This does also fail when running the whole test suite:

/browser/components/sessionstore/test/browser_601955.js | first tab should not be pinned yet

Both tests seem to do fine when run in single mode.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

The code in nsSessionStartup has been removed already by another patch. What's left is to remove the rest of it in about:sessionrestore, just like Michael's patch did already.

Comment 10

[Nathan Yee [:nyee]]

Attachment: bug-864041-fix

This is the fix for the only Firefox 2+3 backwards compatibility code I have managed to find as of now.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8454845 [details] [diff] [review]
bug-864041-fix

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

Hi,
I think you should still remove test browser_581593.js, remove the try/catch (keeping just the JSON.parse call).

Basically here we are undoing Bug 581593 (most of http://hg.mozilla.org/mozilla-central/rev/c04353b79cfa), that is what the previous patch attached here was doing, apart from the nsSessionStartup changes.

Also, considered the previous attempt failed on some tests, we need a Try server run... Do you have Commit level 1 access? If not, just attach the patch and needinfo me, I will push there for you.

Comment 12

[Nathan Yee [:nyee]]

Attachment: bug-864041-fix

Comment 13

[Nathan Yee [:nyee]]

Attachment: bug-864041-fix

Comment 14

[Nathan Yee [:nyee]]

I do not currently have Commit level 1 access. Do you happen to know any way I could be granted this?

Comment 15

[Nathan Yee [:nyee]]

Reminder comment for someone to push the patch to try server.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Sorry, I totally missed that I'm the mentor for this bug :/ Marco was so kind to take over while I was away. I'll push it to try in a minute, Nathan. Thank you for working on this!

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=79817cd6c767

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8457416 [details] [diff] [review]
bug-864041-fix

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

Try says it's good to go!

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Is this checkin-needed then?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Yes.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/5afb5ea1d0d2

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5afb5ea1d0d2