J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Description • 8 years ago

Bug 1241714 adds a selected device.  We need to apply the user agent when it's selected.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 1 • 8 years ago

To do this, we'll need to create a DevTools RDP client in the RDM tool and use it apply the UA setting.

We already did something similar for UA in the old RDM tool[1].  Once you have the RDP client, you can use it to set the new UA[2].  So, for the new RDM, we would pass the desired UA to this RDP client when a device is chosen.

In the new RDM, I believe the RDP client would be part of the manager.js module outside the page.  So we'd want to communicate with that when the device changes.

That's a high level overview, feel free to needinfo? about details as needed!

[1]: https://dxr.mozilla.org/mozilla-central/rev/5511d54a3f172c1d68f98cc55dce4de1d0ba1b51/devtools/client/responsivedesign/responsivedesign.jsm#266-279
[2]: https://dxr.mozilla.org/mozilla-central/rev/5511d54a3f172c1d68f98cc55dce4de1d0ba1b51/devtools/client/responsivedesign/responsivedesign.jsm#975

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 2 • 8 years ago

In my notes from bug 1269882, we agreed that:

"For user agent changes, we would not reload automatically, as developers should understand that a reload is needed to fully apply a UA change."

So for this bug, we don't want to reload after applying the UA.

	Tim Nguyen :ntim Assignee
	Comment 3 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)

> To do this, we'll need to create a DevTools RDP client in the RDM tool and
> use it apply the UA setting.
> 
> In the new RDM, I believe the RDP client would be part of the manager.js
> module outside the page.  So we'd want to communicate with that when the
> device changes.

This approach won't work until bug 1240907 is fixed. The UA will end up changing only for the top-level RDM window. The content iframe won't be affected since it has the `mozbrowser` and `isolate` attributes, which make the iframe fully independent from the RDM window.

You can open this testcase:
data:text/html,<button onclick="this.nextSibling.textContent = navigator.userAgent">write down UA</button><div></div>

then paste this code inside the console:
var Ci = Components.interfaces;
var ds = window.QueryInterface(Ci.nsIInterfaceRequestor)
                               .getInterface(Ci.nsIWebNavigation)
                               .QueryInterface(Ci.nsIDocShell);
ds.customUserAgent = "foo";

You'll see that navigator.userAgent changes for the top-level RDM window, but if you click the webpage's button, the UA is not being changed (normally it should change live without any reload).

	Tim Nguyen :ntim Assignee
	Comment 4 • 8 years ago

(In reply to Tim Nguyen :ntim (not accepting requests) from comment #3)
> The UA will end up changing only for the top-level RDM window. The content iframe won't be
> affected since it has the `mozbrowser` and `isolate` attributes, which make
> the iframe fully independent from the RDM window.
To clarify, the mozbrowser iframe docshell isn't a child docshell (because mozbrowser iframes are special iframes) for the RDM window's docshell, which will cause the custom UA not to be passed on.
This is the code that passes on the flag to the child docshells: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3153

Either way, we don't want the UA to be changed for the RDM window, so we'll need bug 1240907 to be fixed first anyway.

> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> 
> > To do this, we'll need to create a DevTools RDP client in the RDM tool and
> > use it apply the UA setting.
> > 
> > In the new RDM, I believe the RDP client would be part of the manager.js
> > module outside the page.  So we'd want to communicate with that when the
> > device changes.
> 
> This approach won't work until bug 1240907 is fixed.
It would work if we would target the DebuggerServer to the mozbrowser iframe, but that's basically bug 1240907.

	Tim Nguyen :ntim Assignee
	Comment 5 • 8 years ago

Of course, it's still possible to go forward with the suggested approach, and fix bug 1240907 later (I suppose it won't require much client side changes, but more server side changes), but I find it a bit odd to proceed backwards.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 6 • 8 years ago

(In reply to Tim Nguyen :ntim from comment #3)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> 
> > To do this, we'll need to create a DevTools RDP client in the RDM tool and
> > use it apply the UA setting.
> > 
> > In the new RDM, I believe the RDP client would be part of the manager.js
> > module outside the page.  So we'd want to communicate with that when the
> > device changes.
> 
> This approach won't work until bug 1240907 is fixed. The UA will end up
> changing only for the top-level RDM window. The content iframe won't be
> affected since it has the `mozbrowser` and `isolate` attributes, which make
> the iframe fully independent from the RDM window.
> 
> You can open this testcase:
> data:text/html,<button onclick="this.nextSibling.textContent =
> navigator.userAgent">write down UA</button><div></div>
> 
> then paste this code inside the console:
> var Ci = Components.interfaces;
> var ds = window.QueryInterface(Ci.nsIInterfaceRequestor)
>                                .getInterface(Ci.nsIWebNavigation)
>                                .QueryInterface(Ci.nsIDocShell);
> ds.customUserAgent = "foo";
> 
> You'll see that navigator.userAgent changes for the top-level RDM window,
> but if you click the webpage's button, the UA is not being changed (normally
> it should change live without any reload).

Thanks for the detailed comments :ntim!  Jaideep and I also noticed this issue about a day before your comments were posted when he shared a patch over IRC.  I told him the same thing, that it would likely make sense to wait for bug 1240907 first so it's easier to complete this one and be confident that it is correct.

	Jaideep Bhoosreddy [:jbhoosreddy]
	Comment 7 • 8 years ago

This is a WIP patch of the work I did before when this bug was still blocked.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 8 • 8 years ago

Let's wait for the emulation actor Benoit is adding in bug 1285566.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 14 • 8 years ago

Comment on attachment 8782038 [details] [diff] [review]
Move UA emulation to emulation actor

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

Looks good, just some naming things to resolve.  Thanks for working on it!

::: devtools/server/actors/emulation.js
@@ +50,5 @@
>    },
>  
> +  /* User agent override */
> +
> +  _previousCustomUserAgent: null,

Let's follow the naming pattern from touch events in the actor, so `_previousUserAgentOverride`, etc.

@@ +52,5 @@
> +  /* User agent override */
> +
> +  _previousCustomUserAgent: null,
> +
> +  setCustomUserAgent(userAgent) {

`setUserAgentOverride`

@@ +63,5 @@
> +    this.docShell.customUserAgent = userAgent;
> +    return true;
> +  },
> +
> +  getCustomUserAgent() {

`getUserAgentOverride`

@@ +67,5 @@
> +  getCustomUserAgent() {
> +    return this.docShell.customUserAgent;
> +  },
> +
> +  restoreUserAgent() {

`clearUserAgentOverride`

@@ +69,5 @@
> +  },
> +
> +  restoreUserAgent() {
> +    if (this._previousCustomUserAgent !== null) {
> +      return this.setCustomUserAgent(this._previousCustomUserAgent);

Update `clearTouchEventsOverride` to also return like this as well.

::: devtools/shared/specs/emulation.js
@@ +48,5 @@
> +
> +    restoreUserAgent: {
> +      request: {},
> +      response: {
> +        valueChanged: RetVal("boolean")

`valueChanged` seems like a better name here, please change the touch events `reload` to `valueChanged` in this file.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 15 • 8 years ago

Comment on attachment 8782040 [details] [diff] [review]
Test for device list

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

::: devtools/client/responsive.html/test/browser/browser.ini
@@ +14,5 @@
>    !/devtools/client/inspector/test/shared-head.js
>    !/devtools/client/shared/test/test-actor.js
>    !/devtools/client/shared/test/test-actor-registry.js
>  
> +[browser_device_change.js]

You forgot to add the test to the patch. ;)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 16 • 8 years ago

Comment on attachment 8782039 [details] [diff] [review]
Add user agent emulation to new RDM

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

Seems close, but there's a small regression left to fix.

::: devtools/client/responsive.html/components/device-selector.js
@@ +35,5 @@
>      if (target.value === OPEN_DEVICE_MODAL_VALUE) {
>        onUpdateDeviceModalOpen(true);
>        return;
>      }
> +    let selected;

It seems like something here is causing a regression:

STR:

1. Choose a device
2. Use the corner resize gripper to move to some random size

ER:

Should go back to "no device selected"

AR:

Device name still in place, though the device UA is removed.

::: devtools/client/responsive.html/manager.js
@@ +404,4 @@
>      }
>    },
>  
> +  waitForReload() {

Remove this, looks unused.

@@ +429,5 @@
>        this.emulationFront.clearTouchEventsOverride();
>      }
>    }),
>  
> +  updateUserAgent: Task.async(function* (userAgent) {

Seems like you could remove Task.async here since yielding isn't needed for control flow currently.

@@ +432,5 @@
>  
> +  updateUserAgent: Task.async(function* (userAgent) {
> +    if (userAgent) {
> +      yield this.emulationFront.setCustomUserAgent(
> +        userAgent

Nit: Seems like this would fit on one line.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 17 • 8 years ago

Comment on attachment 8782124 [details] [diff] [review]
Make old RDM UA emulation use emulation actor

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

Thanks for updating it, seems to work well!

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +274,5 @@
>      }
>      this.client = new DebuggerClient(DebuggerServer.connectPipe());
>      yield this.client.connect();
>      let {tab} = yield this.client.getTab();
> +    yield this.client.attachTab(tab.actor);

Is `attachTab` actually needed still?

@@ +971,5 @@
>     * Change the user agent string
>     */
>    changeUA: Task.async(function* () {
>      let value = this.userAgentInput.value;
> +    let valueChanged;

Maybe just `changed` for brevity?

@@ +976,3 @@
>      if (value) {
> +      valueChanged = yield this.emulationFront.setCustomUserAgent(
> +        value

Probably fits on one line?  Unsure...  90 chars is our current length.

	Tim Nguyen :ntim Assignee
	Comment 18 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> Comment on attachment 8782039 [details] [diff] [review]
> Add user agent emulation to new RDM
> 
> Review of attachment 8782039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems close, but there's a small regression left to fix.
> 
> ::: devtools/client/responsive.html/components/device-selector.js
> @@ +35,5 @@
> >      if (target.value === OPEN_DEVICE_MODAL_VALUE) {
> >        onUpdateDeviceModalOpen(true);
> >        return;
> >      }
> > +    let selected;
> 
> It seems like something here is causing a regression:
> 
> STR:
> 
> 1. Choose a device
> 2. Use the corner resize gripper to move to some random size
> 
> ER:
> 
> Should go back to "no device selected"
> 
> AR:
> 
> Device name still in place, though the device UA is removed.
> 
Looks like a leftover from Jaideep's patch, I'll take a look if I can remove this.

	Tim Nguyen :ntim Assignee
	Comment 19 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> Comment on attachment 8782124 [details] [diff] [review]
> Make old RDM UA emulation use emulation actor
> 
> Review of attachment 8782124 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for updating it, seems to work well!
> 
> ::: devtools/client/responsivedesign/responsivedesign.jsm
> @@ +274,5 @@
> >      }
> >      this.client = new DebuggerClient(DebuggerServer.connectPipe());
> >      yield this.client.connect();
> >      let {tab} = yield this.client.getTab();
> > +    yield this.client.attachTab(tab.actor);
> 
> Is `attachTab` actually needed still?
Yes, otherwise waitForReload() doesn't work (as it relies on TabActor events).

	Tim Nguyen :ntim Assignee
	Comment 20 • 8 years ago

MozReview-Commit-ID: KhxfaGnuuUd

	Tim Nguyen :ntim Assignee
	Comment 21 • 8 years ago

MozReview-Commit-ID: 9005ktskEVk

	Tim Nguyen :ntim Assignee
	Comment 22 • 8 years ago

MozReview-Commit-ID: EQawVubnMAp

	Tim Nguyen :ntim Assignee
	Comment 23 • 8 years ago

MozReview-Commit-ID: K56yifj9xfI

	Tim Nguyen :ntim Assignee
	Comment 24 • 8 years ago

MozReview-Commit-ID: KhxfaGnuuUd

	Tim Nguyen :ntim Assignee
	Comment 25 • 8 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 26 • 8 years ago

Comment on attachment 8783263 [details] [diff] [review]
Move custom UA emulation to emulation actor

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

Great, looks good to me!  Thanks for adjusting the names to fit my desire for parallel naming. :)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 27 • 8 years ago

Comment on attachment 8783264 [details] [diff] [review]
Change old RDM to use emulation actor

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

Great, looks good to me, thanks for working on it!

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +984,1 @@
>      let reloaded = this.waitForReload();

Move this inside the `if (changed)` block

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 28 • 8 years ago

Comment on attachment 8783266 [details] [diff] [review]
Test for device list

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

Would be great to add the enhancement I mentioned, but looks good to me!

::: devtools/client/responsive.html/test/browser/browser_device_change.js
@@ +23,5 @@
> +
> +  // Test device where UA field is blank
> +  switchDevice(ui, "Laptop (1366 x 768)");
> +  yield waitForViewportResizeTo(ui, 1366, 768);
> +  yield testUserAgent(ui, DEFAULT_UA);

It would be great to also check for the regression I noticed during the previous review in this test.  So, something like choose a device, nudge the resize gripper (borrow helpers from browser_mouse_resize.js), verify the device selector label says "no device selected".

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 29 • 8 years ago

Comment on attachment 8783265 [details] [diff] [review]
Add user agent emulation to new RDM

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

Great, thanks for working on this!

::: devtools/client/responsive.html/components/resizable-viewport.js
@@ +106,5 @@
>  
>      // Update the viewport store with the new width and height.
>      this.props.onResizeViewport(width, height);
>      // Change the device selector back to an unselected device
> +    this.props.onChangeViewportDevice({name: ""});

Nit: use `{ name: "" }` style (wish we could lint one way or the other, but I've tried to be consistent in new RDM at least...)

::: devtools/client/responsive.html/components/viewport-dimension.js
@@ +113,5 @@
>        return;
>      }
>  
>      // Change the device selector back to an unselected device
> +    this.props.onChangeViewportDevice({name: ""});

Nit: use `{ name: "" }` style (wish we could lint one way or the other, but I've tried to be consistent in new RDM at least...)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 30 • 8 years ago

Also, feel free to file a follow up to discuss UX options with Helen to make it more obvious that a UA is applied.

	Pulsebot
	Comment 31 • 8 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/fec37519e65f
Move custom UA emulation to emulation actor. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/6f5b69e1d1a9
Change old RDM to use emulation actor. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/bba8077c6c0a
Add user agent emulation to new RDM. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/0faec8f3a8b5
Test for device list. r=jryans

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fec37519e65f
https://hg.mozilla.org/mozilla-central/rev/6f5b69e1d1a9
https://hg.mozilla.org/mozilla-central/rev/bba8077c6c0a
https://hg.mozilla.org/mozilla-central/rev/0faec8f3a8b5

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 33 • 8 years ago

For testing this:

1. Start RDM
2. Choose a device
  * A new user agent is now applied
3. Open the DevTools console, and check "navigtor.userAgent"
  * It should have a new value that depends on the device
4. Reload the page with network monitor open
  * For each request, the User-Agent request header should match the new device value as well

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 34 • 8 years ago

This causes the frequent intermittent bug 1297523.

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 35 • 8 years ago

I managed to reproduce this issue on Firefox 50.0a1 (2016-07-27), under Windows 10x64, using the STR from Comment 33.

I've performed the same STR on Firefox 51.0a1 (2016-09-12) across platforms [1] and for a part of the devices, the User Agent matches the new device value, but for the other part of the devices, the default Firefox user agent is displayed. For eg. Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 user agent is displayed for the devices displayed below:
- Laptop (1366x768)
- Laptop (1920x1080)
- Laptop (1920x1080) with touch
- LG watch 		
- LG watch R
- Samsung Gear Live
- 1080 Full HD Television				
- 4k Ultra HD Television
- 720p HD Televisoin
The same result is also displayed on Mac OS X and on Ubuntu.

Is this an expected behavior?

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

	Tim Nguyen :ntim Assignee
	Comment 36 • 8 years ago

(In reply to Mihai Boldan, QA [:mboldan] from comment #35)
> I managed to reproduce this issue on Firefox 50.0a1 (2016-07-27), under
> Windows 10x64, using the STR from Comment 33.
> 
> I've performed the same STR on Firefox 51.0a1 (2016-09-12) across platforms
> [1] and for a part of the devices, the User Agent matches the new device
> value, but for the other part of the devices, the default Firefox user agent
> is displayed. For eg. Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0)
> Gecko/20100101 Firefox/51.0 user agent is displayed for the devices
> displayed below:
> - Laptop (1366x768)
> - Laptop (1920x1080)
> - Laptop (1920x1080) with touch
That's expected, those devices have generic names.

> - LG watch 		
> - LG watch R
> - Samsung Gear Live
That sounds like a bug with the device definitions. Can you file an issue here: https://github.com/mozilla/simulated-devices/ ?

> - 1080 Full HD Television				
> - 4k Ultra HD Television
> - 720p HD Televisoin
Seems expected as well, as the device definition considers them as Firefox OS devices.

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 37 • 8 years ago

I've logged Bug 1302964 and https://github.com/mozilla/simulated-devices/issues/16 based on Comment 36.
Since there are no other found issues, I am marking this bug Verified Fixed.