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

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Places
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
the test should really run in content, probably can be converted to a mochitest-plain or mochitest-browser.

Updated

2 years ago
tracking-e10s: --- → +
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Comment 2

2 years ago
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
(Reporter)

Comment 3

2 years ago
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.
(Reporter)

Comment 4

2 years ago
Sorry, I mistyped most of the comment. The point is this test should be rewritten as a mochitest-browser.
(Assignee)

Comment 5

2 years ago
Panos asked me to pick this up.
Assignee: mak77 → nhnt11
(Reporter)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8730517 [details] [diff] [review]
Patch
Attachment #8730517 - Flags: review?(mak77)
(Assignee)

Comment 8

2 years ago
Created attachment 8730521 [details] [diff] [review]
Patch

Rebased.
Attachment #8730517 - Attachment is obsolete: true
Attachment #8730517 - Flags: review?(mak77)
Attachment #8730521 - Flags: review?(mak77)
(Reporter)

Comment 9

2 years ago
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)
(Assignee)

Comment 10

2 years ago
Created attachment 8730820 [details] [diff] [review]
Patch v2

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)
(Reporter)

Comment 11

2 years ago
(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 :)
(Reporter)

Comment 12

2 years ago
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+
(Assignee)

Comment 13

2 years ago
Created attachment 8731477 [details] [diff] [review]
Patch v3
Attachment #8730820 - Attachment is obsolete: true
Attachment #8731477 - Flags: review+
(Assignee)

Updated

2 years ago
Depends on: 1255066
Keywords: checkin-needed

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/632931aedcbb
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/632931aedcbb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.