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)
DevTools
Responsive Design Mode
Tracking
(firefox47 fixed, relnote-firefox 47+)
RESOLVED
FIXED
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.
Comment 1•12 years ago
|
||
definitely. Thanks for filing this.
Comment 2•12 years ago
|
||
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?
Reporter | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
also, no reason we couldn't do both.
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
Maybe creating some kind of emulation tab, with the ability to emulate user agents, media queries, ...
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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
Comment 10•11 years ago
|
||
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
Assignee | ||
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
> 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•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
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
Comment 15•11 years ago
|
||
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
Updated•11 years ago
|
User Story: (updated)
Assignee | ||
Comment 16•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Blocks: emulation-devtools
Comment 18•11 years ago
|
||
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.
Whiteboard: [devedition-40]
Comment 20•10 years ago
|
||
Assigning P1 priority for some devedition-40 bugs.
Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Comment 21•10 years ago
|
||
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.
Comment 23•10 years ago
|
||
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•10 years ago
|
||
(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
Status: NEW → ASSIGNED
Updated•10 years ago
|
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
Blocks: 1156199
Component: Developer Tools: User Stories → Developer Tools: Responsive Mode
Blocks: 1173146
Blocks: 1067320
Assignee | ||
Comment 27•10 years ago
|
||
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
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8687691 -
Flags: feedback?
Assignee | ||
Updated•10 years ago
|
Attachment #8687691 -
Flags: feedback? → feedback?(jryans)
Assignee | ||
Comment 29•10 years ago
|
||
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)
Assignee | ||
Comment 31•10 years ago
|
||
(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 33•10 years ago
|
||
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)
Assignee | ||
Comment 34•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8687691 -
Attachment is obsolete: true
Attachment #8687693 -
Attachment is obsolete: true
Attachment #8699659 -
Flags: review?(poirot.alex)
Attachment #8699659 -
Flags: review?(jryans)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8699660 -
Flags: review?(jryans)
Comment 38•10 years ago
|
||
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+
Assignee | ||
Comment 41•10 years ago
|
||
(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 ?
Assignee | ||
Comment 42•10 years ago
|
||
This works fine.
Attachment #8699659 -
Attachment is obsolete: true
Attachment #8699660 -
Attachment is obsolete: true
Attachment #8701249 -
Flags: review?(jryans)
Assignee | ||
Comment 43•10 years ago
|
||
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)
Assignee | ||
Comment 44•10 years ago
|
||
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)
Assignee | ||
Comment 45•10 years ago
|
||
Attachment #8701446 -
Flags: review?(jryans)
Assignee | ||
Comment 46•10 years ago
|
||
Attachment #8701447 -
Flags: review?(jryans)
Assignee | ||
Comment 47•10 years ago
|
||
Comment on attachment 8701446 [details] [diff] [review]
Part 2 - UI for Responsive Mode
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b37215fceb5c
Attachment #8701446 -
Flags: ui-review?(hholmes)
Assignee | ||
Comment 48•10 years ago
|
||
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+
Assignee | ||
Comment 52•10 years ago
|
||
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+
Assignee | ||
Comment 54•10 years ago
|
||
Addressed comments + Made setUserAgent a sync function (it was unnecessarily async earlier)
Attachment #8701642 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8701639 -
Flags: ui-review?(hholmes)
Assignee | ||
Comment 55•10 years ago
|
||
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 56•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8701639 -
Flags: ui-review?(hholmes)
Assignee | ||
Comment 57•10 years ago
|
||
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+
Assignee | ||
Comment 58•10 years ago
|
||
Keywords: checkin-needed
Comment 59•10 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 60•10 years ago
|
||
(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)
Comment 61•10 years ago
|
||
(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)
Comment 62•10 years ago
|
||
Assignee | ||
Comment 63•10 years ago
|
||
Attachment #8702630 -
Flags: review?(vporof)
Comment 64•10 years ago
|
||
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+
![]() |
||
Comment 66•10 years ago
|
||
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
Assignee | ||
Comment 67•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 68•10 years ago
|
||
Folded parts 3 & 4.
Attachment #8701642 -
Attachment is obsolete: true
Attachment #8702630 -
Attachment is obsolete: true
Attachment #8702717 -
Flags: review+
Assignee | ||
Comment 69•10 years ago
|
||
Hopefully this should fix the 2 last bustages. I'll push to try to be sure.
Assignee | ||
Comment 70•10 years ago
|
||
Assignee | ||
Comment 71•10 years ago
|
||
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 72•10 years ago
|
||
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-
Assignee | ||
Comment 73•10 years ago
|
||
(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.
Comment 74•10 years ago
|
||
(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.
Assignee | ||
Comment 75•10 years ago
|
||
Part 1 landed on m-c: https://hg.mozilla.org/mozilla-central/rev/f28fd9bd845c
Assignee | ||
Comment 76•10 years ago
|
||
Assignee | ||
Comment 77•10 years ago
|
||
Assignee | ||
Comment 78•10 years ago
|
||
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
Assignee | ||
Comment 79•9 years ago
|
||
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+
Assignee | ||
Comment 81•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 82•9 years ago
|
||
Attachment #8702356 -
Attachment is obsolete: true
Attachment #8702717 -
Attachment is obsolete: true
Attachment #8714964 -
Flags: review?(jryans)
Assignee | ||
Comment 83•9 years ago
|
||
Attachment #8714965 -
Flags: review?(jryans)
Assignee | ||
Comment 84•9 years ago
|
||
Assignee | ||
Comment 85•9 years ago
|
||
Removed the wrapper for attachTab()
Attachment #8714964 -
Attachment is obsolete: true
Attachment #8714964 -
Flags: review?(jryans)
Attachment #8714986 -
Flags: review?(jryans)
Assignee | ||
Comment 86•9 years ago
|
||
Fixed up commit message
Attachment #8714965 -
Attachment is obsolete: true
Attachment #8714965 -
Flags: review?(jryans)
Assignee | ||
Updated•9 years ago
|
Attachment #8714988 -
Flags: review?(jryans)
Assignee | ||
Comment 87•9 years ago
|
||
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 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)
Attachment #8714988 -
Flags: review?(jryans) → review+
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.
Assignee | ||
Comment 90•9 years ago
|
||
Assignee | ||
Comment 91•9 years ago
|
||
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).
Assignee | ||
Comment 92•9 years ago
|
||
(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 :/
Assignee | ||
Comment 93•9 years ago
|
||
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.
Assignee | ||
Comment 94•9 years ago
|
||
(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.
Assignee | ||
Comment 95•9 years ago
|
||
Attachment #8714986 -
Attachment is obsolete: true
Attachment #8714988 -
Attachment is obsolete: true
Assignee | ||
Comment 96•9 years ago
|
||
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.
Assignee | ||
Comment 99•9 years ago
|
||
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)
Assignee | ||
Comment 100•9 years ago
|
||
I've disabled the 3 failing tests on Windows 7 debug e10s and filed bug 1252201.
Attachment #8724877 -
Flags: review?(jryans)
Assignee | ||
Comment 101•9 years ago
|
||
Attachment #8724875 -
Flags: review?(jryans) → review+
Attachment #8724877 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 102•9 years ago
|
||
Attachment #8724978 -
Flags: review?(jryans)
Assignee | ||
Comment 103•9 years ago
|
||
Assignee | ||
Comment 104•9 years ago
|
||
(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.
Attachment #8724978 -
Flags: review?(jryans) → review+
Comment 105•9 years ago
|
||
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
Comment 107•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/750799a0d1a9
https://hg.mozilla.org/mozilla-central/rev/664023b70692
https://hg.mozilla.org/mozilla-central/rev/ed627972a480
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment 109•9 years ago
|
||
-> https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Responsive_Design_Mode_controls
Ryan, does this cover it?
Flags: needinfo?(jryans)
(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)
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•