Closed Bug 1240750 Opened 4 years ago Closed 4 years ago

[e10s] Fix and re-enable /layout/base/tests/browser_bug617076.js

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: tracy, Assigned: tracy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

This test case is skip-if = e10s because it touches content (adds a button).  I'll attach a patch shortly, that passes try.  It puts the button add in a ContentTask and modernizes the test case to not use CPOWs.
Attached patch browser_bug617076.patch (obsolete) — Splinter Review
Patched attached for review.

Passed try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a02e9af21974
Attachment #8709459 - Flags: review?(jmathies)
Comment on attachment 8709459 [details] [diff] [review]
browser_bug617076.patch

I would like mconley to review this. generally looks good to me, but mike is far better at finding mochitest test nits than I am.
Attachment #8709459 - Flags: review?(jmathies) → review?(mconley)
Comment on attachment 8709459 [details] [diff] [review]
browser_bug617076.patch

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

This looks great! Thanks Tracy - this test is far easier to read now that you've cleaned it up. I have a few notes - see below. Feel free to ask me if you have any questions about what I've written.

::: layout/base/tests/browser_bug617076.js
@@ +6,5 @@
>   * 5. remove the about:addons tab
>   * 6. remove the blank tab
>   *
>   * the test succeeds if it doesn't trigger any assertions
> +*/

Style nit - one more space indent here so that the stars line up.

@@ +20,1 @@
>      e.setAttribute('label', "hello");

While you're here, would you mind switching the single-quote usage around 'label' and 'tooltiptext' to double-quotes? Just for consistency.

@@ +20,4 @@
>      e.setAttribute('label', "hello");
>      e.setAttribute('tooltiptext', "world");
>      doc.documentElement.insertBefore(e, doc.documentElement.firstChild);
> +    });

Nit - alignment - unindent this two spaces please.

@@ +28,2 @@
>  
> +  function getElement() {

I think we can make this simpler - if we give the button an ID, we can pass that to synthesizeMouse as the first argument using a CSS selector.

Example:

On line 22, do:

e.setAttribute("id", "test-button");

Then, for each synthesizeMouse call, do:

yield BrowserTestUtils.synthesizeMouse("#test-button", ...);

that will do the job of correctly finding the element in the content process, and avoids needing to get at the button via the content CPOW.

I'm actually unsure how this was working before - the test adds a button, but doesn't give it an ID, and then getElement attempts to get at the button CPOW using the id "button", and it shouldn't find anything.

Anyhow, adding the "test-button" ID will probably make this simpler, and you can then get rid of getElement.

@@ +32,5 @@
>  
> +  // Select the testTab then perform mouse events on inserted button
> +  gBrowser.selectedTab = testTab;
> +  let e = getElement;
> +  let win = gBrowser.selectedBrowser;

Nit: Generally speaking, we like to keep variables named like "win" or "window" or "wins" pointing at root browser windows, whereas for things like gBrowser.selectedBrowser, which is a browser inside a tab, we usually call that "browser" or "b".

Can we call this browser instead? I know it's a minor detail, but it helps with readability.

@@ +38,5 @@
> +  try {
> +    yield BrowserTestUtils.synthesizeMouse(e, 1, 1, { type: "mouseover" }, win);
> +    yield BrowserTestUtils.synthesizeMouse(e, 2, 6, { type: "mousemove" }, win);
> +    yield BrowserTestUtils.synthesizeMouse(e, 2, 4, { type: "mousemove" }, win);
> +    } finally {

Nit - busted indentation for this finally block. Please unindent two spaces.

@@ +45,3 @@
>  
> +  // cleanup
> +  gBrowser.removeTab(testTab);

I think we're trying to use more of the asynchronous removal stuff in order to avoid races and side effects with tests that run quickly one after the other.

Can you use:

yield BrowserTestUtils.removeTab(testTab);
yield BrowserTestUtils.removeTab(tab2);

instead please?
Attachment #8709459 - Flags: review?(mconley) → review-
Thanks for the review, Mike.  This patch addresses your comments.
Attachment #8709459 - Attachment is obsolete: true
Attachment #8710083 - Flags: review?(mconley)
Comment on attachment 8710083 [details] [diff] [review]
browser_bug617076-2.patch

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

Assuming this passes for you locally (I didn't apply or run it locally), then r=me! Great job, Tracy!
Attachment #8710083 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> 
> Assuming this passes for you locally (I didn't apply or run it locally),
> then r=me! Great job, Tracy!

Mike thanks again for the review. Yes, it runs fine locally.  Jim mentored me much along the way. 

My head is already spinning trying to figure out next test case in my queue.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ddc6c387b2a1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.