1241582 - Port touch, window_close tests from legacy tool

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

There are various tests in the legacy tool that should be ported over to the new one so we have similar coverage.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

I investigate a bit the current tests we have in the legacy tool and here the result:

a) tests that are ported already:
- browser_responsive_cmd.js
- browser_responsive_devicewidth.js

b) tests that cannot be ported until bug 1240907 is fixed:
- browser_responsivecomputedview.js
- browser_responsiveruleview.js
- browser_responsiveui_customuseragent.js (this should be added by bug 1254386)

c) tests that are unnecessary to port (because tested already in several other tests):
- browser_responsiveui.js

d) tests that cannot be ported because lacks of feature:
- browser_responsiveuiaddcustompreset.js

e) so the tests left are:
- browser_responsiveui_touch.js
- browser_responsiveui_window_close.js

The last one can be – and should be, due the memory leak we had – implemented, the first one I think can be implemented, but I'm not familiar too much with the current touch simulation.

So I'm not sure too much about the scope of this bug – it looks to me more like a meta bug: when it should be considered fixed? When we port all "b", "d" and "e"? In that case this bug is blocked by several other bugs; and maybe it would make sense split it.

If the touch can be ported – I'll investigate more later – we can port the tests in the "e" section, but still no the others.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for this investigation!

I think groups "b" and "e" include the coverage I want us to have in the new RDM.  It doesn't seem like we'll be adding a "custom preset" feature soon, so group "d" seems out of scope.

So for now, I would say do group "e" in this bug, since we want that one and it has no blockers.  I filed bug 1283460 to handle this group.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1241582 - Port touch, window_close tests from legacy tool;

Review commit: https://reviewboard.mozilla.org/r/77848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77848/

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b9fe65b6ff6

The ES oranges are already fixed in the mozreview current push.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8789751 [details]
Bug 1241582 - Port touch, window_close tests from legacy tool;

https://reviewboard.mozilla.org/r/77848/#review76350

Overall, this looks good, thanks for working on it!  My main concern is leaking the `unload` listener.  Should be ready for r+ once that's resolved.  Hopefully :ntim, :gl, or :ochameau can pick up the next round while I am away.

::: devtools/client/responsive.html/manager.js:118
(Diff revision 1)
>        }
>        this.activeTabs.delete(tab);
>  
>        if (!this.isActiveForWindow(window)) {
>          this.removeMenuCheckListenerFor(window);
> +        window.removeEventListener("unload", ui);

Why do you remove the listener here?  Let's think through the case of 1 browser window, 2 tabs, RDM open in each tab.  By adding the unload listener in ResponsiveUI.init, there will be two listeners added, one for ResponsiveUI from each tab.  Here though, the listener is only removed when closing the _last_ RDM for a browser window, so there other tab's RDM listener is leaked.

It seems like it would be better to remove the unload listener in ResponsiveUI.destroy to resolve this.

::: devtools/client/responsive.html/manager.js:355
(Diff revision 1)
>      // If our tab is about to be closed, there's not enough time to exit
>      // gracefully, but that shouldn't be a problem since the tab will go away.
>      // So, skip any yielding when we're about to close the tab.
> -    let isTabClosing = options && options.reason == "TabClose";
> +    let isWindowClosing = options && options.reason === "unload";
> +    let isTabClosing = options && options.reason === "TabClose";
> +    let isClosing = isWindowClosing || isTabClosing;

`isClosing` is a little too general to me, since we're always closing something in this method (RDM at the very least!), but `isClosing` is only true for the window / tab shutdown cases.

Let's do this:

1. Remove `isClosing`
2. Revert all the ifs that checked `isTabClosing` back to that
3. Define `isTabClosing` as:

```
isTabClosing = (options && options.reason === "TabClose") || isWindowClosing
```

I think this seems pretty logical for our use here, since the tab is definitely closing if the window is closing, and it seems to skip the same checks that you wanted to skip in the window case.

::: devtools/client/responsive.html/test/browser/browser.ini:17
(Diff revision 1)
>    !/devtools/client/framework/test/shared-head.js
>    !/devtools/client/framework/test/shared-redux-head.js
>    !/devtools/client/inspector/test/shared-head.js
>    !/devtools/client/shared/test/test-actor.js
>    !/devtools/client/shared/test/test-actor-registry.js
> -
> +  touch.html

Nit: let's sort that above the absolute entries (so just after head.js)

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:41
(Diff revision 1)
> +    "Touch simulation is stopped on click.");
> +}
> +
> +function* testWithNoTouch(ui) {
> +  yield injectEventUtils(ui);
> +  yield ContentTask.spawn(ui.getViewportBrowser(), {}, function* () {

Not required, but there is also a `spawnViewportTask` helper for this, just saves a few chars, that's all.

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:49
(Diff revision 1)
> +    let div = content.document.querySelector("div");
> +    let x = 0, y = 0;
> +
> +    info("testWithNoTouch: Initial test parameter and mouse mouse outside div");
> +    x = -1; y = -1;
> +    yield EventUtils.synthesizeMouse(div, x, y,

Nit: it seems like you alternate between indenting the next line 2 spaces vs. aligning under EventUtils.   Okay with either, just choose one way throughout.

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:57
(Diff revision 1)
> +    div.style.backgroundColor = "";
> +
> +    info("testWithNoTouch: Move mouse into the div element");
> +    yield EventUtils.synthesizeMouseAtCenter(div,
> +      { type: "mousemove", isSynthesized: false }, content);
> +    is(div.style.backgroundColor, "red", "mouseenter or mouseover should work");

Cool, I don't think I knew you could run test asserts directly inside the ContentTask!  (I think I've always returned a value from the task and then checked that.)  It seems to work and it makes sense to do it this way for this test.

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:125
(Diff revision 1)
> +  });
> +}
> +
> +function* testWithMetaViewportEnabled(ui) {
> +  let domViewportEnabled = Services.prefs.getBoolPref(DOM_VIEWPORT_ENABLED);
> +  Services.prefs.setBoolPref(DOM_VIEWPORT_ENABLED, true);

`pushPrefEnv`[1] like the old test is a better approach since it auto-adds a cleanup function to revert the pref in case the test fails midway due to some exception.

Someone recently made `pushPrefEvn` return its own promise, so you don't need to manually wrap it to get a promise like the old test did.

[1]: https://dxr.mozilla.org/mozilla-central/rev/176aff980979bf588baed78c2824571a6a7f2b96/testing/specialpowers/content/specialpowersAPI.js#1022

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:214
(Diff revision 1)
> +    EventUtils.window = {};
> +    EventUtils.parent = EventUtils.window;
> +    EventUtils._EU_Ci = Components.interfaces; // eslint-disable-line
> +    EventUtils._EU_Cc = Components.classes; // eslint-disable-line
> +    // EventUtils' `sendChar` function relies on the navigator to synthetize
> +    // events.

Nit: seems like this fits on one line since we use 90 chars now

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:218
(Diff revision 1)
> +    // EventUtils' `sendChar` function relies on the navigator to synthetize
> +    // events.
> +    EventUtils.navigator = content.navigator;
> +    EventUtils.KeyboardEvent = content.KeyboardEvent;
> +
> +    EventUtils.synthesizeClick = (element) => new Promise(resolve => {

Nit: drop parens for single arg => function

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:235
(Diff revision 1)
> +    Services.scriptloader.loadSubScript(
> +      "chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);
> +  });
> +}
> +
> +const enableTouchSimulation = (ui) => new Promise(

Nit: drop parens for single arg => function

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:239
(Diff revision 1)
> +
> +const enableTouchSimulation = (ui) => new Promise(
> +  Task.async(function* (resolve) {
> +    let browser = ui.getViewportBrowser();
> +
> +    browser.addEventListener("mozbrowserloadend", function onLoad() {

Seems like we could move this wait for reload part inside `updateTouchSimulation` so others can get that waiting as well.  (I don't think anyone else yields on `updateTouchSimulation` so far.)

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8789751 [details]
Bug 1241582 - Port touch, window_close tests from legacy tool;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77848/diff/1-2/

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Tim, let me know if you're able to review that – otherwise I'll check if Gabriel is available.
I should have addressed all the comments but this one:

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Sept. 10 - 25) from comment #5)
> Comment on attachment 8789751 [details]
> Bug 1241582 - Port touch, window_close tests from legacy tool;

> Seems like we could move this wait for reload part inside
> `updateTouchSimulation` so others can get that waiting as well.  (I don't
> think anyone else yields on `updateTouchSimulation` so far.)

(Apparently that the second time mozreview cancels my comments, I was pretty sure I added there but there is no trace of them)

The reason I didn't move the wait for the reload part it's because the nature of the event: it waits that the load of the entire page is finished, that's good in unit testing but I wouldn't put that in the app code. If we would do so, we should probably change approach and definitely event. Since also Ryan told that anyone else yields on `updateTouchSimulation` so far, I didn't think it's necessary work on that now, if it would needed in future code will work on that there, once is needed.

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8789751 [details]
Bug 1241582 - Port touch, window_close tests from legacy tool;

https://reviewboard.mozilla.org/r/77848/#review77216

r=me with comments addressed.

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:21
(Diff revision 2)
> +  yield testWithMetaViewportEnabled(ui);
> +  yield testWithMetaViewportDisabled(ui);
> +  testTouchButton(ui);
> +});
>  
> -  // Wait until the viewport has been added
> +function testTouchButton(ui) {

nit: I think it would help with readability if you order the function definitions the order the functions are called

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:195
(Diff revision 2)
> +
> +  yield waitUntilState(store, state => state.viewports.length == 1);
> +  yield waitForFrameLoad(ui, TEST_URL);
> +}
> +
> +function* injectEventUtils(ui) {

I don't feel too comfortable injecting EventUtils like this, but if that's the only way we can make this test work without CPOWs, then it's fine. Maybe elment.dispatchEvent(new Event(...)) could be a better alternative?

::: devtools/client/responsive.html/test/browser/browser_touch_simulation.js:205
(Diff revision 2)
> +
> +    let EventUtils = content.EventUtils = {};
> +
> +    EventUtils.window = {};
> +    EventUtils.parent = EventUtils.window;
> +    EventUtils._EU_Ci = Components.interfaces; // eslint-disable-line

I'm guessing eslint isn't happy because of Components, if that's the case I'd prefer using
/* eslint-disable rule-name */
...
/* eslint-enable rule-name */

::: devtools/client/responsive.html/test/browser/browser_window_close.js:15
(Diff revision 2)
> +
> +  newWindow.focus();
> +  yield once(newWindow.gBrowser, "load", true);
> +
> +  let tab = newWindow.gBrowser.selectedTab;
> +  yield ResponsiveUIManager.openIfNeeded(newWindow, tab);

nit: use let { manager } = yield openRDM(tab); which is more consistent with other tests.

::: devtools/client/responsive.html/test/browser/touch.html:10
(Diff revision 2)
> +<title>test</title>
> +
> +
> +<style>
> +  div {
> +    border:1px solid red;

nit: space after border:

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8789751 [details]
Bug 1241582 - Port touch, window_close tests from legacy tool;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77848/diff/2-3/

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8789751 [details]
Bug 1241582 - Port touch, window_close tests from legacy tool;

https://reviewboard.mozilla.org/r/77848/#review76350

> Not required, but there is also a `spawnViewportTask` helper for this, just saves a few chars, that's all.

The original code uses that. However, eslint causes a lot of error because the internal usage of content. I wanted to avoid to add extra characters for the comment in order to makes happy ESLint. Using directly `ContentTask.spawn ui.getViewportBrowser()` it was a better approach at the end.

> Nit: drop parens for single arg => function

I'm doing that now, but I would like to know if there is a coding style guide about that. For example, in Add-on SDK team we started to don't have the parens for single arg, but then we discovered that often it wasn't always readable at a first glance.

> Seems like we could move this wait for reload part inside `updateTouchSimulation` so others can get that waiting as well.  (I don't think anyone else yields on `updateTouchSimulation` so far.)

I drop that for the time being, in case Tim as well thinks it should be done, I'll move that on `ui.updateTouchSimulation`. The reason because I didn't do that, it's due the nature of the event: "mozbrowserloadend" is emitted when the load is finished, so it could takes a lot depends by the page, and it's not necessary what we want to do in the app code. However, in a controlled environment as the tests, that is the safest solution. In case we decided we want to change the `updateTouchSimulation` too we'll probably needs to come with a different approach / event.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/382fe531c38c
Port touch, window_close tests from legacy tool; r=ntim

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Okay, now it published the previous comment that I thought was lost and not the one for Tim. Peeerfect. :)

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8789751 [details]
Bug 1241582 - Port touch, window_close tests from legacy tool;

https://reviewboard.mozilla.org/r/77848/#review77216

> I don't feel too comfortable injecting EventUtils like this, but if that's the only way we can make this test work without CPOWs, then it's fine. Maybe elment.dispatchEvent(new Event(...)) could be a better alternative?

I would keep this approach, for the moment at least, since this is the approach we have in several part of our codebase to deal with content and `EventUtils`. In case, we could have a generic bug to change that – not only here.

> I'm guessing eslint isn't happy because of Components, if that's the case I'd prefer using
> /* eslint-disable rule-name */
> ...
> /* eslint-enable rule-name */

It wasn’t the Components, but the camelcase rule. Anyway, I used the way you suggested; thanks!

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/382fe531c38c