Closed Bug 828008 Opened 12 years ago Closed 9 years ago

Provide UI to emulate custom user agent

Categories

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

defect

Tracking

(firefox47 fixed, relnote-firefox 47+)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed
relnote-firefox --- 47+

People

(Reporter: tetsuharu, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [difficulty=hard] )

User Story

A user wishes to debug a mobile layout on desktop Firefox using Responsive Mode. The site is using a UserAgent string to determine if the device is mobile or not. The user goes to the option in Firefox' Responsive View and selects the Android Gecko option and reloads the page. Page loads with android Gecko layout.

Attachments

(4 files, 24 obsolete files)

3.06 KB, patch
ntim
: review+
ntim
: checkin+
Details | Diff | Splinter Review
10.81 KB, patch
jryans
: review+
Details | Diff | Splinter Review
3.85 KB, patch
jryans
: review+
Details | Diff | Splinter Review
2.98 KB, patch
jryans
: review+
Details | Diff | Splinter Review
If devtools can change/override Firefox's user agent strings to Firefox Mobile, It'll be useful for debugging, I seem.
definitely. Thanks for filing this.
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?
(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.
also, no reason we couldn't do both.
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.
Maybe creating some kind of emulation tab, with the ability to emulate user agents, media queries, ...
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.
this could be a feature. I'm moving this into our User Stories component to get some product management love.
Component: Developer Tools → Developer Tools: User Stories
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
(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 :(
> 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."
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.
Flags: needinfo?(rcampbell)
We could provide emulation options like Chrome or IE does.
Flags: needinfo?(rcampbell)
Summary: Need 'switch user agent strings' for debugging → Create an emulation developer tool
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.
Summary: Create an emulation developer tool → Provide option to 'switch UserAgent strings' for aid in browser emulation
User Story: (updated)
(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.
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.
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.
Depends on: 1137681
Assigning P1 priority for some devedition-40 bugs. Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
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.
Whiteboard: [devedition-40] → [devedition-40][difficulty=hard]
(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.
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).
(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.
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.
Assignee: nobody → jryans
Whiteboard: [devedition-40][difficulty=hard] → [difficulty=hard]
Clearing assignment for now, since we've moved this out of devedition-40.
Assignee: jryans → nobody
Status: ASSIGNED → NEW
Component: Developer Tools: User Stories → Developer Tools: Responsive Mode
Platform work is being done in bug 1137681.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Summary: Provide option to 'switch UserAgent strings' for aid in browser emulation → Provide UI to emulate custom user agent
Attachment #8687691 - Flags: feedback?
Attachment #8687691 - Flags: feedback? → feedback?(jryans)
Attached patch Part 2: UI in network monitor (obsolete) — Splinter Review
This is just a test UI for the devtools actor, nothing really final here.
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.
Attachment #8687691 - Flags: feedback?(jryans) → feedback?(poirot.alex)
(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.
(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 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.
Attachment #8687691 - Flags: feedback?(poirot.alex)
(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.
(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.
Attached patch Part 1 - TabActor changes (obsolete) — Splinter Review
Attachment #8687691 - Attachment is obsolete: true
Attachment #8687693 - Attachment is obsolete: true
Attachment #8699659 - Flags: review?(poirot.alex)
Attachment #8699659 - Flags: review?(jryans)
Attached patch Part 2 - UI for responsive mode (obsolete) — Splinter Review
Attachment #8699660 - Flags: review?(jryans)
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.
Attachment #8699659 - Flags: review?(poirot.alex) → review+
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.
Attachment #8699659 - Flags: review?(jryans) → review-
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).
Attachment #8699660 - Flags: review?(jryans) → feedback+
(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 ?
Attached patch Part 1 - TabActor changes (obsolete) — Splinter Review
This works fine.
Attachment #8699659 - Attachment is obsolete: true
Attachment #8699660 - Attachment is obsolete: true
Attachment #8701249 - Flags: review?(jryans)
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.
Attachment #8701255 - Flags: ui-review?(hholmes)
Attachment #8701255 - Flags: review?(jryans)
Attached patch Part 1 - TabActor changes (obsolete) — Splinter Review
Attachment #8701249 - Attachment is obsolete: true
Attachment #8701255 - Attachment is obsolete: true
Attachment #8701249 - Flags: review?(jryans)
Attachment #8701255 - Flags: ui-review?(hholmes)
Attachment #8701255 - Flags: review?(jryans)
Attachment #8701445 - Flags: review?(jryans)
Attached patch Part 2 - UI for Responsive Mode (obsolete) — Splinter Review
Attachment #8701446 - Flags: review?(jryans)
Attached patch Part 3 - Test (obsolete) — Splinter Review
Attachment #8701447 - Flags: review?(jryans)
Attachment #8701446 - Flags: ui-review?(hholmes)
Attached patch Part 3 - Test (obsolete) — Splinter Review
Removed an unrelated change
Attachment #8701447 - Attachment is obsolete: true
Attachment #8701447 - Flags: review?(jryans)
Attachment #8701448 - Flags: review?(jryans)
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 `!==`.
Attachment #8701445 - Flags: review?(jryans) → review+
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
Attachment #8701446 - Flags: review?(jryans) → review+
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.
Attachment #8701448 - Flags: review?(jryans) → review+
Addressed comments.
Attachment #8701445 - Attachment is obsolete: true
Attachment #8701446 - Attachment is obsolete: true
Attachment #8701448 - Attachment is obsolete: true
Attachment #8701446 - Flags: ui-review?(hholmes)
Attachment #8701638 - Flags: review+
Attached patch Part 2 - UI for responsive mode (obsolete) — Splinter Review
Addressed comments.
Attachment #8701639 - Flags: review+
Attached patch Part 3 - Test (obsolete) — Splinter Review
Addressed comments + Made setUserAgent a sync function (it was unnecessarily async earlier)
Attachment #8701642 - Flags: review+
Attachment #8701639 - Flags: ui-review?(hholmes)
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/ntim.bugs@gmail.com-b37215fceb5cb6fc9332b3973e5ca17f82e71679/
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.
Attachment #8701639 - Flags: review+ → review?(ntim.bugs)
Attachment #8701639 - Flags: ui-review?(hholmes)
Attached patch Part 2 - UI for responsive mode (obsolete) — Splinter Review
Changed placeholder to "Custom User Agent" The rotation lag seems like a separate issue to me.
Attachment #8701639 - Attachment is obsolete: true
Attachment #8701639 - Flags: review?(ntim.bugs)
Attachment #8702356 - Flags: review+
(In reply to Pulsebot from comment #59) > https://hg.mozilla.org/integration/fx-team/rev/f28fd9bd845c All 3 patches should have been checked in.
Flags: needinfo?(cbook)
(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!
Flags: needinfo?(cbook)
Attachment #8702630 - Flags: review?(vporof)
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
Attachment #8702630 - Flags: review?(vporof) → review+
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 on attachment 8701638 [details] [diff] [review] Part 1 - TabActor changes This part wasn't backed out (It's not a problem though).
Attachment #8701638 - Flags: checkin+
Keywords: leave-open
Attached patch Part 3 - Test (obsolete) — Splinter Review
Folded parts 3 & 4.
Attachment #8701642 - Attachment is obsolete: true
Attachment #8702630 - Attachment is obsolete: true
Attachment #8702717 - Flags: review+
Hopefully this should fix the 2 last bustages. I'll push to try to be sure.
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.
Attachment #8702721 - Flags: review?(vporof)
Attachment #8702721 - Flags: review?(poirot.alex)
Attachment #8702721 - Flags: review?(paul)
Attachment #8702721 - Flags: review?(lclark)
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/ntim.bugs@gmail.com-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?
Attachment #8702721 - Flags: review?(vporof)
Attachment #8702721 - Flags: review?(poirot.alex)
Attachment #8702721 - Flags: review?(paul)
Attachment #8702721 - Flags: review?(lclark)
Attachment #8702721 - Flags: review-
(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/ntim.bugs@gmail.com- > 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.
(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.
Depends on: 1235953
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.
Attachment #8702721 - Attachment is obsolete: true
Attachment #8706104 - Flags: review?(jryans)
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?
Attachment #8706104 - Flags: review?(jryans) → feedback+
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!)
Attachment #8706104 - Attachment is obsolete: true
Keywords: leave-open
Attached patch Part 2 - UI for responsive mode (obsolete) — Splinter Review
Attachment #8702356 - Attachment is obsolete: true
Attachment #8702717 - Attachment is obsolete: true
Attachment #8714964 - Flags: review?(jryans)
Attached patch Part 3 - Test (obsolete) — Splinter Review
Attachment #8714965 - Flags: review?(jryans)
Attached patch Part 2 - UI for responsive mode (obsolete) — Splinter Review
Removed the wrapper for attachTab()
Attachment #8714964 - Attachment is obsolete: true
Attachment #8714964 - Flags: review?(jryans)
Attachment #8714986 - Flags: review?(jryans)
Attached patch Part 3 - Test (obsolete) — Splinter Review
Fixed up commit message
Attachment #8714965 - Attachment is obsolete: true
Attachment #8714965 - Flags: review?(jryans)
Attachment #8714988 - Flags: review?(jryans)
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.
Attachment #8714986 - Flags: review?(jryans)
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.
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).
(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 :/
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.
(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.
Attached patch Part 2 - UI for responsive mode (obsolete) — Splinter Review
Attachment #8714986 - Attachment is obsolete: true
Attachment #8714988 - Attachment is obsolete: true
Attached patch Part 3 - Test (obsolete) — Splinter Review
Attached patch Wait for reloads (obsolete) — Splinter Review
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.
Flags: needinfo?(ntim.bugs)
(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.
Thanks jryans!
Attachment #8723763 - Attachment is obsolete: true
Attachment #8723764 - Attachment is obsolete: true
Attachment #8723909 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8724875 - Flags: review?(jryans)
Attached patch Part 3 - TestSplinter Review
I've disabled the 3 failing tests on Windows 7 debug e10s and filed bug 1252201.
Attachment #8724877 - Flags: review?(jryans)
(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.
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
relnote-firefox: --- → ?
Keywords: dev-doc-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(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!
Flags: needinfo?(jryans)
Depends on: 1256314, 1256315
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: