828008 - Provide UI to emulate custom user agent

Status: RESOLVED FIXED

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

Description

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9]]

If devtools can change/override Firefox's user agent strings to Firefox Mobile, It'll be useful for debugging, I seem.

Comment 1

[Rob Campbell [:rc] (:robcee)]

definitely. Thanks for filing this.

Comment 2

[Rob Campbell [:rc] (:robcee)]

not sure what the best way to expose this would be. We already have several useragent add-ons that can provide this feature.

wonder if a GCLI command would work?

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> not sure what the best way to expose this would be. We already have several
> useragent add-ons that can provide this feature.
> 
> wonder if a GCLI command would work?

I thought add the checkbox which change/UA strings to toolbox like scratchpad/tilt/responsive view if the feature switches Firefox Desktop/Mobile only.
But maybe GCLI command is good for this purpose. It is flexible.

Comment 4

[Rob Campbell [:rc] (:robcee)]

also, no reason we couldn't do both.

Comment 5

[Dane MacMillan [:danemacmillan]]

This would be great to have. A possible location could in the "Toolbox Options," but having it inline with some of the other buttons, next to "3D View" or "Responsive Design Mode," for example, could be even better and more accessible. An additional location, which I think would be incredible, would be to also have the option to switch user agents directly in the "Responsive Design Mode" view, with an option next to the ones currently available (for choosing dimensions, rotating, simulating touch events, and taking a screenshot); this location makes a lot of sense, considering the huge shift to mobile.

Comment 7

[Tim Nguyen :ntim]

Maybe creating some kind of emulation tab, with the ability to emulate user agents, media queries, ...

Comment 8

[Dane MacMillan [:danemacmillan]]

I think that's a good idea. If such a feature were created, it could probably replace the "Responsive Design Mode" tab. "Emulator" is arguably a more precise nomenclature for such a body of tools, too. All the tools under "Responsive Design Mode" would fit very well under "Emulator," as well as the addition of new, proposed tools related to this area, such as user agent switching.

Comment 9

[Rob Campbell [:rc] (:robcee)]

this could be a feature. I'm moving this into our User Stories component to get some product management love.

Comment 10

[Aaron Train [:aaronmt]]

Helpful for web-compat testing, I currently use https://addons.mozilla.org/en-US/firefox/addon/user-agent-quick-switch/, in Chrome, I just use the built in override in their emulation tab in the inspector.

/me adds a bug vote

Comment 11

[Tim Nguyen :ntim]

(In reply to Aaron Train [:aaronmt] from comment #10)
> Helpful for web-compat testing, I currently use
> https://addons.mozilla.org/en-US/firefox/addon/user-agent-quick-switch/, in
> Chrome, I just use the built in override in their emulation tab in the
> inspector.
> 
> /me adds a bug vote
I don't like the UI of this one, doesn't feel integrated :(

Comment 12

[Dane MacMillan [:danemacmillan]]

> in Chrome, I just use the built in override in their emulation tab in the inspector.

Chrome definitely has a nice feature set for this kind of functionality, and would probably be a good archetype for a native version in Firefox's Developer Tools.

> I currently use https://addons.mozilla.org/en-US/firefox/addon/user-agent-quick-switch/

I also sometimes use a plugin for this, for lack of any native tool. Such a tool should be important enough to deserve its own spot in the Developer Tools roster, as just about any modern development workflows almost demands these tests. Five years ago I was regularly testing stuff for IE compatibility; today, that spot has pretty much been usurped by mobile testing, and in some cases, has taken priority over desktop. 

As I was writing this, a client on call at my workplace was arguing with the devteam about only releasing a mobile version of their product and blocking desktop traffic, which is obviously a very misinformed and unintelligent position, but it indicates how important mobile has become for a lot of companies that dabble in tech.

Having some kind of emulation built in to the Developer Tools' "Responsive Design Mode" would greatly facilitate this sort of testing. I actually use the Chrome DevTools when it comes to this sort of thing, instead of running a few plugins in Firefox to achieve the same purpose.

A great first step in this area for the Developer Tools would be the ability to switch user agents while in "Responsive Design Mode."

Comment 13

[Dane MacMillan [:danemacmillan]]

Can this bug be renamed to something that makes mention of mobile emulation? I'm seeing a number of seemingly disparate bug reports or requests that would ultimately fit very nicely in a conversation about mobile emulation as a whole. On just the UserVoice feedback pages I've found at least three different requests that could be combined to provide a more unified voice behind mobile emulation; I know there are reports on BugZilla that would also fit nicely. All of them are requesting features that fall perfectly under such a named tool set.

I think it's fair to say this thread has the potential to be a more open-ended discussion about mobile emulation, and not just about user agent switching. It would be great to have a single entry of discussion instead of having a spread of conversations across different locations, so no valuable feedback is lost while this feature is considered. I'll tie everything together if this can happen, because we're not getting the most accurate picture in terms of demand.

Comment 14

[Tim Nguyen :ntim]

We could provide emulation options like Chrome or IE does.

Comment 15

[Rob Campbell [:rc] (:robcee)]

no, we're not building an emulator here. The idea behind this bug was to be able to change useragent strings so the browser would *appear* to be another browser, maybe mobile or otherwise.

If we want to create device emulators, that would be a pretty heavy bit of work (think of CSS prefixes, touch events, etc). Not saying it's not worth discussing but this bug could provide a lot of benefit and be reasonably easy to do.

Comment 16

[Tim Nguyen :ntim]

(In reply to Rob Campbell [:rc] (:robcee) from comment #15)

> If we want to create device emulators, that would be a pretty heavy bit of
> work (think of CSS prefixes, touch events, etc). Not saying it's not worth
> discussing but this bug could provide a lot of benefit and be reasonably
> easy to do.
I'm not talking about full device emulation. I'm talking of emulating things like : Geolocation, media queries, and viewports.

Comment 17

[Dane MacMillan [:danemacmillan]]

As Tim stated, it would be the emulation of devices in the most superficial sense. That is sufficient--and really exactly what developers want--as most modern front-end breakpoints are superficial: stuff like user agent, viewport dimensions, and locale are the most common breakpoints developers build against. Stuff like vendor prefixes and true emulation in the engine sense are overkill, and not the purpose of the request for changing the overall direction of the report.

If you look at the emulation options in Chrome, they are also superficial, but truly sufficient--and moreover, precisely what is needed--to get the job done. If we wanted genuine emulation on the engine level, we open another browser; that is still very necessary for most developers' workflows.

Comment 18

[Jan Keromnes [:janx]]

As Tim mentioned, switching User Agent strings plays well with other device-emulation features like geolocation, media queries and viewports.

In bug 1028905 comment 15, I suggest adding a "Device" toolbox panel for all these, with (among other things) an input field controlling the User Agent string, and a list of common device profiles that have preset User Agents. It's basically the same idea as comment 7 or comment 8.

Comment 20

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 21

[Brian Grinstead [:bgrins]]

Is there a way to change the UA for a single tab target?  From what I understand, the current UA switching  is either global across the entire browser, or per-domain.  This would cause having the tools opened in one tab to have major side effects across the rest of the opened tabs, which would be a big problem.

Comment 22

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

(In reply to Brian Grinstead [:bgrins] from comment #21)
> Is there a way to change the UA for a single tab target?  From what I
> understand, the current UA switching  is either global across the entire
> browser, or per-domain.  This would cause having the tools opened in one tab
> to have major side effects across the rest of the opened tabs, which would
> be a big problem.

That's my understanding as well.  Platform work is needed to be able to set this per tab / docshell / something.

Comment 23

[Jan Keromnes [:janx]]

I had a look at this earlier, and your suspicions are correct: no way to switch UA for a single tab target without platform change, only browser-wide or per-domain (also per-subdomain) UA changes.

However, while it would be very nice to have per-tab UA switch (we'll want that eventually), I don't think it's too bad UX to create a tool that switches UA globally for the whole browser (initially). The reason I think so is that users who need to change UA in Firefox currently use one of the popular addons to do that; and these all switch the UA globally, which from what I understand is not a major pain point (maybe not even a minor pain point, as long as the UI explicitly shows that any change is global to the browser).

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Jan Keromnes [:janx] from comment #23)
> However, while it would be very nice to have per-tab UA switch (we'll want
> that eventually), I don't think it's too bad UX to create a tool that
> switches UA globally for the whole browser (initially). The reason I think
> so is that users who need to change UA in Firefox currently use one of the
> popular addons to do that; and these all switch the UA globally, which from
> what I understand is not a major pain point (maybe not even a minor pain
> point, as long as the UI explicitly shows that any change is global to the
> browser).

For the built in tools, there should be an expectation that opening them on tab 1 won't mess with tab 2.  That's why things like 'disable cache' and 'disable js' are active only on the tab that the toolbox is targeting, and only while the toolbox is opened.

I can imagine lots of situations where changing the UA would cause problems with other tabs (some navigation on a different tab happens and you get served a mobile page, or JS on another page detects the UA string and does something different).  Then to debug this, you would need to be sure that the emulation features aren't enabled on some other tab.

We should at least investigate how much platform work this will be before deciding what to do.

Comment 25

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

I agree that per-tab control is likely needed before the DevTools can offer something, since most of our tools operate with a per-tab UX in general.

I'll plan to at least investigate the platform work when I return from PTO.

Comment 26

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

Clearing assignment for now, since we've moved this out of devedition-40.

Comment 27

[Tim Nguyen :ntim]

Platform work is being done in bug 1137681.

Comment 28

[Tim Nguyen :ntim]

Attachment: Part 1: Create a new actor for user agent emulation

Comment 29

[Tim Nguyen :ntim]

Attachment: Part 2: UI in network monitor

This is just a test UI for the devtools actor, nothing really final here.

Comment 30

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

Comment on attachment 8687691 [details] [diff] [review]
Part 1: Create a new actor for user agent emulation

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

In the past, we've thrown such `docshell` settings into `TabActor#reconfigure`[1], however those have largely been just boolean flags so far, while you have a silghtly larger API here.

I think the most important piece is ensuring we reset to the default UA in all cases: toolbox closes cleanly, server dies unexpectedly, etc.

Alex, any opinion on new actor vs. `TabActor#reconfigure`?

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1410

::: devtools/server/actors/useragent.js
@@ +33,5 @@
> +                      .getInterface(Ci.nsIWebNavigation)
> +                      .QueryInterface(Ci.nsIDocShell);
> +  },
> +
> +  get currentUA() {

This seems unused, did you mean to expose it over the protocol?

Also, maybe it's "more correct" for testing, etc. to check `window.navigator.userAgent` and skip the conditional?  I assume that's supposed have the current value in all cases.

Comment 31

[Tim Nguyen :ntim]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30)
> Comment on attachment 8687691 [details] [diff] [review]
> Part 1: Create a new actor for user agent emulation
> 
> Review of attachment 8687691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the past, we've thrown such `docshell` settings into
> `TabActor#reconfigure`[1], however those have largely been just boolean
> flags so far, while you have a silghtly larger API here.
> 
> I think the most important piece is ensuring we reset to the default UA in
> all cases: toolbox closes cleanly, server dies unexpectedly, etc.
I was originally unsure whether I should add the event listener that from the server or from the client. But now that you mention it, I guess I should do that from the server.
> Alex, any opinion on new actor vs. `TabActor#reconfigure`?
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> webbrowser.js#1410
I've checked this file, and I'm not too fond of putting the UA stuff in there. The docshell flags in there seem very closely linked to the settings, there's even a toggleDevToolsSettings function in there. UA emulation doesn't really fit as a setting, and if we include it in the file, the code in there should be cleaned up a bit (seperate the settings bits from the file).

> ::: devtools/server/actors/useragent.js
> @@ +33,5 @@
> > +                      .getInterface(Ci.nsIWebNavigation)
> > +                      .QueryInterface(Ci.nsIDocShell);
> > +  },
> > +
> > +  get currentUA() {
> 
> This seems unused, did you mean to expose it over the protocol?
I guess isUASpoofed should be enough. currentUA can be used to display the currently used UA in the UI, but yeah, just knowing isUASpoofed is enough since you can easily get the default UA without the actor.

Comment 32

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

(In reply to Tim Nguyen [:ntim] from comment #31)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30)
> > ::: devtools/server/actors/useragent.js
> > @@ +33,5 @@
> > > +                      .getInterface(Ci.nsIWebNavigation)
> > > +                      .QueryInterface(Ci.nsIDocShell);
> > > +  },
> > > +
> > > +  get currentUA() {
> > 
> > This seems unused, did you mean to expose it over the protocol?
> I guess isUASpoofed should be enough. currentUA can be used to display the
> currently used UA in the UI, but yeah, just knowing isUASpoofed is enough
> since you can easily get the default UA without the actor.

It's probably better that we *do* expose it though, right?  Seems like a natural place to make it easy to read the UA for a UI, just like you say.  When you're connected to some remote device, you don't know it's UA easily without sending something over the RDP, so it seems natural to offer a way to read it.

Comment 33

[Alexandre Poirot [:ochameau]]

Comment on attachment 8687691 [details] [diff] [review]
Part 1: Create a new actor for user agent emulation

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30)
> Alex, any opinion on new actor vs. `TabActor#reconfigure`?

reconfigure is going to handle features you want:
- auto reset correctly on all the various cases you want to reset (And trust me that's not easy! And yes it has to be done from server side)
- optionally reload the document to see the changes

You could handle auto reset correctly, by sending a "detach" event from the tab actor, like window-destroyed event for ex for from _detach function. Then you UserAgent actor could listen to this and clean itself up. As jryans said, this is important to handle that correctly.

But then, if the UserAgent is also going to reload the page... I think it is better to make it into reconfigure request. `reconfigure` is mostly used for settings panel, but not only. It is meant to be generic. I can only see the function names/comments as being disturbing, having a string or a boolean makes no difference here. So I don't see what we would have to refactor to support useragent?

Also, we shouldn't bring up actors with unused code/requests. At the end, you are only using setUserAgent.
I know that the UI patch is just a first shot, but at the end we should be careful to not introduce things that aren't used.

Comment 34

[Tim Nguyen :ntim]

(In reply to Alexandre Poirot [:ochameau] from comment #33)
> Comment on attachment 8687691 [details] [diff] [review]
> Part 1: Create a new actor for user agent emulation
> 
> Review of attachment 8687691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30)
> > Alex, any opinion on new actor vs. `TabActor#reconfigure`?
> 
> reconfigure is going to handle features you want:
> - auto reset correctly on all the various cases you want to reset (And trust
> me that's not easy! And yes it has to be done from server side)
> - optionally reload the document to see the changes
> 
> You could handle auto reset correctly, by sending a "detach" event from the
> tab actor, like window-destroyed event for ex for from _detach function.
> Then you UserAgent actor could listen to this and clean itself up. As jryans
> said, this is important to handle that correctly.
>
> But then, if the UserAgent is also going to reload the page...
My latest platform patch works without reloading. \o/

> I think it is better to make it into reconfigure request. `reconfigure` is mostly used for
> settings panel, but not only. It is meant to be generic. I can only see the
> function names/comments as being disturbing, having a string or a boolean
> makes no difference here. So I don't see what we would have to refactor to
> support useragent?
Fair enough, but I'm a bit worried about importing that big file just for the UA stuff. It may be worth splitting out the docshell flags (disable JS, disable cache) out in a new actor and put the UA stuff there.

> Also, we shouldn't bring up actors with unused code/requests. At the end,
> you are only using setUserAgent.
> I know that the UI patch is just a first shot, but at the end we should be
> careful to not introduce things that aren't used.
Yeah, this is just a first shot, so I don't know what we'll need in the final UI.

Comment 35

[Alexandre Poirot [:ochameau]]

(In reply to Tim Nguyen [:ntim] from comment #34)
> (In reply to Alexandre Poirot [:ochameau] from comment #33)
> > But then, if the UserAgent is also going to reload the page...
> My latest platform patch works without reloading. \o/

I imagine navigator.userAgent can be modified live, but that won't modify all content already generated on client side, nor all server side logic.

> Fair enough, but I'm a bit worried about importing that big file just for
> the UA stuff. It may be worth splitting out the docshell flags (disable JS,
> disable cache) out in a new actor and put the UA stuff there.

webbrowser.js and especially TabActor is loaded no matter what if you open a toolbox.
Note that I'm in sync with jryans here. It would be ok to have another actor, I just prefer seeing that integrated into TabActor. But depending on your additional needs it may make sense to pull this out.

Comment 36

[Tim Nguyen :ntim]

Attachment: Part 1 - TabActor changes

Comment 37

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Comment 38

[Alexandre Poirot [:ochameau]]

Comment on attachment 8699659 [details] [diff] [review]
Part 1 - TabActor changes

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

Looks good, thanks.

What about having a test?
Note that it could be at Responsive design level, it would cover both client and server.

Comment 39

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

Comment on attachment 8699659 [details] [diff] [review]
Part 1 - TabActor changes

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

See comments to update the restore behavior.

I agree a test would also be good to have here.  Maybe it would show up in the other patch, if it's a test at the RD UI level.

::: devtools/server/actors/webbrowser.js
@@ +1493,5 @@
>    _restoreDocumentSettings: function () {
>      this._restoreJavascript();
>      this._setCacheDisabled(false);
>      this._setServiceWorkersTestingEnabled(false);
> +    this._setCustomUserAgent("");

We need to change this to use a strategy like `restoreJavascript`, which:

a. Only writes a value on close if this actor ever made a previous change to the value
b. Tracks the "previous" value at the time of first write by this actor

In more detail, a problem arises with the current version when both RD and the toolbox connects: there is a separate actor instance for each one.  So, if you try the following steps:

1. Set custom UA from new input in RD
2. Open toolbox
3. Check navigator.userAgent (it matches step 1)
4. Close toolbox
5. Open toolbox
6. Check navigator.userAgent (it's been reset to default by step 4)

With the modified strategy I am suggesting, we avoid this problem because the toolbox's tab actor will never write the UA, so it will also never attempt to restore it to some confusing value.

Comment 40

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

Comment on attachment 8699660 [details] [diff] [review]
Part 2 - UI for responsive mode

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

Overall, I think this UI is nice given the simplicity we are working with, since there's no device list yet, etc.  If you have a custom UA you want to try, you can paste it in.  If not, it doesn't really get in the way too much.  It also makes it quite clear when a custom UA is active with the bright blue highlight.

Please prepare a try build a request a ui-review from :helenvholmes, so we can get her thoughts as well.

Also, as mentioned in the other patch's review, let's add a test for this.  If you need help writing the test, please let me know.

Thanks for working on this!

::: devtools/client/locales/en-US/responsiveUI.properties
@@ +18,5 @@
>  # LOCALIZATION NOTE  (responsiveUI.screenshot): tooltip of the screenshot button.
>  responsiveUI.screenshot=Screenshot
>  
> +# LOCALIZATION NOTE  (responsiveUI.userAgent): placeholder for the user agent input.
> +responsiveUI.userAgent=User Agent

Does it seem more obvious what to do with the field if it says "Custom User Agent"?  I am not sure either way...  I am hoping you and/or Helen will have an opinion.

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +203,5 @@
>    this.bound_close = this.close.bind(this);
>    this.bound_startResizing = this.startResizing.bind(this);
>    this.bound_stopResizing = this.stopResizing.bind(this);
>    this.bound_onDrag = this.onDrag.bind(this);
> +  this.changeUA = this.changeUA.bind(this);

Nit: Use the `bound_` style to match the rest of this file.

@@ +256,5 @@
> +      DebuggerServer.init();
> +      DebuggerServer.addBrowserActors();
> +    }
> +    this.client = new DebuggerClient(DebuggerServer.connectPipe());
> +    this.client.connect(() => {

Alex was going to update `connect` to support promises, but it seems it has not yet been done...  You could use promise style here anyway by wrapping this call.  If you go with promises, you can use `Task.async` and `yield` in this function to make it easier to read.

@@ +261,5 @@
> +      this.client.getTab().then((tabResponse) => {
> +        let tab = tabResponse.tab;
> +        this.client.attachTab(tab.actor, (response, tabClient) => {
> +          if (!tabClient) {
> +            Cu.reportError("Error: failed to attach tab");

`console.error` might be nicer, since it reports a stack trace.  Also, probably good to mention the context, that this is responsive design, etc.

@@ +264,5 @@
> +          if (!tabClient) {
> +            Cu.reportError("Error: failed to attach tab");
> +            return;
> +          }
> +          this.tabActor = tabClient;

Let's keep the name `tabClient` on `this` as well to reduce confusion of terms.

@@ +478,5 @@
>      this.toolbar.appendChild(this.screenshotbutton);
>  
> +    this.userAgentInput = this.chromeDoc.createElement("textbox");
> +    this.userAgentInput.className = "devtools-responsiveui-textinput";
> +    this.userAgentInput.setAttribute("placeholder", this.strings.GetStringFromName("responsiveUI.userAgent"));

Use "placeholder" in the string ID, such as "responsiveUI.userAgentPlaceholder"

@@ +879,5 @@
>         }
>       }
>     }),
>  
> +  changeUA: function RUI_ChangeUA() {

Nit: Named functions go against the linting rules, so remove the `RUI_ChangeUA`.  The rest of the file will get cleaned up eventually (or removed).

@@ +884,5 @@
> +    var value = this.userAgentInput.value;
> +    if (value !== "") {
> +      this.userAgentInput.setAttribute("attention", "true");
> +    }
> +    else {

Nit: Put this on the previous line (eslint should warn for this).

Comment 41

[Tim Nguyen :ntim]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40)
> ::: devtools/client/locales/en-US/responsiveUI.properties
> @@ +18,5 @@
> >  # LOCALIZATION NOTE  (responsiveUI.screenshot): tooltip of the screenshot button.
> >  responsiveUI.screenshot=Screenshot
> >  
> > +# LOCALIZATION NOTE  (responsiveUI.userAgent): placeholder for the user agent input.
> > +responsiveUI.userAgent=User Agent
> 
> Does it seem more obvious what to do with the field if it says "Custom User
> Agent"?  I am not sure either way...  I am hoping you and/or Helen will have
> an opinion.
"Custom User Agent" could work. We could as well put "Default user agent" as placeholder, since if the input is empty, no custom UA is applied. I don't really have a preference though, I'll leave that to Helen.

> @@ +261,5 @@
> > +      this.client.getTab().then((tabResponse) => {
> > +        let tab = tabResponse.tab;
> > +        this.client.attachTab(tab.actor, (response, tabClient) => {
> > +          if (!tabClient) {
> > +            Cu.reportError("Error: failed to attach tab");
> 
> `console.error` might be nicer, since it reports a stack trace.  Also,
> probably good to mention the context, that this is responsive design, etc.
Isn't `console.error` available only with Console.jsm ? Is it worth importing the file ?

Comment 42

[Tim Nguyen :ntim]

Attachment: Part 1 - TabActor changes

This works fine.

Comment 43

[Tim Nguyen :ntim]

Attachment: Part 2 - Add UI in responsive mode

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58846d696e17
Helen, can you take a look at the UI please ? I'd like to know if the interaction seems right to you and your opinion on comment 41.


:jryans, I plan to add the test in a third patch tomorrow.

Comment 44

[Tim Nguyen :ntim]

Attachment: Part 1 - TabActor changes

Comment 45

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for Responsive Mode

Comment 46

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Comment 47

[Tim Nguyen :ntim]

Comment on attachment 8701446 [details] [diff] [review]
Part 2 - UI for Responsive Mode

Try build:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b37215fceb5c

Comment 48

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Removed an unrelated change

Comment 49

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

Comment on attachment 8701445 [details] [diff] [review]
Part 1 - TabActor changes

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

Great, the restore now seems to function correctly for all the cases I could think of. :)

::: devtools/server/actors/webbrowser.js
@@ +1594,5 @@
> +  /**
> +   * Sets custom user agent for the current tab
> +   */
> +  _setCustomUserAgent: function(userAgent) {
> +    if (this._previousCustomUserAgent == null) {

Since we're giving a very specific meaning to null here, let's use `===`.

@@ +1604,5 @@
> +  /**
> +   * Restore the user agent, before the actor modified it
> +   */
> +  _restoreUserAgent: function() {
> +    if (this._previousCustomUserAgent != null) {

Since we're giving a very specific meaning to null here, let's use `!==`.

Comment 50

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

Comment on attachment 8701446 [details] [diff] [review]
Part 2 - UI for Responsive Mode

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

Code changes seem good, so let's see what :helenvholmes thinks about the UI.

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +885,5 @@
> +      this.userAgentInput.setAttribute("attention", "true");
> +    } else {
> +      this.userAgentInput.removeAttribute("attention", "true");
> +    }
> +    this.tabClient.reconfigure({"customUserAgent": value});

Nit: don't really need quotes here

Comment 51

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

Comment on attachment 8701448 [details] [diff] [review]
Part 3 - Test

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

Great, thanks for adding a test!

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +265,5 @@
>              return;
>            }
>            this.tabClient = tabClient;
>            this.userAgentInput.hidden = false;
> +          ResponsiveUIManager.emit("connected-to-server", { tab: this.tab });

Nit: Looks like the prior style in this file is camelCase for event names like `contentResize`, instead of hyphens.

Comment 52

[Tim Nguyen :ntim]

Attachment: Part 1 - TabActor changes

Addressed comments.

Comment 53

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Addressed comments.

Comment 54

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Addressed comments + Made setUserAgent a sync function (it was unnecessarily async earlier)

Comment 55

[Tim Nguyen :ntim]

Helen, comments 40 and 41 pretty much sums up the UX related conversation. Also, if it eases up your ui review, here are some try builds: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-b37215fceb5cb6fc9332b3973e5ca17f82e71679/

Comment 56

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8701639 [details] [diff] [review]
Part 2 - UI for responsive mode

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

I'd like "Custom User Agent" as a placeholder, since it is sort of bizarre on its own, I think. Since it's only placeholder I think it's not worth nitpicking too much at the moment.

I'm noticing some lag when you rotate a viewport; any ideas on what could be causing that? Doesn't feel smooth enough to be an animation, slow enough that it feels like jank. I'd like to see that resolved. Other than that I'm feeling good about this iteration of the UI.

Comment 57

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Changed placeholder to "Custom User Agent"

The rotation lag seems like a separate issue to me.

Comment 58

[Tim Nguyen :ntim]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b37215fceb5c

Comment 59

[Pulsebot]

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

Comment 60

[Tim Nguyen :ntim]

(In reply to Pulsebot from comment #59)
> https://hg.mozilla.org/integration/fx-team/rev/f28fd9bd845c

All 3 patches should have been checked in.

Comment 61

[Carsten Book [:Tomcat]]

(In reply to Tim Nguyen [:ntim] from comment #60)
> (In reply to Pulsebot from comment #59)
> > https://hg.mozilla.org/integration/fx-team/rev/f28fd9bd845c
> 
> All 3 patches should have been checked in.

oh sorry done now!

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/773075ac2210
https://hg.mozilla.org/integration/fx-team/rev/6dbba51c24d2

Comment 63

[Tim Nguyen :ntim]

Attachment: Part 4 - Fix mochitest-e10s-dt failures on fx-team

Comment 64

[Victor Porof [:vporof][:vp]]

Comment on attachment 8702630 [details] [diff] [review]
Part 4 - Fix mochitest-e10s-dt failures on fx-team

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

LG

Comment 65

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

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

Comment 66

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Backed out all three patches in https://hg.mozilla.org/integration/fx-team/rev/ab22ba41b8fc for failing M(dt2) on Linux x64 debug and OS 10.6 debug and M-e10s(dt5) on Windows 7 debug.

Failing push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=6dbba51c24d2

M(dt2):
https://treeherder.mozilla.org/logviewer.html#?job_id=6356458&repo=fx-team

TEST-UNEXPECTED-FAIL | leakcheck | default process: 4005291 bytes leaked (AnimationTimeline, AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, ...)

M-e10s(dt5)
TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js | leaked 1 docShell(s) until shutdown

Comment 67

[Tim Nguyen :ntim]

Comment on attachment 8701638 [details] [diff] [review]
Part 1 - TabActor changes

This part wasn't backed out (It's not a problem though).

Comment 68

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Folded parts 3 & 4.

Comment 69

[Tim Nguyen :ntim]

Attachment: Part 4 - Wait for all async tasks to finish before emitting "off" event

Hopefully this should fix the 2 last bustages. I'll push to try to be sure.

Comment 70

[Tim Nguyen :ntim]

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

Comment 71

[Tim Nguyen :ntim]

Comment on attachment 8702721 [details] [diff] [review]
Part 4 - Wait for all async tasks to finish before emitting "off" event

So, this only fixed one of the failures (dt5 Windows 7 debug, browser_styleeditor_media-sidebar-links.js), but it still makes bug 1186675 a permanent leak. Anyway, I'd still like to gather some feedback on this patch.

Besides, I think bug 1189015 fixes the leak.

I don't know who's the most appropriate person to review this since a lot of people are on PTO.

Comment 72

[Alexandre Poirot [:ochameau]]

Comment on attachment 8702721 [details] [diff] [review]
Part 4 - Wait for all async tasks to finish before emitting "off" event

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

Please resubmit new patch instead of just the interdiff as I haven't reviewed the previous patch.
And it looks like you haven't followed what :jryans suggested, like using promises/tasks in connectToServer.
changeUA should also wait for tabClient to be available somehow. Having connectToServer be a task would help.
tabClient may not be defined yet by the time we call changeUA as connectToServer is full of async calls.

Also, do you have a particular test in mind or an exception on try that would be fixed with this patch?
browser_styleeditor_media-sidebar-links.js? I can't find a try push with this test failing.
 
Your original patch may be much more complex than it seems.
I imagine that this new DebuggerClient instanciated by responsive mode can easily overlap between tests.
Look for responsivedesign.jsm in test logs, there is many exceptions. Like this one:
  TypeError: this._progressListener is null
  http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-a11841ccdecd0954a637917d10d3aa39b888a9fb/try-win64-debug/try_win8_64-debug_test-mochitest-devtools-chrome-4-bm126-tests1-windows-build236.txt.gz
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda407b7ae90&selectedJob=14942424
You might have to fix many tests (all the one involving responsive mode) in order to ensure waiting for this "off" event.
I can easily see many tests that wouldn't...

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +334,4 @@
>      this._telemetry.toolClosed("responsive");
>      let childOff = () => {
>        this.mm.removeMessageListener("ResponsiveMode:Stop:Done", childOff);
> +      if (this.client) {

Is there a case where this.client is null here, that would be surprising?

Comment 73

[Tim Nguyen :ntim]

(In reply to Alexandre Poirot [:ochameau] from comment #72)
> Comment on attachment 8702721 [details] [diff] [review]
> Part 4 - Wait for all async tasks to finish before emitting "off" event
> 
> Review of attachment 8702721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please resubmit new patch instead of just the interdiff as I haven't
> reviewed the previous patch.
> And it looks like you haven't followed what :jryans suggested, like using
> promises/tasks in connectToServer.
This is more tricky than you think, most of the Responsive mode code was designed to be synchronous.

> changeUA should also wait for tabClient to be available somehow. Having
> connectToServer be a task would help.
It does, the UA input is hidden until tabClient is available. I doubt an user would call changeUA manually from the browser console.

> tabClient may not be defined yet by the time we call changeUA as
> connectToServer is full of async calls.
> 
> Also, do you have a particular test in mind or an exception on try that
> would be fixed with this patch?
> browser_styleeditor_media-sidebar-links.js? I can't find a try push with
> this test failing.
fx-team push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=426164db47c6&selectedJob=6358495

> Your original patch may be much more complex than it seems.
> I imagine that this new DebuggerClient instanciated by responsive mode can
> easily overlap between tests.
> Look for responsivedesign.jsm in test logs, there is many exceptions. Like
> this one:
>   TypeError: this._progressListener is null
>
> http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> a11841ccdecd0954a637917d10d3aa39b888a9fb/try-win64-debug/try_win8_64-
> debug_test-mochitest-devtools-chrome-4-bm126-tests1-windows-build236.txt.gz
>  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dda407b7ae90&selectedJob=14942424
> You might have to fix many tests (all the one involving responsive mode) in
> order to ensure waiting for this "off" event.
> I can easily see many tests that wouldn't...
I've seen some tests that just close the tab. Doesn't that call the close function ?

> ::: devtools/client/responsivedesign/responsivedesign.jsm
> @@ +334,4 @@
> >      this._telemetry.toolClosed("responsive");
> >      let childOff = () => {
> >        this.mm.removeMessageListener("ResponsiveMode:Stop:Done", childOff);
> > +      if (this.client) {
> 
> Is there a case where this.client is null here, that would be surprising?
Most of the test wait for the `on` event, but the `on` event doesn't wait for the DebuggerClient to be created. Replacing the `on` event with the `connectedToServer` event makes more tests fail.

Comment 74

[Alexandre Poirot [:ochameau]]

(In reply to Tim Nguyen [:ntim] from comment #73)
> This is more tricky than you think, most of the Responsive mode code was
> designed to be synchronous.

It doesn't mean ResponsiveUI has to be a promise, but having connectToServer handle a promise would help doing what I suggest in onChangeUA.

> > changeUA should also wait for tabClient to be available somehow. Having
> > connectToServer be a task would help.
> It does, the UA input is hidden until tabClient is available. I doubt an
> user would call changeUA manually from the browser console.

Tests most likely doesn't wait for unhidding.

> I've seen some tests that just close the tab. Doesn't that call the close
> function ?

It will, but it won't wait for responsive mode cleanup, so that the debugger client may still be in process of cleaning up itself.

> Most of the test wait for the `on` event, but the `on` event doesn't wait
> for the DebuggerClient to be created. Replacing the `on` event with the
> `connectedToServer` event makes more tests fail.

It seems to say that the tests are already weak :/
Or it highlights the fact that we doesn't wait correctly for this client to be completely destroyed before moving on to another test. Waiting for TabClose isn't enough.

Comment 75

[Tim Nguyen :ntim]

Part 1 landed on m-c: https://hg.mozilla.org/mozilla-central/rev/f28fd9bd845c

Comment 76

[Tim Nguyen :ntim]

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

Comment 77

[Tim Nguyen :ntim]

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

Comment 78

[Tim Nguyen :ntim]

With bug 1235953 and without part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ef15d8af170

With bug 1235953 and with part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bcddaf7c3e6

Comment 79

[Tim Nguyen :ntim]

Attachment: Part 4 - Wait for all async tasks to finish before emitting on and off events

The responsive mode GCLI command test fails, probably because checks are not done in a async way. Past that, I'd still like to have a code review of this patch.

Comment 80

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

Comment on attachment 8706104 [details] [diff] [review]
Part 4 - Wait for all async tasks to finish before emitting on and off events

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

For GCLI, maybe you need to capture the `once` promise *before* the each GCLI command runs, and then yield it *after*?  It's possible the on / off event was already emitted before your call to `once` took place.

Does it seem like this change causes the page to open more slowly than before?  I am just curious since we're delaying toggling some UI parts now.  I tried it locally it seems pretty speedy though.  Let me know if it feels slower to open to you.

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +182,5 @@
> +    let waitForMessage = (message) => {
> +      return new Promise((resolve, reject) => {
> +        let listener = () => {
> +          resolve();
> +          this.mm.removeMessageListener(message, listener);

Remove the listener first.

@@ +252,5 @@
> +
> +    let {tab} = yield this.client.getTab();
> +
> +    let tabClient = yield new Promise((resolve, reject) => {
> +      this.client.attachTab(tab.actor, (response, tabClient) => {

`attachTab` now returns a promise, so you could use that rather than making your own wrapper.  See bug 1227978.

@@ +262,5 @@
>        });
>      });
> +
> +    this.tabClient = tabClient;
> +    this.userAgentInput.hidden = false;

This like about `userAgentInput` is not related to server connection.  Perhaps it goes above in `init` after `connectToServer` is done?

Comment 81

[Tim Nguyen :ntim]

Comment on attachment 8706104 [details] [diff] [review]
Part 4 - Wait for all async tasks to finish before emitting on and off events

Bug 1239562 is landing a similar patch (thanks jryans!)

Comment 82

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Comment 83

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Comment 84

[Tim Nguyen :ntim]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=275f732e9644

Comment 85

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Removed the wrapper for attachTab()

Comment 86

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Fixed up commit message

Comment 87

[Tim Nguyen :ntim]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f3f0c50471f

With possible test fix (for the docshell leak): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9b2e82296d1

Comment 88

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

Comment on attachment 8714986 [details] [diff] [review]
Part 2 - UI for responsive mode

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

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ -37,5 @@
>  const INPUT_PARSER = /(\d+)[^\d]+(\d+)/;
>  
>  const SHARED_L10N = new ViewHelpers.L10N("chrome://devtools/locale/shared.properties");
>  
> -function debug(msg) {

Don't delete this, I just added it. :P

In fact, uncommenting this and the one in responsivedesign-child.js in your try run should help spot the problem.

@@ -198,5 @@
>      }
>    },
>  
>    init: Task.async(function*() {
> -    debug("INIT BEGINS");

Restore all of these.

@@ +372,5 @@
>      this.tab.linkedBrowser.messageManager.sendAsyncMessage("ResponsiveMode:Stop");
>      yield stopped;
>  
> +    yield new Promise((resolve, reject) => {
> +      this.client.close(resolve);

You should also null out this.client and this.tabClient.

@@ +1124,5 @@
>      Services.prefs.setCharPref("devtools.responsiveUI.presets", JSON.stringify(registeredPresets));
>    },
>  }
>  
> +loader.lazyGetter(ResponsiveUI.prototype, "strings", function () {

Might as well move up to the top of the file while you're at it, below the other lazy loads.

Comment 89

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

Comment on attachment 8714986 [details] [diff] [review]
Part 2 - UI for responsive mode

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

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ +371,5 @@
>      let stopped = this.waitForMessage("ResponsiveMode:Stop:Done");
>      this.tab.linkedBrowser.messageManager.sendAsyncMessage("ResponsiveMode:Stop");
>      yield stopped;
>  
> +    yield new Promise((resolve, reject) => {

I think your test is hanging because `closeRDM` now waits for `contentResize` event from the child.  However, the `Stop` message above turns off those events.  So, move this block just before the `toolClosed` call.

Comment 90

[Tim Nguyen :ntim]

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

Comment 91

[Tim Nguyen :ntim]

I think the test is hanging because closeRDM() no longer works without an argument (it used to before), probably because the manager takes too long to fetch the current RDM instance.

Let's see if this confirms my suspicions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df5863b89008

I'm not sure why it makes the devtools intermittent fail (much) more frequently though. Something is probably not properly cleaned up, but not sure what (this.client & this.tabClient are being nulled out now).

Comment 92

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment #91)
> I think the test is hanging because closeRDM() no longer works without an
> argument (it used to before), probably because the manager takes too long to
> fetch the current RDM instance.
> 
> Let's see if this confirms my suspicions:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=df5863b89008
So it wasn't that. Probably a timing issue somewhere then. I can't reproduce the failure locally unfortunately :/

Comment 93

[Tim Nguyen :ntim]

The failure screenshot tells me a *lot*. Seems like the UA is not being reset for whatever reason, the resize handles have an odd placement too.

Comment 94

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] (mostly busy until Feb. 12th) from comment #93)
> Seems like the UA is not being reset for whatever reason
This makes sense actually (I'm too tired to take a look at my own test). Something hangs on shutdown.

Comment 95

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Comment 96

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

Comment 97

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

Attachment: Wait for reloads

I believe I've found the issue.  Since the UA change triggers an auto-reload, we need to wait for this reload to finish before allowing the test to continue.

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

:ntim, if the try looks good, fold this into your part 2 before landing.

Comment 98

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #97)
> :ntim, if the try looks good, fold this into your part 2 before landing.

Sorry, I mean it would fold into part 3, since that's where the userAgentChanged event first appeared.

Comment 99

[Tim Nguyen :ntim]

Attachment: Part 2 - UI for responsive mode

Thanks jryans!

Comment 100

[Tim Nguyen :ntim]

Attachment: Part 3 - Test

I've disabled the 3 failing tests on Windows 7 debug e10s and filed bug 1252201.

Comment 101

[Tim Nguyen :ntim]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8842c694a11a

Comment 102

[Tim Nguyen :ntim]

Attachment: Part 4 - Fix browser_styleeditor_media_sidebar_links.js

Comment 103

[Tim Nguyen :ntim]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=065d3408b51b

Comment 104

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #103)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=065d3408b51b

Try seems green, but with some (hopefully) unrelated failures.

Comment 105

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/750799a0d1a9
https://hg.mozilla.org/integration/fx-team/rev/664023b70692
https://hg.mozilla.org/integration/fx-team/rev/ed627972a480

Comment 106

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

Release Note Request (optional, but appreciated)
[Why is this notable]: Allows users to control user agent to simulate a mobile device
[Suggested wording]: Custom user agents supported in Responsive Design Mode
[Links (documentation, blog post, etc)]: Nothing yet, should have MDN soon

Comment 107

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/750799a0d1a9
https://hg.mozilla.org/mozilla-central/rev/664023b70692
https://hg.mozilla.org/mozilla-central/rev/ed627972a480

Comment 108

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 (Aurora) release notes.

Comment 109

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Responsive_Design_Mode_controls

Ryan, does this cover it?

Comment 110

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

(In reply to Will Bamberg [:wbamberg] from comment #109)
> ->
> https://developer.mozilla.org/en-US/docs/Tools/
> Responsive_Design_Mode#Responsive_Design_Mode_controls
> 
> Ryan, does this cover it?

Great, looks good to me!