From bug 1261225: (In reply to :Margaret Leibovic from bug 1261225 comment #15) > Thanks for the test! This looks good to me, although it could be worth > filing a follow-up to investigate a way to avoid the sleep statements, > especially if we plan to add more session restore tests (we should!). > > ::: mobile/android/tests/browser/chrome/test_session_form_data.html:256 > (Diff revision 3) > > + // Note: Gecko currently doesn't restore the inner form data after navigation, > > + // so we can't check for the inner form value. > > + todo_is(getInputValue(browser, {id: "txt", frame: 0}), INNER_VALUE, "inner value present after navigation"); > > + > > + // Allow the session store a bit of time to actually capture the form data. > > + yield sleep(CLOSE_TAB_WAIT); > > This makes me a bit nervous... could we add a notificaiton somewhere in the > session restore code to help make tests more deterministic?

Review commit: https://reviewboard.mozilla.org/r/44691/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44691/

Comment on attachment 8738735 [details] MozReview Request: Bug 1262565 - Replace usage of timeouts in session store form data test with waiting for appropriate events. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44691/diff/1-2/

Comment on attachment 8738735 [details] MozReview Request: Bug 1262565 - Replace usage of timeouts in session store form data test with waiting for appropriate events. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44691/diff/2-3/

Comment on attachment 8738735 [details] MozReview Request: Bug 1262565 - Replace usage of timeouts in session store form data test with waiting for appropriate events. r=margaret https://reviewboard.mozilla.org/r/44691/#review41813 ::: mobile/android/components/SessionStore.js:438 (Diff revision 3) > // As _collectTabData() doesn't save any form data, we need to manually > // capture it to bridge the time until the next input event arrives. > this.onTabInput(aWindow, aBrowser); > } > > + let evt = new Event("TabSessionDataUpdated", {"bubbles":true, "cancelable":false}); I was imagining using a Services.obs notification here... but this seems reasonable. I started looking at the desktop code for inspiration to see what they do: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm This code has changed a lot since it was forked a long time ago! It seems like they using notifications, but also have some similar events: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1711 Their events are all "SS" prefixed, maybe we should do the same thing for these events? However, this seems like it's fine enough to land as-is if that's what you want to do.

https://reviewboard.mozilla.org/r/44691/#review41813 > I was imagining using a Services.obs notification here... but this seems reasonable. I started looking at the desktop code for inspiration to see what they do: > https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm > > This code has changed a lot since it was forked a long time ago! > > It seems like they using notifications, but also have some similar events: > https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1711 > > Their events are all "SS" prefixed, maybe we should do the same thing for these events? However, this seems like it's fine enough to land as-is if that's what you want to do. Thanks for the feedback. My decision to use events was partially down to not being all that familiar yet with the various ways of passing notifications around and partially because we've already got those helper methods in head.js which are set up to work with events. The prefix doesn't sound like a bad idea, I'll change the event names before landing.

Comment on attachment 8738735 [details] MozReview Request: Bug 1262565 - Replace usage of timeouts in session store form data test with waiting for appropriate events. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44691/diff/3-4/