Closed Bug 924886 Opened 6 years ago Closed 6 years ago

Update Metro session store component to share same session store state as Desktop's session store

Categories

(Firefox for Metro Graveyard :: Components, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: bbondy, Unassigned)

References

Details

(Keywords: meta, Whiteboard: [story] [beta28])

Attachments

(2 files)

Session store is the best way for us to do the "Transfer Environment to Metro/Desktop" feature.
We currently use our own session store component though. We should standardize on using the same one as Desktop Firefox.

To do this we should undo the custom registration: 
http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/components.manifest#l26

Take out the build config:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/moz.build#l34
http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/moz.build#l8

It looks like the only extra thing that Metro's version has is:
boolean shouldRestore();
I think it should be replaced with the property check of canRestoreLastSession

Browser's registration:
http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.manifest#l15
http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/moz.build#l12

There may be some extra unexpected fallout from this as well that I can't predict.
p=3
Summary: Work - Remove custom session store code and use Desktop's version → Story - Remove custom session store code and use Desktop's version
Whiteboard: feature=story u=tbd c=tbd p=3
Whiteboard: feature=story u=tbd c=tbd p=3 → [block28] feature=story u=tbd c=tbd p=3
Assignee: nobody → msamuel
Per discussions on IRC, mfinkle mentioned we should not try to bring the session state code together, it would be more work than expected.

Let's boot this work out until the end and do it if time permits.
Assignee: msamuel → nobody
Removing block-28 flag since we can release without it, but it would be really nice to have parity with chrome for this.
Whiteboard: [block28] feature=story u=tbd c=tbd p=3 → feature=story u=tbd c=tbd p=3
Whiteboard: feature=story u=tbd c=tbd p=3 → [release28] feature=story u=tbd c=tbd p=3
Summary: Story - Remove custom session store code and use Desktop's version → Story - Update Metro session store component to share same session store state as Desktop's session store
[15:33:14] <mfinkle> http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#1517
[15:33:31] <mfinkle> example of desktop SessionStore.jsm not being friendly to Metro
[15:33:43] <mfinkle> it uses aTab
[15:33:55] <mfinkle> and that tab.xml widget
[15:34:33] <mfinkle> http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#1626
[15:34:38] <mfinkle> uses tabbrowser.xml
[15:34:58] <mfinkle> we can't use the same sessionstore.js data between metro and desktop
Adding 5 more points here, I'll subtract 5 points from the contingency bug.
Whiteboard: [release28] feature=story u=tbd c=tbd p=3 → [release28] feature=story u=tbd c=tbd p=8
dupe of bug 886336?
This bug was originally a bug to use Desktop's session store but mfinkle mentioned that's not a good direction to go because Desktop's session store uses a lot of front end code directly that Metro doesn't have. I don't know for sure if that is still the case, but I think mfinkle showed a direct example of it in comment 4.
I ran into an issue when testing shared profiles using the oak build that I believe is related to this bug. When switching between Desktop -> Metro, you'll sometimes receive a completely blank page. Attached a screenshot so we have some reference for QA.
Assignee: nobody → msamuel
Blocks: metrov1it20
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] feature=story u=tbd c=tbd p=8 → [block28] feature=story u=tbd c=tbd p=8
Hi Kamil, were you able to regularly reproduce this issue? If so, would you be able to post STR? If you're not sure how you were able to reproduce it but it happens again, could you attach your sessionatore.js file as soon as it happens? It should be in a path that looks something like this: C:\Users\<username>\AppData\Roaming\Mozilla\Firefox\Profiles\<your profile>\sessionstore.js

Thanks!
Flags: needinfo?(kamiljoz)
Hi Marina,

I'm not really sure how to reproduce this 100% as it happens really rarely and only seen it happen twice after a few days of testing (usually happens when switching between environments). I will definitely keep my eye out for this when going through the shared profiles ticket and will attach sessionatore.js as soon as it happens.
Flags: needinfo?(kamiljoz)
I looked at the sessionstore files saved by both metro and desktop and after some comparison and testing, so far, found these potential issues:

1) Opening a tab on desktop and immediately switching to metro seems to switch before storing the session and thus the tabs won't show up
2) slightly different format for storing closed tabs in metro vs. desktop so opening recently closed tabs doesn't work (easy fix)
3) form data and cookies are not saved in metro
4) if multiple windows are open on desktop, a switch to metro will lose session info for windows that weren't switched over
5) favicons are saved in different formats (though not obvious where these are used when restoring a session)
6) about:start is not valid on desktop
7) about:newtab is not valid on metro

I think all these issues besides #3 and #4 can be fixed relatively quickly and are worthwhile addressing since they're visible to users. Brian, thoughts? Anything you'd like to add here?
Flags: needinfo?(netzen)
Great analysis!

(In reply to Marina Samuel [:emtwo] from comment #11)
> I looked at the sessionstore files saved by both metro and desktop and after
> some comparison and testing, so far, found these potential issues:
> 
> 1) Opening a tab on desktop and immediately switching to metro seems to
> switch before storing the session and thus the tabs won't show up

I think we need to add the code to do the switch a little bit later in the shutdown of the browser process. I've noticed with debug builds sometimes I get a profile in use problem from Desktop to Metro, but never get it the other way around.


> 2) slightly different format for storing closed tabs in metro vs. desktop so
> opening recently closed tabs doesn't work (easy fix)

Great! Probably obvious, but make sure to change the Metro implementation to match Desktop and not the other way around :)

> 3) form data and cookies are not saved in metro
If it's a non trivial amount of work just making sure that the formats are compatible and kicking that part out to a new spawned bug is fine with me. Please post a new bug with that if you want to do that.

> 4) if multiple windows are open on desktop, a switch to metro will lose
> session info for windows that weren't switched over

Could we maybe change the logic of how we read to import all window tabs into the same window on Metro?

> 5) favicons are saved in different formats (though not obvious where these
> are used when restoring a session)
Different formats in which way? (Like do we convert to ICO or something?) Probably consolidate to the way Desktop does it.

> 6) about:start is not valid on desktop
Adding some code to automatically convert that to/from about:newtab I think is the way to go there. 

> 7) about:newtab is not valid on metro
> 
See above.

> I think all these issues besides #3 and #4 can be fixed relatively quickly
> and are worthwhile addressing since they're visible to users. Brian,
> thoughts? Anything you'd like to add here?

Feel free to spawn a new bug for #4 as well.
Please put point estimates in the posted bugs if you want to handle them there instead of here.
Flags: needinfo?(netzen)
(In reply to Brian R. Bondy [:bbondy] from comment #12)
> > 4) if multiple windows are open on desktop, a switch to metro will lose
> > session info for windows that weren't switched over
> 
> Could we maybe change the logic of how we read to import all window tabs
> into the same window on Metro?

Yes, I think that would be the most ideal solution until we have support for multiple windows in metro.

> > 5) favicons are saved in different formats (though not obvious where these
> > are used when restoring a session)
> Different formats in which way? (Like do we convert to ICO or something?)
> Probably consolidate to the way Desktop does it.

I just mean the json structure is different. But it should be possible to change it to the way desktop does it.
k sounds good :)
Whiteboard: [block28] feature=story u=tbd c=tbd p=8 → [release28] feature=story u=tbd c=tbd p=8
Blocks: metrov1it21
No longer blocks: metrov1it20
Whiteboard: [release28] feature=story u=tbd c=tbd p=8 → [beta28] feature=story u=tbd c=tbd p=8
Depends on: 948983
Depends on: 949612
Blocks: 950159
No longer blocks: 950159
Depends on: 950159
Depends on: 950174
Hi Marina,

I was going through iteration #20 testing using the build listed below and ran into the issue I described in comment #8. I basically launched Firefox Desktop and then switched over to Firefox Metro and received a complete blank page without anything loading. I got the file that you mentioned from comment #9 and attached it to this ticket. Let me know if there's anything else I can do to help out.
Depends on: 951725
Assignee: msamuel → nobody
Blocks: metrov1backlog
No longer blocks: metrov1it21
Priority: P2 → --
QA Contact: jbecerra
Whiteboard: [beta28] feature=story u=tbd c=tbd p=8 → [story] [beta28]
Depends on: 956368
Summary: Story - Update Metro session store component to share same session store state as Desktop's session store → Update Metro session store component to share same session store state as Desktop's session store
Status: ASSIGNED → NEW
No longer depends on: 950174
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
No longer blocks: metrov1backlog
Verified fixed with latest Aurora 29.0a2 and latest Beta (28 beta 3) on Win 8 64-bit: when switching between Desktop -> Metro, I don't receive anymore a blank page.
Status: RESOLVED → VERIFIED
Keywords: verifyme
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.