Closed Bug 1235372 Opened 4 years ago Closed 4 years ago

Enable browser e10s tests in browser/components/search/test

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

No description provided.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
These tests seem only to need a check for about:blank
Enable browser_oneOffHeader.js which seems to work as is, and remove need for Promise.jsm
Blocks: e10s-tests
tracking-e10s: --- → +
Attachment #8702275 - Flags: review?(florian)
Attachment #8702276 - Flags: review?(florian)
Attachment #8702277 - Flags: review?(florian)
Attachment #8702278 - Flags: review?(florian)
Attachment #8702282 - Flags: review?(florian)
Comment on attachment 8702275 [details] [diff] [review]
browser_426329.js

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

::: browser/components/search/test/browser_426329.js
@@ +126,5 @@
> +
> +  let focusPromise = BrowserTestUtils.waitForEvent(searchBar, "focus");
> +  gURLBar.focus();
> +  searchBar.focus();
> +  return focusPromise;

I was initially a bit confused by this line returning a promise after the line returning nothing 5 lines above. I wonder if we should make this be |yield focusPromise;| for better readability.

@@ +202,5 @@
>  add_task(function* testRightClick() {
>    preTabNo = gBrowser.tabs.length;
> +  gBrowser.selectedBrowser.loadURI("about:blank");
> +  yield new Promise(resolve => {
> +    setTimeout(function() {

This isn't related to what you are fixing in this bug, but I wonder if there's something we could use here to replace that 5s timer.
Attachment #8702275 - Flags: review?(florian) → review+
Comment on attachment 8702276 [details] [diff] [review]
browser_private_search_perwindowpb.js

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

::: browser/components/search/test/browser_private_search_perwindowpb.js
@@ +13,2 @@
>  
> +  function performSearch(aWin, aIsPrivate) {

Should this become |function* performSearch|?

@@ +25,3 @@
>    }
>  
> +  function addEngine() {

Is there any way this could use promiseNewEngine from head.js?
Attachment #8702276 - Flags: review?(florian) → review+
Attachment #8702277 - Flags: review?(florian) → review+
Comment on attachment 8702278 [details] [diff] [review]
browser_contextmenu.js

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

::: browser/components/search/test/browser_contextmenu.js
@@ +62,5 @@
> +      });
> +      content.document.getSelection().selectAllChildren(content.document.body);
> +    });
> +  });
> +  

nit: trailing whitespace.
Attachment #8702278 - Flags: review?(florian) → review+
Comment on attachment 8702280 [details] [diff] [review]
search_engine_aboutblank

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

I'm assuming you meant to request review here too.
Attachment #8702280 - Flags: review+
Attachment #8702282 - Flags: review?(florian) → review+
You need to log in before you can comment on or make changes to this bug.