1285566 - Use per-tab API for touch events

Status: VERIFIED FIXED

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

Description

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

Bug 970346 is adding a per-tab platform API for touch events.

To make use of this in RDM, we'll need to do something like:

* Add some kind of DevTools actor support to flip the new platform attribute.  Historically we've managed these things from webbrowser.js, and it's probably best to keep going with this pattern.  User agent is a good example[1].
* The RDM UI needs to use a DevTools client to change the actor setting.  So far, we have not done this from the new RDM UI.  The old RDM UI does it for user agent[2].  This might be something that happens in manager.js for the new UI, I haven't completely thought through that part.

[1]: https://dxr.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898/devtools/server/actors/webbrowser.js#1755
[2]: https://dxr.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898/devtools/client/responsivedesign/responsivedesign.jsm#975

Comment 1

[Benoit Chabod [:bigben]]

I'm willing to take this bug, if no one else has started working on it!
From my understanding, we have 2 ways of doing this:
- Use the reconfigure method on TabActor like in the old RDM, and add our own private method in webbrowser.js, something like _setOverrideTouch that would flip the attribute on the docshell.
- Use a custom actor inheriting from TabActor, something like "EmulationActor" that will have access to the docshell (https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/actor-hierarchy.md#107) and will do the flipping.

I've already written an EmulationActor for bug 1069882, but I guess the first method has the advantage to take care of auto-reset properly, like Alex explained here:
https://bugzilla.mozilla.org/show_bug.cgi?id=828008#c33

Ryan, what do you think?

Comment 2

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

(In reply to Benoit Chabod [:bigben] from comment #1)
> I'm willing to take this bug, if no one else has started working on it!
> From my understanding, we have 2 ways of doing this:
> - Use the reconfigure method on TabActor like in the old RDM, and add our
> own private method in webbrowser.js, something like _setOverrideTouch that
> would flip the attribute on the docshell.
> - Use a custom actor inheriting from TabActor, something like
> "EmulationActor" that will have access to the docshell
> (https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/actor-
> hierarchy.md#107) and will do the flipping.
> 
> I've already written an EmulationActor for bug 1069882, but I guess the
> first method has the advantage to take care of auto-reset properly, like
> Alex explained here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=828008#c33
> 
> Ryan, what do you think?

Yes, let's use the new emulation actor you are adding for this.  I added several notes about the reset philosophy in bug 1069882 comment 26.

Comment 3

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

By the way, you may want to prioritize this bug first and land the emulation actor over here, since it seems more "ready to go" than geolocation since there aren't UX issues to resolve.  I suspect many of us will need the emulation actor soon.  :zer0 probably needs it for dPR, I will likely want to move UA to the actor, etc.

Comment 4

[Benoit Chabod [:bigben]]

Attachment: Create an EmulationActor and use per-tab API for touch simulator

I agree with you, this is a great bug to setup the emulation actor!
Here's a first draft, I tried to follow the advices you gave in bug 1069882.

I've changed the reload mechanism accordingly, and in this patch the RDM reloads its content depending on the current override status. Maybe we could even do reloadNeeded = (!dom.w3c_touch_events.enabled && !currentOverride=enabled), to be more precise. What do you think?

Comment 5

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

Comment on attachment 8779782 [details] [diff] [review]
Create an EmulationActor and use per-tab API for touch simulator

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

I think you're heading in the right direction, but there's a bit more complexity to tackle here.

::: devtools/client/responsive.html/manager.js
@@ +344,5 @@
>      this.swap = null;
>  
> +    // Close the debugger client used to speak with emulation actor
> +    yield new Promise((resolve, reject) => {
> +      this.client.close(resolve);

Some day this will return a promise itself (bug 1243452)...

@@ +363,5 @@
> +      DebuggerServer.addBrowserActors();
> +    }
> +    this.client = new DebuggerClient(DebuggerServer.connectPipe());
> +    yield this.client.connect();
> +    let {tab} = yield this.client.getTab();

Nit: We've been using { tab } style in this file

@@ +409,5 @@
>    },
>  
>    updateTouchSimulation: Task.async(function* (enabled) {
>      if (enabled) {
> +      let currentOverride = yield this.client.request({

I guess we shouldn't assume that touch is disabled when starting, someone else could have enabled it.  So we need to dispatch the current start to the RDM UI on start.

@@ +413,5 @@
> +      let currentOverride = yield this.client.request({
> +        to: this.emulationActor,
> +        type: "getTouchEventsOverride",
> +      });
> +      let reloadNeeded = (currentOverride != Ci.nsIDocShell.TOUCHEVENTS_OVERRIDE_ENABLED);

If it's already enabled, you should be able to just return and do nothing else, right?

@@ +421,5 @@
> +        type: "setTouchEventsOverride",
> +        flag: Ci.nsIDocShell.TOUCHEVENTS_OVERRIDE_ENABLED,
> +      });
> +
> +      yield this.touchEventSimulator.start();

Since we now have an actor, it doesn't really make sense to also have this additional module that starts a frame script, since the actor is also running on the content process as well.  Ideally the actor would directly run the simulation if you set the override.

Another constraint to be aware of is there are other consumers that also use the touch simulator[1].  So, we should preserve the existing API for them (or else we need to repair them).

One approach would be:

1. Extract the core touch simulation out of simulator-content.js to simulator-core.js.  core.js would be a CommonsJS module that exports a simulator object that can be passed a window to operate on.
2. content.js would retain the messageManager send / receive bits to communicate with other consumers.  If someone tells it to start simulation, it will load core.js and start the simulator.
3. The actor would load core.js and start the simulator when the override is set.

This avoids the work of modifying the other consumers, both of which will eventually be deleted anyway.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=touch%2Fsimulator&redirect=true

::: devtools/server/actors/emulation.js
@@ +14,5 @@
> +  },
> +
> +  _previousTouchEventsOverride: null,
> +
> +  setTouchEventsOverride(flag) {

When setting, first check if the value to set is the same as the current value.  If it already matches, return and don't record a previous value.  See `_toggleDevToolsSettings` in webbrowser.js.

@@ +35,5 @@
> +  disconnect() {
> +    this.destroy();
> +  },
> +
> +  destroy() {

Null out the reference to the docshell on destroy.

Comment 6

[Tim Nguyen :ntim]

Benoit, instead of using protocol request calls. I recommend creating a front class for the actor, so you can get a prettier and more simple API to work with.

You can grab a look at this patch (the UserAgentFront part): https://bug828008.bmoattachments.org/attachment.cgi?id=8687691

Although now things have changed, and the front and the actor now need to be in different files (the front should be in devtools/shared/fronts)

Comment 7

[Benoit Chabod [:bigben]]

Attachment: Create an EmulationActor and use per-tab API for touch simulator, v2

Alright, here's a new version of the EmulationActor and the corresponding refactor of the touch simulator!
I've addressed all your comments Ryan, except this one:

> I guess we shouldn't assume that touch is disabled when starting, someone
> else could have enabled it.  So we need to dispatch the current start to the
> RDM UI on start.

As discussed on IRC, we will handle this and other potential multiple EmulationActor issues in follow-up bugs.
I guess the main changes with the previous patch come from the simulator rewriting, I've verified that both the actor codepath and the frame script codepath (still used by the old RDM and b2g simulator) are working.

Tim, I followed your advice and created a Front object. Thanks for the suggestion! :)

Comment 8

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

Comment on attachment 8780513 [details] [diff] [review]
Create an EmulationActor and use per-tab API for touch simulator, v2

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

Great, this is coming together well!  I'd like to see one more round just to make sure the SimulatorCore piece is working as expected with multiple tabs.

::: devtools/client/responsive.html/manager.js
@@ +356,5 @@
> +    }
> +    this.client = new DebuggerClient(DebuggerServer.connectPipe());
> +    yield this.client.connect();
> +    let { tab } = yield this.client.getTab();
> +    this.emulationFront = EmulationFront(this.client, { actor: tab.emulationActor });

It's more typical to pass the tab's entire form to the front, so in this case you can do just `EmulationFront(this.client, tab);`.  Then, in the front's initialize method, pull off the correct actor property, like in this example[1].

[1]: https://dxr.mozilla.org/mozilla-central/rev/233ab21b64b5d5e9f2f16ea2d4cfb4c8b293c5c4/devtools/shared/fronts/inspector.js#946

::: devtools/server/actors/emulation.js
@@ +8,5 @@
> +const protocol = require("devtools/shared/protocol");
> +const { emulationSpec } = require("devtools/shared/specs/emulation");
> +const { SimulatorCore } = require("devtools/shared/touch/simulator-core");
> +
> +let EmulationActor = protocol.ActorClassWithSpec(emulationSpec, {

You'll need to rebase this, bug 1288423 changed this to just `ActorClass`.

::: devtools/shared/fronts/emulation.js
@@ +8,5 @@
> +
> +/**
> + * The corresponding Front object for the EmulationActor.
> + */
> +const EmulationFront = FrontClassWithSpec(emulationSpec, {

You'll need to rebase this, bug 1288423 changed this to just `FrontClass`.

::: devtools/shared/touch/simulator-core.js
@@ +54,5 @@
> +    if (this.simulatorTarget) {
> +      // Simulator is already started
> +      return;
> +    }
> +    this.simulatorTarget = simulatorTarget;

At the moment, there can only be one `SimulatorCore`, so if you try to enable simulation in two different tabs at the same time, the second one doesn't actually start the simulator since there is already a simulator target set.  Unlike the simulator-content.js frame script which was loaded once for each window, this module is only loaded a single time per process.

It seems like the easiest fix is to make SimulatorCore be a function and put these properties and methods on its prototype.  Then the frame script and actor would create an instance with `new SimulatorCore(target)` or something similar.

Comment 9

[Benoit Chabod [:bigben]]

Attachment: Create an EmulationActor and use per-tab API for touch simulator, v3

Comments addressed!
Indeed, this works much better if you try to use the simulator in two RDM tabs at the same time.

Comment 10

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

Comment on attachment 8780668 [details] [diff] [review]
Create an EmulationActor and use per-tab API for touch simulator, v3

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

Great, this version looks good to me.  Thanks for working on it!

Comment 11

[Benoit Chabod [:bigben]]

Thanks for the review!
Here's the associated try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=265089656259

Devtools failures look related to intermittent bugs, so I guess this is ready to ship :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

What's with the browser_shutdown_close_sync.js RDM failures across all platforms?

Comment 13

[Tim Nguyen :ntim]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> What's with the browser_shutdown_close_sync.js RDM failures across all
> platforms?

We're now waiting for the client to close before setting this.destroyed = true;. Therefor shutdown is no longer synchronous, but async.

Comment 14

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

Comment on attachment 8780668 [details] [diff] [review]
Create an EmulationActor and use per-tab API for touch simulator, v3

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

::: devtools/client/responsive.html/manager.js
@@ +336,4 @@
>      this.swap = null;
>  
> +    // Close the debugger client used to speak with emulation actor
> +    yield new Promise((resolve, reject) => {

We need to preserve the special case sync behavior that browser_shutdown_close_sync.js is checking for so that Firefox shutdown behaves as expected.

You should guard this async behavior with `if (!isTabClosing) {` like the other yield statements in the destroy path.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c289e719ca5d
Create an EmulationActor and use per-tab API for touch simulator, r=jryans

Comment 16

[Tim Nguyen :ntim]

(In reply to Pulsebot from comment #15)
> https://hg.mozilla.org/integration/fx-team/rev/c289e719ca5d
> Create an EmulationActor and use per-tab API for touch simulator, r=jryans

This contains the fix jryans suggested, which I've verified locally.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in devtools test browser_toolbox_computed_view.js:

https://hg.mozilla.org/integration/fx-team/rev/b773199257dc43a4ed4ded8f41dc01f12e2ba222

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=c289e719ca5dbb182f198c0322793769d9fa0092
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11147296&repo=fx-team

Comment 18

[Tim Nguyen :ntim]

So I took a look at this, so both failing tests (ruleview/computedview) have 2 clients opened, my guess is that's the cause of the failure.

I tried to figure out why we didn't get a docshell leak like bug 1252201 or 1250058. So I took a look at head.js for both old and new RDM and I noticed that the toolbox is being started up differently:
Both old and new RDM use gDevTools.showToolbox(target, "tool"); but new RDM calls target.makeRemote(); before showing the toolbox.

- New RDM: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#311
Calls inspector/test/shared-head.js openInspector function which calls the above ^
- Old RDM: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/test/head.js#103
Calls the responsivedesign/test/head.js openInspector function

Not sure if that makes a big difference, but it's worth checking if using the same startup functions makes the docshell leak from bug 1252201 appear. If that's the case, we should probably take a look at fixing up the startup functions to support 2 clients at once.

Comment 19

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

I think the is that, for the moment, the debugger server isn't following the frame swaps in RDM, so it doesn't notice the tab removal in browser_shutdown_close_sync.js.  This should be resolved by bug 1240912 that is in review.  

My initial suggested fix prevents the client from closing during that test, which wedges the server in a confused state, since it also doesn't realize the tab closed.  This confused state breaks the toolbox tests when they try to register a test actor.

To get things working here, I changed destroy to always close the client, but only yield when async is okay.  The sync shutdown test adds additional checks to make sure the client is closed before moving on to the next test (to avoid intermittent failures).

Let's see what try says:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe47a85d0f8

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/831107a4950a
Create an EmulationActor and use per-tab API for touch simulator, r=jryans

Comment 21

[Benoit Chabod [:bigben]]

Ryan and Tim, thanks for fixing the issue for me.
I was a bit busy and I didn't find enough time to investigate this properly.
I hope this patch won't cause any other failures and that we will be able to use the EmulationActor in other bugs as soon as possible!

Comment 22

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

(In reply to Benoit Chabod [:bigben] from comment #21)
> Ryan and Tim, thanks for fixing the issue for me.
> I was a bit busy and I didn't find enough time to investigate this properly.
> I hope this patch won't cause any other failures and that we will be able to
> use the EmulationActor in other bugs as soon as possible!

No worries, the test failures ended up being a bit trickier than I had imagined...  Thanks again for working on this!

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/831107a4950a

Comment 24

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

For testing this, here's the general idea:

1. Start RDM in tab A
2. Enable touch simulator (hand icon)
3. Go to some touch events page such as https://mozilla.github.io/mozhacks/touch-events/event-listener.html
  * If you went to your test page before enable touch simulator, reload the page before continuing
4. Touch events should be working
5. Open a new tab B without RDM
6. In tab B, touch events should remain disabled even though you can still use them in tab A with RDM

Comment 25

[Mihai Boldan, Desktop QA [:mboldan]]

I manage to test this issue on Firefox 51.0a1 (2016-09-01) across platforms[1] and using the STR from Comment 24. 
Since the issue is no longer reproducible, I am marking this bug Verified Fixed.

[1] Windows 10x64, Mac OS X 10.11.1 and Ubuntu 16.04x64