Closed Bug 1262565 Opened 4 years ago Closed 4 years ago

Avoid sleep statements in session store form data test

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(1 file)

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 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/
Attachment #8738735 - Flags: review?(margaret.leibovic) → review+
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/
https://hg.mozilla.org/mozilla-central/rev/ef111ba6ddf3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.