Closed Bug 1254592 Opened 5 years ago Closed 5 years ago

[e10s] fix test_history_post.xul to run in content and remove duped test_bug_94514.html

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: mak, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

the test should really run in content, probably can be converted to a mochitest-plain or mochitest-browser.
I think we should go with a mochitest-plain with loadChromeScript (bug 1251139 is about to land)
the test should submit the form, wait for the iframe to be loaded, then wait for the chrome script to check the Places database.
while here, we should also remove test_bug_94514.html and bug94514-postpage.html, since they are testing the same thing...
Summary: [e10s] test_history_post.xul should run in content → [e10s] fix test_history_post.xul to run in content and remove duped test_bug_94514.html
In the end, this would end up being the only mochitest-plain in Places... I'd to limit the harnesses we can fail so let's go with mochitest-browser instead.
Sorry, I mistyped most of the comment. The point is this test should be rewritten as a mochitest-browser.
Panos asked me to pick this up.
Assignee: mak77 → nhnt11
Feel free to take it and thank you. I didn't start working on it, apart from looking if it was needed to convert it for e10s.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8730517 - Flags: review?(mak77)
Attached patch Patch (obsolete) — Splinter Review
Rebased.
Attachment #8730517 - Attachment is obsolete: true
Attachment #8730517 - Flags: review?(mak77)
Attachment #8730521 - Flags: review?(mak77)
Comment on attachment 8730521 [details] [diff] [review]
Patch

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

Looks good, still something to fix, will have a quick second look.

::: toolkit/components/places/tests/browser/browser_history_post.js
@@ +1,1 @@
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");

this should not be needed case head.js does that already

@@ +3,5 @@
> +const PAGE_URI = "http://example.com/tests/toolkit/components/places/tests/browser/history_post.html";
> +const SJS_URI = NetUtil.newURI("http://example.com/tests/toolkit/components/places/tests/browser/history_post.sjs");
> +
> +add_task(function* () {
> +  yield BrowserTestUtils.withNewTab({gBrowser: gBrowser, url: PAGE_URI}, Task.async(function* (aBrowser) {

I think you can avoid passing gBrowser, if no browser is specified it will default to gBrowser.

@@ +18,5 @@
> +      submit.click();
> +      yield p;
> +    });
> +    let history = Cc["@mozilla.org/browser/history;1"]
> +                    .getService(Ci.mozIAsyncHistory);

This is not necessary, just use PlacesUtils.history where needed.

@@ +20,5 @@
> +    });
> +    let history = Cc["@mozilla.org/browser/history;1"]
> +                    .getService(Ci.mozIAsyncHistory);
> +    yield new Promise((resolve, reject) => {
> +      history.isURIVisited(SJS_URI, function (aURI, aIsVisited) {

you can use promiseIsURIVisited from head.js

@@ +24,5 @@
> +      history.isURIVisited(SJS_URI, function (aURI, aIsVisited) {
> +        ok(!aIsVisited, "The POST page should not be added to history");
> +
> +        let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +                            .DBConnection;

Please let's use the async API.

let db = yield PlacesUtils.promiseDBConnection();
let rows = yield db.execute(query, { page_url: SJS_URI.spec });
ok(rows.length, 0, ...)
Attachment #8730521 - Flags: review?(mak77)
Attached patch Patch v2 (obsolete) — Splinter Review
Thanks!

(In reply to Marco Bonardo [::mak] from comment #9)
> @@ +3,5 @@
> > +const PAGE_URI = "http://example.com/tests/toolkit/components/places/tests/browser/history_post.html";
> > +const SJS_URI = NetUtil.newURI("http://example.com/tests/toolkit/components/places/tests/browser/history_post.sjs");
> > +
> > +add_task(function* () {
> > +  yield BrowserTestUtils.withNewTab({gBrowser: gBrowser, url: PAGE_URI}, Task.async(function* (aBrowser) {
> 
> I think you can avoid passing gBrowser, if no browser is specified it will
> default to gBrowser.

Following the code path from [1], I don't think so...
[1] https://dxr.mozilla.org/mozilla-central/rev/d6ee82b9a74155b6bfd544166f036fc572ae8c56/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#65
Attachment #8730521 - Attachment is obsolete: true
Attachment #8730820 - Flags: review?(mak77)
(In reply to Nihanth Subramanya [:nhnt11] from comment #10)
> Following the code path from [1], I don't think so...
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> d6ee82b9a74155b6bfd544166f036fc572ae8c56/testing/mochitest/BrowserTestUtils/
> BrowserTestUtils.jsm#65

ah, you're right, that code doesn't make sense imo, but ok :)
Comment on attachment 8730820 [details] [diff] [review]
Patch v2

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

thanks, lgtm.

::: toolkit/components/places/tests/browser/browser_history_post.js
@@ +1,5 @@
> +const PAGE_URI = "http://example.com/tests/toolkit/components/places/tests/browser/history_post.html";
> +const SJS_URI = NetUtil.newURI("http://example.com/tests/toolkit/components/places/tests/browser/history_post.sjs");
> +
> +add_task(function* () {
> +  yield BrowserTestUtils.withNewTab({gBrowser: gBrowser, url: PAGE_URI}, Task.async(function* (aBrowser) {

nit: just gBrowser (in ES6 when defining an object you can just type {gBrowser} and it will be the same as {gBrowser: gBrowser})

::: toolkit/components/places/tests/browser/history_post.html
@@ +9,5 @@
> +      <input type="submit" id="submit"/>
> +    </form>
> +    <p id="display"></p>
> +    <div id="content" style="display:none;"></div>
> +    <pre id="test"></pre>

nit: The <p>, <div> and <pre> are likely pointless in a mochitest-browser test since the harness doesn't use them.
Attachment #8730820 - Flags: review?(mak77) → review+
Attached patch Patch v3Splinter Review
Attachment #8730820 - Attachment is obsolete: true
Attachment #8731477 - Flags: review+
Depends on: 1255066
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/632931aedcbb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.