1262565 - Avoid sleep statements in session store form data test

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

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?

Comment 1

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1262565 - Replace usage of timeouts in session store form data test with waiting for appropriate events. r=margaret

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

Comment 2

[Jan Henning [:JanH]]

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 3

[Jan Henning [:JanH]]

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 4

[:Margaret Leibovic]

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.

Comment 5

[Jan Henning [:JanH]]

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 6

[Jan Henning [:JanH]]

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/

Comment 7

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e11fcb607f

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef111ba6ddf3