Work - Add Content Selection Browser Chrome Tests

RESOLVED FIXED in Firefox 23

Status

Firefox for Metro
Tests
P5
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 23
x86_64
Windows 8.1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [selection])

Attachments

(6 attachments, 15 obsolete attachments)

4.25 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
7.18 KB, patch
rsilveira
: review+
Details | Diff | Splinter Review
11.81 KB, patch
rsilveira
: review+
Details | Diff | Splinter Review
8.68 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
11.88 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
16.60 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
Comment hidden (empty)

Updated

6 years ago
Summary: Add content selection browser chrome tests → Work - Add content selection browser chrome tests
Whiteboard: feature=work

Updated

5 years ago
Blocks: 831985
(Assignee)

Comment 1

5 years ago
Created attachment 718987 [details] [diff] [review]
patch v.1
Attachment #718987 - Flags: review?(sfoster)
Comment on attachment 718987 [details] [diff] [review]
patch v.1

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

I got some test failures when I ran this: 

mochitest-metro-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | copy text test - Got the world, expected The rabbit
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | selection test - Got nothing so VER, expected nothing so VERY
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | selection test - Got nothing so VERY remarkable in that; nor did Alice think it so, expected nothing so VERY remarkable in that; nor did Alice think it so

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | selection test - Got down, expected moment
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | Test timed out

... I saw your comments on bugs found, are these expected failures?

See notes on https://bugzilla.mozilla.org/show_bug.cgi?id=846685 and the proposed setUp, run, tearDown fixture methods.

::: browser/metro/base/tests/browser_selection_tests_02.html
@@ +6,5 @@
> +
> +<body>
> +
> +<form action="texarea.html" method="submit" autocomplete="on">
> +<center>

Nit: Center? Really? :) To avoid moments of mild nausea in our fellow front-enders lets use margin: 0 auto; on the textarea and the other centered-things in these tests

::: browser/metro/base/tests/head.js
@@ +580,5 @@
>        info(gCurrentTest.desc);
>        yield Task.spawn(gCurrentTest.run);
>        info("END "+gCurrentTest.desc);
> +      if (gCurrentTest.cleanup)
> +        gCurrentTest.cleanup();

See https://bugzilla.mozilla.org/show_bug.cgi?id=846685, I'll attach a patch there which extends our test lifecycle to support optional, optionally async setUp and tearDown method. If you don't mind making the same change in your tests we'll avoid some conflicts when I land my stuff :)
Attachment #718987 - Flags: review?(sfoster) → review-
(Assignee)

Comment 3

5 years ago
For the test failures, were you able to get those reliably over a few test runs? I'm curious if these are in random orange territory or if there's something different between our machines that cause this.
I ran the tests 3 times and they seemed to fail in the same place each time.
(Assignee)

Comment 5

5 years ago
Created attachment 720440 [details] [diff] [review]
test helpers v.1

Lots of little tweaks to try and get these running more reliably.
Attachment #718987 - Attachment is obsolete: true
Attachment #720440 - Flags: review?(sfoster)
(Assignee)

Comment 6

5 years ago
Created attachment 720442 [details] [diff] [review]
tests v.1
Attachment #720442 - Flags: review?(sfoster)
(Assignee)

Comment 7

5 years ago
Created attachment 720443 [details] [diff] [review]
html
(Assignee)

Comment 8

5 years ago
Created attachment 720444 [details] [diff] [review]
updated html

- updated per comments
Attachment #720443 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 720794 [details] [diff] [review]
check for landscape mode

Chatted with mbrubeck about this on irc. I think your test failures are due to running the tests in portrait mode. If not please let me know. Since these tests require a wide screen, we're going to opt out of them if someone runs them in a mode other than landscape.
Attachment #720794 - Flags: review?(sfoster)
Comment on attachment 720794 [details] [diff] [review]
check for landscape mode

>+  info("browser_selection_tests need landscape mode to run.");

Could you change this from "info(...)" to "todo(false, ...)"?  Just so it'll be a little more visible in the summary when the tests complete.
Comment on attachment 720440 [details] [diff] [review]
test helpers v.1

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

Just some nits and thoughts below, looks good otherwise.

::: browser/metro/base/tests/head.js
@@ +102,5 @@
>  
> +function showNavBar()
> +{
> +  let promise = waitForEvent(Elements.tray, "transitionend");
> +  if (!ContextUI.isVisible) {

It seems odd to create a promise but only resolve it in the if clause. This triggers a warning on sometimes having no return value. However, provided showNavBar is called via a Task.spawn it should handle either eventuality

@@ +110,5 @@
> +}
> +
> +function fireAppBarDisplayEvent()
> +{
> +  let promise = waitForEvent(Elements.tray, "transitionend");

This makes me think that the appbar should provide its own shown/hidden events rather than making us hook into transitionend (and it you look at ContextUI you'll see there's a further setTimeout after that). But, works for now

@@ +554,5 @@
> +    EventUtils.synthesizeTouchAtPoint(this._endPoint.xPos, this._endPoint.yPos,
> +                                      { type: "touchend" }, this._win);
> +    purgeEventQueue();
> +    this._win = null;
> +  },

nit: trailing comma.

@@ +598,5 @@
>        }
> +      try {
> +        yield Task.spawn(gCurrentTest.run.bind(gCurrentTest));
> +      } catch (ex) {
> +        ok(false, "gCurrentTest.run", ex.message);

All tests should have a .desc property, so we can improve the error message here by including that in the output
Attachment #720440 - Flags: review?(sfoster) → review+
Comment on attachment 720794 [details] [diff] [review]
check for landscape mode

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

Looks good.
Attachment #720794 - Flags: review?(sfoster) → review+
(Assignee)

Comment 13

5 years ago
We were wondering yesterday if you were running these in landscape mode. Was that the case? I did so and ended up with failures similar to to the log you posted to pastebin yesterday.
(Assignee)

Updated

5 years ago
Component: Input → Tests
(Assignee)

Comment 14

5 years ago
I think I found a flaw in this that when fixed smooths things out. We don't treat touchup like a touchmove when calculating selection in the page. So in the tests, if the difference between TouchDragAndHold's last touchmove and touchend is wide enough such that we don't snap to the right word, tests might fail. DOM's snap seems a bit unpredictable in general. I'll try to work up a test case for this a file a bug on selection. In the mean time though I'm testing some fixes that should improve the reliability of these tests.
(Assignee)

Comment 15

5 years ago
on my surface pro, which was failing pretty bad:

Browser Chrome Test Summary
	Passed: 81
	Failed: 0
	Todo: 0

woot!
(Assignee)

Updated

5 years ago
Attachment #720442 - Attachment is obsolete: true
Attachment #720442 - Flags: review?(sfoster)
Comment on attachment 720794 [details] [diff] [review]
check for landscape mode

Could you make the browser_context_menu_tests.js bail out on non-landscape screens too?
(Assignee)

Comment 17

5 years ago
These failures all seem to stem from latency in message manager. The tests rely on selection manager values (selection state, monocle xPos) while walking through steps in each test.

I've tried compensating for this using purgeEventQueue calls, but this doesn't guarantee all mm events are delivered since one mm message can trigger additional message that a call to purgeEventQueue won't pick up on.

I'll try to add additional state getters to the selection code to help compensate for this.
(Assignee)

Comment 18

5 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> Comment on attachment 720794 [details] [diff] [review]
> check for landscape mode
> 
> Could you make the browser_context_menu_tests.js bail out on non-landscape
> screens too?

sure, will do.
(Assignee)

Comment 19

5 years ago
Created attachment 721716 [details] [diff] [review]
changes to selection handlers

This adds better SelectionHandler state tracking to SelectionHelperUI.
Attachment #721716 - Flags: review?(mbrubeck)
(Assignee)

Comment 20

5 years ago
Created attachment 721717 [details] [diff] [review]
tests v.2
Attachment #721717 - Flags: review?(sfoster)
(Assignee)

Comment 21

5 years ago
Created attachment 721720 [details] [diff] [review]
rollup
(Assignee)

Comment 22

5 years ago
Created attachment 721725 [details] [diff] [review]
rollup

rollup with misc. nits from reviews addressed. I also removed some old commenting in the selection helpers.
Attachment #721720 - Attachment is obsolete: true
Comment on attachment 721716 [details] [diff] [review]
changes to selection handlers

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

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +51,5 @@
>    _selectionMoveActive: false,
>    _lastMarker: "",
>    _debugOptions: { dumpRanges: false, displayRanges: false },
>    _snap: true,
> +  _steadyState: [],

Can we use a counter instead of an array here?

Basically this is nonzero if there is an operation in progress, and zero otherwise, right?  Could you add a brief comment explaining that?
Attachment #721716 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 24

5 years ago
Created attachment 722409 [details] [diff] [review]
rollup v.2

Changes include:
- listening for scroll completion on page and frame scrolls
- upping various timeouts and requesting more time for the overall test via requestLongerTimeout (for debug builds)
- decreasing the number of touch events sent in TouchDragAndHold to cut down on latency
- added "browser normalize time" header tests for each page load
Attachment #721725 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
also - 
- added start screen visible check on first page load
Comment on attachment 722409 [details] [diff] [review]
rollup v.2

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

See inline comments, and test failures at: http://pastebin.mozilla.org/2201668

::: browser/metro/base/tests/browser_selection_tests.js
@@ +169,5 @@
> +
> +    // check for active selection
> +    is(getTrimmedSelection(gWindow).toString(), "", "selection test");
> +  },
> +  tearDown: function tearDown() {

Would it be worth calling addTab in the setUp for each test and starting with a clean sheet? (and kill the tab in tearDown)

@@ +558,5 @@
> +      return SelectionHelperUI.isHandlerSteady;
> +      }, kCommonWaitMs, kCommonPollMs);
> +
> +    // active state
> +    is(SelectionHelperUI.isActive, true, "selection active");

ok() not is()

@@ +658,5 @@
> +    yield hideContextUI();
> +
> +    gWindow = Browser.selectedTab.browser.contentWindow;
> +
> +    yield waitForMs(3000);

what are we waiting for here? addTab waits for pageshow - is that too early maybe?

::: browser/metro/base/tests/head.js
@@ +106,5 @@
>  }
>  
> +function showNavBar()
> +{
> +  let promise = waitForEvent(Elements.tray, "transitionend");

Note that ContextUI has another setTimeout in its transitionend handler, which calls ContentAreaObserver.updateContentArea, not sure if that's useful or relevant? 
I think the promise should be returned in either case, just setTimeout 0 and resolve it if isVisible is true

@@ +115,5 @@
> +}
> +
> +function fireAppBarDisplayEvent()
> +{
> +  let promise = waitForEvent(Elements.tray, "transitionend");

We were kicking around an idea for ContextUI.dismiss to return a promise that resolves when it is truly done dimissing and transitioning, see #849015
Attachment #722409 - Flags: review-
(Assignee)

Comment 27

5 years ago
(In reply to Sam Foster [:sfoster] from comment #26)
> Comment on attachment 722409 [details] [diff] [review]
> rollup v.2
> 
> Review of attachment 722409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See inline comments, and test failures at:
> http://pastebin.mozilla.org/2201668
> 
> ::: browser/metro/base/tests/browser_selection_tests.js
> @@ +169,5 @@
> > +
> > +    // check for active selection
> > +    is(getTrimmedSelection(gWindow).toString(), "", "selection test");
> > +  },
> > +  tearDown: function tearDown() {
> 
> Would it be worth calling addTab in the setUp for each test and starting
> with a clean sheet? (and kill the tab in tearDown)

I think that would make things even worse. Opening tabs brings up the context ui and in debug builds, can take a very very long time to complete. The browser also needs to stabilize afterward.

> 
> @@ +558,5 @@
> > +      return SelectionHelperUI.isHandlerSteady;
> > +      }, kCommonWaitMs, kCommonPollMs);
> > +
> > +    // active state
> > +    is(SelectionHelperUI.isActive, true, "selection active");
> 
> ok() not is()
> 
> @@ +658,5 @@
> > +    yield hideContextUI();
> > +
> > +    gWindow = Browser.selectedTab.browser.contentWindow;
> > +
> > +    yield waitForMs(3000);
> 
> what are we waiting for here? addTab waits for pageshow - is that too early
> maybe?

open tab overhead. Really not needed on release builds, but debug builds need a lot of time to "calm down" before the test runs.

> 
> ::: browser/metro/base/tests/head.js
> @@ +106,5 @@
> >  }
> >  
> > +function showNavBar()
> > +{
> > +  let promise = waitForEvent(Elements.tray, "transitionend");
> 
> Note that ContextUI has another setTimeout in its transitionend handler,
> which calls ContentAreaObserver.updateContentArea, not sure if that's useful
> or relevant? 
> I think the promise should be returned in either case, just setTimeout 0 and
> resolve it if isVisible is true
> 
> @@ +115,5 @@
> > +}
> > +
> > +function fireAppBarDisplayEvent()
> > +{
> > +  let promise = waitForEvent(Elements.tray, "transitionend");
> 
> We were kicking around an idea for ContextUI.dismiss to return a promise
> that resolves when it is truly done dimissing and transitioning, see #849015

sounds good.
Depends on: 849015
(Assignee)

Comment 28

5 years ago
Maybe after we get tests running in automation and have the time to work on test stability we can revisit this. But currently there's no way tests this complex can run reliably on mc.
Assignee: jmathies → nobody
(Assignee)

Updated

5 years ago
Attachment #721717 - Flags: review?(sfoster)

Updated

5 years ago
Blocks: 842108
Priority: -- → P1
Summary: Work - Add content selection browser chrome tests → Change - Add Content Selection Browser Chrome Tests
Whiteboard: feature=work → feature=change c=Content_features u=metro_firefox_user p=0

Updated

5 years ago
Blocks: 841214
No longer blocks: 842108
Priority: P1 → P5
(Assignee)

Updated

5 years ago
Blocks: 851388
(Assignee)

Updated

5 years ago
Whiteboard: feature=change c=Content_features u=metro_firefox_user p=0 → feature=change c=Content_features u=metro_firefox_user p=0 [selection]
(Assignee)

Updated

5 years ago
Blocks: 852090
No longer blocks: 851388

Updated

5 years ago
No longer blocks: 841214
Summary: Change - Add Content Selection Browser Chrome Tests → Work - Add Content Selection Browser Chrome Tests
Whiteboard: feature=change c=Content_features u=metro_firefox_user p=0 [selection]
(Assignee)

Updated

5 years ago
Attachment #720444 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #721716 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #721717 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #722409 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #720794 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #720440 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 854203
(Assignee)

Updated

5 years ago
Whiteboard: [selection]
(Assignee)

Comment 29

5 years ago
Created attachment 738632 [details] [diff] [review]
part 1 - remove old state tracking

Remove some old test related state tracking I'm replacing with better helper routines.
Assignee: nobody → jmathies
(Assignee)

Updated

5 years ago
Attachment #738632 - Flags: review?(sfoster)
(Assignee)

Comment 30

5 years ago
Created attachment 738634 [details] [diff] [review]
head helpers

Matt, you already approved most of this a while back. I've added one helper - isDebugBuild(), which I'm using to disable selection tests for debug build test runs.
Attachment #738634 - Flags: review?(mbrubeck)
(Assignee)

Comment 31

5 years ago
Created attachment 738636 [details] [diff] [review]
selection test helpers

This adds some props on SelectionHelperUI for better tracking of state, and a neat ping/pong utility call that can be used to insure all message manager messages have been flushed.
Attachment #738636 - Flags: review?(mbrubeck)
(Assignee)

Comment 32

5 years ago
Created attachment 738637 [details] [diff] [review]
basic selection tests v.1

Basic content selection tests
Attachment #738637 - Flags: review?(sfoster)
(Assignee)

Comment 33

5 years ago
Created attachment 738639 [details] [diff] [review]
text area selection tests v.1
Attachment #738639 - Flags: review?(rsilveira)
(Assignee)

Comment 34

5 years ago
Created attachment 738640 [details] [diff] [review]
sub frame content tests v.1
Attachment #738640 - Flags: review?(rsilveira)
(Assignee)

Comment 35

5 years ago
I have more of these in the works, but I'd like to get the basics landed first.
Comment on attachment 738632 [details] [diff] [review]
part 1 - remove old state tracking

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

Looks straightforward.
Attachment #738632 - Flags: review?(sfoster) → review+
Comment on attachment 738634 [details] [diff] [review]
head helpers

For simplicity, I'd prefer to just disable these tests at build time by skipping them in Makefile.in:

  ifndef MOZ_DEBUG
  BROWSER_TESTS += ...
  endif
Attachment #738634 - Flags: review?(mbrubeck) → review-
(Assignee)

Comment 38

5 years ago
Created attachment 738682 [details] [diff] [review]
part 2 - head helpers v.2

I think I can carry over the original r+ then from the previous review. Feel free to look this over if you like.
Attachment #738634 - Attachment is obsolete: true
(Assignee)

Comment 39

5 years ago
(In reply to Jim Mathies [:jimm] from comment #38)
> Created attachment 738682 [details] [diff] [review]
> head helpers v.2
> 
> I think I can carry over the original r+ then from the previous review. Feel
> free to look this over if you like.

My bad, it was from sfoster: review+.
(Assignee)

Updated

5 years ago
Attachment #738682 - Attachment description: head helpers v.2 → head helpers v.2 [sfoster:r+]
Comment on attachment 738639 [details] [diff] [review]
text area selection tests v.1

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

::: browser/metro/base/tests/mochitest/browser_selection_textarea.html
@@ +3,5 @@
> +  <head>
> +  </head>
> +<body>
> +<form action="texarea.html" method="submit" autocomplete="on">
> +<div style="text-align: center;">

Please add a fixed margin instead of centering. Centering can cause different monitor sizes to react to sendContextMenu* differently.

::: browser/metro/base/tests/mochitest/browser_selection_textarea.js
@@ +55,5 @@
> +
> +    let promise = waitForEvent(document, "popupshown");
> +    sendContextMenuClick(355, 50);
> +    yield promise;
> +    ok(promise && !(promise instanceof Error), "promise error");

nit: This is not needed, the yield will throw and this will never be executed in case of an error. Same below.

@@ +85,5 @@
> +  setUp: setUpAndTearDown,
> +  tearDown: setUpAndTearDown,
> +  run: function test() {
> +    // work around for buggy context menu display
> +    yield waitForMs(100);

Is there a condition we could use?

@@ +127,5 @@
> +        textLength = newTextLength;
> +        return false;
> +      }
> +      return true;
> +    }, 45000, 1000);

Wow, that's a huge timeout! :) Can we reduce it?
Attachment #738639 - Flags: review?(rsilveira) → review+
Comment on attachment 738640 [details] [diff] [review]
sub frame content tests v.1

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

::: browser/metro/base/tests/mochitest/browser_selection_frame_content.js
@@ +121,5 @@
> +
> +    let scrollPromise = waitForEvent(gFrame.contentDocument.defaultView, "scroll");
> +    gFrame.contentDocument.defaultView.scrollBy(0, 200);
> +    yield scrollPromise;
> +    ok(scrollPromise && !(scrollPromise instanceof Error), "scrollPromise error");

Nit: not needed, yield will throw when there is an error. Same bellow.

@@ +148,5 @@
> +    yield popupPromise;
> +    ok(popupPromise && !(popupPromise instanceof Error), "promise error");
> +
> +     // copy text routes through message manager
> +    yield waitForMs(500);

You can use a waitForCondition like in https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_context_menu_tests.js?rev=37fd7284be3a#77
Attachment #738640 - Flags: review?(rsilveira) → review+
Attachment #738682 - Flags: review+
(Assignee)

Updated

5 years ago
No longer depends on: 849015
(Assignee)

Updated

5 years ago
Attachment #738632 - Attachment description: remove old state tracking → part 1 - remove old state tracking
(Assignee)

Updated

5 years ago
Attachment #738682 - Attachment description: head helpers v.2 [sfoster:r+] → part 2 - head helpers v.2
(Assignee)

Comment 42

5 years ago
Created attachment 739135 [details] [diff] [review]
part 3 - selection test helpers
Attachment #738636 - Attachment is obsolete: true
Attachment #738636 - Flags: review?(mbrubeck)
Attachment #739135 - Flags: review?(mbrubeck)
(Assignee)

Comment 43

5 years ago
Created attachment 739137 [details] [diff] [review]
part 4 - basic selection tests v.1
Attachment #738637 - Attachment is obsolete: true
Attachment #738637 - Flags: review?(sfoster)
Attachment #739137 - Flags: review?(sfoster)
Attachment #739135 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 44

5 years ago
Created attachment 739513 [details] [diff] [review]
part 4 - basic selection tests v.1

Removing the double inclusions in the Makefile that snuck in.
Attachment #739137 - Attachment is obsolete: true
Attachment #739137 - Flags: review?(sfoster)
Attachment #739513 - Flags: review?(sfoster)
Comment on attachment 739513 [details] [diff] [review]
part 4 - basic selection tests v.1

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

Looks great. All pass for me (Sony Vaio Duo 11)
Attachment #739513 - Flags: review?(sfoster) → review+
(Assignee)

Comment 46

5 years ago
(In reply to Rodrigo Silveira [:rsilveira] from comment #40)
> Please add a fixed margin instead of centering. Centering can cause
> different monitor sizes to react to sendContextMenu* differently.
> 
> nit: This is not needed, the yield will throw and this will never be
> executed in case of an error. Same below.

updated.

> @@ +85,5 @@
> > +  setUp: setUpAndTearDown,
> > +  tearDown: setUpAndTearDown,
> > +  run: function test() {
> > +    // work around for buggy context menu display
> > +    yield waitForMs(100);
> 
> Is there a condition we could use?

Nothing reliable. What we need to do is fix the context menu code such that showing context menus in close succession works reliably. I'm guessing we'll get to this when we work on tracking downthe context menu test failures.

> 
> @@ +127,5 @@
> > +        textLength = newTextLength;
> > +        return false;
> > +      }
> > +      return true;
> > +    }, 45000, 1000);
> 
> Wow, that's a huge timeout! :) Can we reduce it?

I'd like to give it the extra time, it's a rather large drag. When these ran in debug builds it definitely needed it.
(Assignee)

Comment 47

5 years ago
(In reply to Rodrigo Silveira [:rsilveira] from comment #41)
> Comment on attachment 738640 [details] [diff] [review]
> sub frame content tests v.1
> 
> Review of attachment 738640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/tests/mochitest/browser_selection_frame_content.js
> @@ +121,5 @@
> > +
> > +    let scrollPromise = waitForEvent(gFrame.contentDocument.defaultView, "scroll");
> > +    gFrame.contentDocument.defaultView.scrollBy(0, 200);
> > +    yield scrollPromise;
> > +    ok(scrollPromise && !(scrollPromise instanceof Error), "scrollPromise error");
> 
> Nit: not needed, yield will throw when there is an error. Same bellow.
> 
> @@ +148,5 @@
> > +    yield popupPromise;
> > +    ok(popupPromise && !(popupPromise instanceof Error), "promise error");
> > +
> > +     // copy text routes through message manager
> > +    yield waitForMs(500);
> 
> You can use a waitForCondition like in
> https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/
> mochitest/browser_context_menu_tests.js?rev=37fd7284be3a#77

updated, thanks!
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.