Closed Bug 950174 Opened 6 years ago Closed 6 years ago

Save form data and scroll position in Metro session store

Categories

(Firefox for Metro Graveyard :: General, defect, P2)

x86_64
Windows 8
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: emtwo, Assigned: emtwo)

References

Details

(Whiteboard: p=5 s=it-30c-29a-28b.2 r=ff30 [qa+])

Attachments

(2 files, 6 obsolete files)

Currently cookies and form data are extra properties that desktop saves in the session store that metro does not. Since the Metro sessionstore.js format is still compatible with desktop without these, this is probably low priority for now.
Blocks: 924886
Whiteboard: [triage]
Whiteboard: [triage] → [beta28]
Whiteboard: [beta28] → [work] [beta28]
Whiteboard: [work] [beta28] → [work] [beta28] p=0
Assignee: nobody → msamuel
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [work] [beta28] p=0 → [beta28] p=5
Blocks: metrov1it22
No longer blocks: metrov1it21
Whiteboard: [beta28] p=5 → [beta28] [feature] p=5
The basic save and restore for formdata is working with this patch. Some remaining things:

* Scroll position isn't saving/restoring. There was some incompatiblity when I tried the desktop version for this so need to continue looking at that
* Desktop uses caching to avoid frequently re-polling for form data. Perhaps a quicker alternative to this would be only saving form data when you close/restart your browser (instead of when you open a new tab, etc)
* Need to do more thorough testing to ensure no regressions. Looked ok with some quick tests
* Need to add comments if there's anything unclear
Attachment #8360582 - Flags: feedback?(mbrubeck)
Attachment #8360573 - Attachment description: Copy some required session store files from browser/ to toolkit/modules → Part 1: Copy some required session store files from browser/ to toolkit/modules
Attachment #8360582 - Attachment description: WIP: Part 1: Form data save and restore → WIP: Part 2: Form data save and restore
Comment on attachment 8360582 [details] [diff] [review]
WIP: Part 2: Form data save and restore

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

This looks good to me and I think it's the correct approach, but for Firefox 28 I'm worried about risk due to (1) the amount of code involved, (2) making changes to toolkit and desktop, and (3) lack of tests for the Metro code.  I think we should retarget this to Fx29 or 30, and focus on getting some tests running for our code.
Attachment #8360582 - Flags: feedback?(mbrubeck) → feedback+
Comment on attachment 8360573 [details] [diff] [review]
Part 1: Copy some required session store files from browser/ to toolkit/modules

For the final version of this patch, we should use "hg mv" to preserve the change history from the files' original locations.
Assignee: msamuel → nobody
Blocks: metrobacklog, 957399
No longer blocks: metrov1it22
QA Contact: jbecerra
Whiteboard: [beta28] [feature] p=5 → [feature] p=0
No longer blocks: 924886
Status: ASSIGNED → NEW
No longer blocks: 957399
Whiteboard: [feature] p=0 → p=5
Assignee: nobody → msamuel
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: p=5 → p=5 s=it-30c-29a-28b.1 r=ff30
Attachment #8360573 - Attachment is obsolete: true
Attachment #8375651 - Flags: review?(ttaubert)
Attachment #8360582 - Attachment is obsolete: true
Attachment #8375654 - Flags: review?(mbrubeck)
Comment on attachment 8375651 [details] [diff] [review]
Part 1: v2: Move required session store files from browser/ to toolkit/modules

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

This looks good. I wonder, should we rename the modules to denote that they're used for SessionStore implementations? I think better would be to create a sessionstore folder to put these in, we will hopefully want to move more files and that would just clutter the modules/ dir more.

::: toolkit/modules/moz.build
@@ +52,5 @@
>  ]
>  
>  EXTRA_PP_JS_MODULES += [
>      'CertUtils.jsm',
> +    'FormData.jsm',

This should be moved to EXTRA_JS_MODULES.
Attachment #8375651 - Flags: review?(ttaubert) → feedback+
Comment on attachment 8375654 [details] [diff] [review]
Part 2: v2: Form data save and restore

Changing this to feedback since I'm adding scroll position save/restore soon as well. You can wait til I get that in before giving feedback too.
Attachment #8375654 - Flags: review?(mbrubeck) → feedback?(mbrubeck)
Addressed comments and moved ScrollPosition.jsm too
Attachment #8375651 - Attachment is obsolete: true
Attachment #8375831 - Flags: review?(ttaubert)
Attachment #8375654 - Attachment is obsolete: true
Attachment #8375654 - Flags: feedback?(mbrubeck)
Attachment #8375835 - Flags: review?(mbrubeck)
Attachment #8375831 - Attachment description: Part 1: v3: Move required session store files from browser/ to toolkit/modulesmove-sessionstore-files → Part 1: v3: Move required session store files from browser/ to toolkit/modules
Comment on attachment 8375831 [details] [diff] [review]
Part 1: v3: Move required session store files from browser/ to toolkit/modules

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

Please don't camel-case the directory name. I forgot to mention that toolkit/components/sessionstore would be a better place, sorry.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +15,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
>  Cu.import("resource://gre/modules/Timer.jsm", this);
> +Cu.import("resource://gre/modules/FormData.jsm", this);
> +Cu.import("resource://gre/modules/ScrollPosition.jsm", this);

Let's keep loading those modules lazily, please.

::: browser/components/sessionstore/src/ContentRestore.jsm
@@ +10,5 @@
>  const Ci = Components.interfaces;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
> +Cu.import("resource://gre/modules/FormData.jsm", this);
> +Cu.import("resource://gre/modules/ScrollPosition.jsm", this);

Load these lazily as well.

::: toolkit/modules/moz.build
@@ +10,5 @@
>  MOCHITEST_CHROME_MANIFESTS += ['tests/chrome/chrome.ini']
>  
> +PARALLEL_DIRS += [
> +    'sessionstore'
> +]

Please ask a build system peer (e.g. :gps) for review on build file changes.
Attachment #8375831 - Flags: review?(ttaubert) → feedback+
Hey Greg, could you please take a look at the build changes in this patch? Thanks!
Attachment #8375831 - Attachment is obsolete: true
Attachment #8376306 - Flags: review?(ttaubert)
Attachment #8376306 - Flags: review?(gps)
Comment on attachment 8375835 [details] [diff] [review]
Part 2: v3: Form data and scroll position save and restore

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

Looks good!  One possible cleanup:

::: browser/metro/components/SessionStore.js
@@ +937,5 @@
>              // Restore current title
>              tab.chromeTab.updateTitle(tabData.entries[tabData.index - 1].title);
>            }
>  
> +          tab.browser.__SS_tabFormData = tabData

This duplicates the __SS_data property.  Can we use __SS_data everywhere, and just check it for ".formdata" and ".scroll" properties before trying to read them?
Attachment #8375835 - Flags: review?(mbrubeck) → review+
Comment on attachment 8376306 [details] [diff] [review]
Part 1: v4: Move required session store files from browser/ to toolkit/modules

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

You can likely merge the contents of the new moz.build into the existing one and everything will behave the same. We try to keep the number of moz.build files in check to make the build config more concentrated and faster to read. In the future, we'll likely fail builds if superfluous moz.build files exist.
Attachment #8376306 - Flags: review?(gps) → review+
Attachment #8376306 - Flags: review?(ttaubert) → review+
Comment on attachment 8375835 [details] [diff] [review]
Part 2: v3: Form data and scroll position save and restore

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

::: browser/metro/base/content/bindings/browser.js
@@ +471,5 @@
> +      case "SessionStore:restoreSessionTabData":
> +        if (message.json.formdata)
> +          FormData.restore(content, message.json.formdata);
> +        if (message.json.scroll)
> +          ScrollPosition.restore(content, message.json.scroll.scroll);

Please note that the .restore() functions are more or less deprecated and we kept them only to maintain backwards compatibility with older session files. In Fx Desktop sessionstore we nowadays use a frame tree that maintains a list of all reachable frames at the point when the page has finished loading. We only collect and restore data for these frames to avoid hassle with pages that creates lots of dynamic frames, see bug 934935 and bug 952998.

I think your patch is fine as is though as you are using collect() only on the top frame and then call restore() to restore that data. This should be fast because it ignores all subframes but isn't quite how Fx Desktop works. We of course don't need to deprecated the restore() functions, if that's how Metro is meant to work then that's okay.

::: browser/metro/components/SessionStore.js
@@ +367,4 @@
>        case "TabOpen":
>          this.updateTabTelemetryVars(window);
> +        let browser = aEvent.originalTarget.linkedBrowser;
> +        browser.addEventListener("load", this, true)

Nit: missing semicolon
Whiteboard: p=5 s=it-30c-29a-28b.1 r=ff30 → p=5 r=ff30
hey Greg, thanks for the review. I tried to merge the new moz.build into the old one by adding the 3 files into toolkit/modules/moz.build under EXTRA_JS_MODULES, and removed PARALLEL_DIRS but I got these errors: https://pastebin.mozilla.org/4329613 what else needs to be done to merge them correctly? Thanks!
Flags: needinfo?(gps)
QA Contact: jbecerra → kamiljoz
Whiteboard: p=5 r=ff30 → p=5 s=it-30c-29a-28b.2 r=ff30
I'm not sure what could be causing that error!

Please post the updated patch.
Flags: needinfo?(gps)
Attached patch Attempt to merge moz.build files (obsolete) — Splinter Review
I think I'm missing something that tells the build file where to look for XPathGenerator.jsm, FormData.jsm and ScrollPosition.jsm. I couldn't find another build file that does this without using PARALLEL_DIRS
Attachment #8377844 - Flags: feedback?(gps)
Comment on attachment 8377844 [details] [diff] [review]
Attempt to merge moz.build files

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

::: toolkit/modules/moz.build
@@ +18,5 @@
>      'Deprecated.jsm',
>      'Dict.jsm',
>      'FileUtils.jsm',
>      'Finder.jsm',
> +    'FormData.jsm',

You need to prefix these with "sessionstore/"
Attachment #8377844 - Flags: feedback?(gps)
(In reply to Matt Brubeck (:mbrubeck) from comment #13)
> This duplicates the __SS_data property.  Can we use __SS_data everywhere,
> and just check it for ".formdata" and ".scroll" properties before trying to
> read them?

I think we can't do that because __SS_data is deleted/reset here: 
http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/SessionStore.js#511

The form data can only be restored after a tab loads and at that point __SS_data no longer contains the form data to be restored.
Attachment #8377844 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/2e754d2d9a42
https://hg.mozilla.org/integration/fx-team/rev/dc6f5c320e68
Whiteboard: p=5 s=it-30c-29a-28b.2 r=ff30 → [fixed-in-fx-team] p=5 s=it-30c-29a-28b.2 r=ff30
Summary: Save form data and cookies in Metro session store → Save form data and scroll position in Metro session store
https://hg.mozilla.org/mozilla-central/rev/2e754d2d9a42
https://hg.mozilla.org/mozilla-central/rev/dc6f5c320e68
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] p=5 s=it-30c-29a-28b.2 r=ff30 → p=5 s=it-30c-29a-28b.2 r=ff30
Target Milestone: --- → Firefox 30
Flagged for testing and verification.
Flags: needinfo?(kamiljoz)
Went through the following verification process using the following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-23-03-02-04-mozilla-central/

Auto-fill Test Cases:

Used the following website to test the basic functionality of form data being saved between the fxmetro and fxdesktop sessions
- http://autofillforms.mozdev.org/test.html

- Added data into the mozdev forms using fxmetro and than ensured that the same data was available under fxdesktop
- Added data into the mozdev forms using dxdesktop and ensured that the same data was available under fxdesktop
- Ensured text fields, radio buttons, check marks, text area's, drop down menu's are all saved when switching between environments
- Ensured that the password field is not being saved when moving through fxmetro and fxdesktop
- Went through the above cases a few times, switching back and forth between the two environments and ensured that the same data was always available
- Went through several different websites and ensured that the form data is being saved for both fxmetro and fxdesktop (Facebook, Twitter, GitHub, LinkedIn, Ebay, Amazon and many others)
- Ensured that all the form data is being saved once submitted on both fxmetro and fxdesktop environments
- Ensured that all the saved from data is still available once the browser is closed/re-opened (tested on both fxmetro & fxdesktop)
- Ensured that updating the browser doesn't remove the saved form data on both fxmetro & fxdesktop
- Went through all of the above test cases in several different variations of snapped view

Scroll Positions Test Cases:

- Loaded several news websites and switched between fxmetro and fxdesktop and ensured that the website loaded in the same general area
- Ensured that when you have several tabs opened, switching between the two environments saves the scroll positions for all the tabs opened
- Used several different websites while testing the scroll position portion (Facebook, Bloomberg, Twitter, Wikipedia, Polygon, Yahoo News, Wall Street Journal and many others)
- Went through all of the above test cases in several different variations of snapped view

Marina, I have a few questions before marking this as verified. Everything is working very well and haven't ran into any serious issues while testing this for a few hours on a few different machines. However, I did run into these possible issues:

- I noticed that when you're switching between the two environments, the scroll positions will sometimes be off a little. I've noticed this happen many times using different websites. Sometimes its off a few sentences, sometimes its off a paragraph or so. It's never really off by a large margin but I just wanted to make sure this is expected and not considered an issues. I assumed that the difference between the scrolling positions could be because of the differences in window size??

- Social media websites such as Facebook and Twitter don't work very well when it comes to saving scroll positions. When you switch between environments, the page loads and than scrolls to the bottom where more of the content is loaded and displayed. However, when the page reaches this section, it stops and doesn't scroll further down to the correct position after more content has been loaded and displayed. I'm not sure if there's anything we can do to fix this, should I create a new bug for this?
Flags: needinfo?(kamiljoz) → needinfo?(msamuel)
Thanks for the very thorough testing, Kamil.

In terms of the social media website issue, I think desktop Firefox may have this same issue. If I adjust my desktop settings to "show my windows and tabs from last time" and I have a Facebook tab open, closing and reopening Firefox will not scroll me to what I was looking at before. Can you confirm this is the issue you're seeing on Metro as well? If so, This is related to the way those websites frequently update the content at given scroll positions and it isn't a Metro-specific issue.

In terms of the slightly offset scroll positions, I think you're right that it's likely due to window size changes. Could you please file that as a separate bug? Thanks!
Flags: needinfo?(msamuel)
No Problem!

I went through the first issue with fxdesktop and ran into the same problem with Facebook and Twitter. When you attempt to restore the session, it will only move you to the bottom of the page. Once more data is loaded by the website, you won't be moved further down to the correct position. Like you mentioned, not a metro-specific issue!

Created Bug #976301 for the offset scroll positions.

Thanks for the feedback Marina!
Status: RESOLVED → VERIFIED
Whiteboard: p=5 s=it-30c-29a-28b.2 r=ff30 → p=5 s=it-30c-29a-28b.2 r=ff30 [qa+]
You need to log in before you can comment on or make changes to this bug.