Closed
Bug 1254386
Opened 8 years ago
Closed 8 years ago
Apply UA from selected device
Categories
(DevTools :: Responsive Design Mode, defect, P1)
DevTools
Responsive Design Mode
Tracking
(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 verified)
People
(Reporter: jryans, Assigned: ntim)
References
(Blocks 1 open bug)
Details
(Whiteboard: [multiviewport] [mvp-rdm])
Attachments
(4 files, 6 obsolete files)
6.49 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
4.14 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
5.00 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
4.30 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
Bug 1241714 adds a selected device. We need to apply the user agent when it's selected.
Reporter | ||
Updated•8 years ago
|
Flags: qe-verify+
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [multiviewport][triage] → [multiviewport] [mvp-rdm]
Updated•8 years ago
|
QA Contact: mihai.boldan
Reporter | ||
Comment 1•8 years ago
|
||
To do this, we'll need to create a DevTools RDP client in the RDM tool and use it apply the UA setting. We already did something similar for UA in the old RDM tool[1]. Once you have the RDP client, you can use it to set the new UA[2]. So, for the new RDM, we would pass the desired UA to this RDP client when a device is chosen. In the new RDM, I believe the RDP client would be part of the manager.js module outside the page. So we'd want to communicate with that when the device changes. That's a high level overview, feel free to needinfo? about details as needed! [1]: https://dxr.mozilla.org/mozilla-central/rev/5511d54a3f172c1d68f98cc55dce4de1d0ba1b51/devtools/client/responsivedesign/responsivedesign.jsm#266-279 [2]: https://dxr.mozilla.org/mozilla-central/rev/5511d54a3f172c1d68f98cc55dce4de1d0ba1b51/devtools/client/responsivedesign/responsivedesign.jsm#975
Updated•8 years ago
|
Assignee: nobody → jaideepb
Reporter | ||
Comment 2•8 years ago
|
||
In my notes from bug 1269882, we agreed that: "For user agent changes, we would not reload automatically, as developers should understand that a reload is needed to fully apply a UA change." So for this bug, we don't want to reload after applying the UA.
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1) > To do this, we'll need to create a DevTools RDP client in the RDM tool and > use it apply the UA setting. > > In the new RDM, I believe the RDP client would be part of the manager.js > module outside the page. So we'd want to communicate with that when the > device changes. This approach won't work until bug 1240907 is fixed. The UA will end up changing only for the top-level RDM window. The content iframe won't be affected since it has the `mozbrowser` and `isolate` attributes, which make the iframe fully independent from the RDM window. You can open this testcase: data:text/html,<button onclick="this.nextSibling.textContent = navigator.userAgent">write down UA</button><div></div> then paste this code inside the console: var Ci = Components.interfaces; var ds = window.QueryInterface(Ci.nsIInterfaceRequestor) .getInterface(Ci.nsIWebNavigation) .QueryInterface(Ci.nsIDocShell); ds.customUserAgent = "foo"; You'll see that navigator.userAgent changes for the top-level RDM window, but if you click the webpage's button, the UA is not being changed (normally it should change live without any reload).
Flags: needinfo?(jryans)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Tim Nguyen :ntim (not accepting requests) from comment #3) > The UA will end up changing only for the top-level RDM window. The content iframe won't be > affected since it has the `mozbrowser` and `isolate` attributes, which make > the iframe fully independent from the RDM window. To clarify, the mozbrowser iframe docshell isn't a child docshell (because mozbrowser iframes are special iframes) for the RDM window's docshell, which will cause the custom UA not to be passed on. This is the code that passes on the flag to the child docshells: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3153 Either way, we don't want the UA to be changed for the RDM window, so we'll need bug 1240907 to be fixed first anyway. > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1) > > > To do this, we'll need to create a DevTools RDP client in the RDM tool and > > use it apply the UA setting. > > > > In the new RDM, I believe the RDP client would be part of the manager.js > > module outside the page. So we'd want to communicate with that when the > > device changes. > > This approach won't work until bug 1240907 is fixed. It would work if we would target the DebuggerServer to the mozbrowser iframe, but that's basically bug 1240907.
Assignee | ||
Comment 5•8 years ago
|
||
Of course, it's still possible to go forward with the suggested approach, and fix bug 1240907 later (I suppose it won't require much client side changes, but more server side changes), but I find it a bit odd to proceed backwards.
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #3) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1) > > > To do this, we'll need to create a DevTools RDP client in the RDM tool and > > use it apply the UA setting. > > > > In the new RDM, I believe the RDP client would be part of the manager.js > > module outside the page. So we'd want to communicate with that when the > > device changes. > > This approach won't work until bug 1240907 is fixed. The UA will end up > changing only for the top-level RDM window. The content iframe won't be > affected since it has the `mozbrowser` and `isolate` attributes, which make > the iframe fully independent from the RDM window. > > You can open this testcase: > data:text/html,<button onclick="this.nextSibling.textContent = > navigator.userAgent">write down UA</button><div></div> > > then paste this code inside the console: > var Ci = Components.interfaces; > var ds = window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIWebNavigation) > .QueryInterface(Ci.nsIDocShell); > ds.customUserAgent = "foo"; > > You'll see that navigator.userAgent changes for the top-level RDM window, > but if you click the webpage's button, the UA is not being changed (normally > it should change live without any reload). Thanks for the detailed comments :ntim! Jaideep and I also noticed this issue about a day before your comments were posted when he shared a patch over IRC. I told him the same thing, that it would likely make sense to wait for bug 1240907 first so it's easier to complete this one and be confident that it is correct.
Flags: needinfo?(jryans)
Comment 7•8 years ago
|
||
This is a WIP patch of the work I did before when this bug was still blocked.
Updated•8 years ago
|
Assignee: jaideepb → nobody
Status: ASSIGNED → NEW
Reporter | ||
Comment 8•8 years ago
|
||
Let's wait for the emulation actor Benoit is adding in bug 1285566.
Depends on: 1285566
Reporter | ||
Updated•8 years ago
|
Blocks: 918098, multiple-viewports-v1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Updated•8 years ago
|
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8776345 -
Attachment is obsolete: true
Attachment #8782038 -
Flags: review?(jryans)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8782039 -
Flags: review?(jryans)
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8782040 -
Flags: review?(jryans)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8782124 -
Flags: review?(jryans)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8782038 [details] [diff] [review] Move UA emulation to emulation actor Review of attachment 8782038 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just some naming things to resolve. Thanks for working on it! ::: devtools/server/actors/emulation.js @@ +50,5 @@ > }, > > + /* User agent override */ > + > + _previousCustomUserAgent: null, Let's follow the naming pattern from touch events in the actor, so `_previousUserAgentOverride`, etc. @@ +52,5 @@ > + /* User agent override */ > + > + _previousCustomUserAgent: null, > + > + setCustomUserAgent(userAgent) { `setUserAgentOverride` @@ +63,5 @@ > + this.docShell.customUserAgent = userAgent; > + return true; > + }, > + > + getCustomUserAgent() { `getUserAgentOverride` @@ +67,5 @@ > + getCustomUserAgent() { > + return this.docShell.customUserAgent; > + }, > + > + restoreUserAgent() { `clearUserAgentOverride` @@ +69,5 @@ > + }, > + > + restoreUserAgent() { > + if (this._previousCustomUserAgent !== null) { > + return this.setCustomUserAgent(this._previousCustomUserAgent); Update `clearTouchEventsOverride` to also return like this as well. ::: devtools/shared/specs/emulation.js @@ +48,5 @@ > + > + restoreUserAgent: { > + request: {}, > + response: { > + valueChanged: RetVal("boolean") `valueChanged` seems like a better name here, please change the touch events `reload` to `valueChanged` in this file.
Attachment #8782038 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 15•8 years ago
|
||
Comment on attachment 8782040 [details] [diff] [review] Test for device list Review of attachment 8782040 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/responsive.html/test/browser/browser.ini @@ +14,5 @@ > !/devtools/client/inspector/test/shared-head.js > !/devtools/client/shared/test/test-actor.js > !/devtools/client/shared/test/test-actor-registry.js > > +[browser_device_change.js] You forgot to add the test to the patch. ;)
Attachment #8782040 -
Flags: review?(jryans) → review-
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 8782039 [details] [diff] [review] Add user agent emulation to new RDM Review of attachment 8782039 [details] [diff] [review]: ----------------------------------------------------------------- Seems close, but there's a small regression left to fix. ::: devtools/client/responsive.html/components/device-selector.js @@ +35,5 @@ > if (target.value === OPEN_DEVICE_MODAL_VALUE) { > onUpdateDeviceModalOpen(true); > return; > } > + let selected; It seems like something here is causing a regression: STR: 1. Choose a device 2. Use the corner resize gripper to move to some random size ER: Should go back to "no device selected" AR: Device name still in place, though the device UA is removed. ::: devtools/client/responsive.html/manager.js @@ +404,4 @@ > } > }, > > + waitForReload() { Remove this, looks unused. @@ +429,5 @@ > this.emulationFront.clearTouchEventsOverride(); > } > }), > > + updateUserAgent: Task.async(function* (userAgent) { Seems like you could remove Task.async here since yielding isn't needed for control flow currently. @@ +432,5 @@ > > + updateUserAgent: Task.async(function* (userAgent) { > + if (userAgent) { > + yield this.emulationFront.setCustomUserAgent( > + userAgent Nit: Seems like this would fit on one line.
Attachment #8782039 -
Flags: review?(jryans) → review-
Reporter | ||
Comment 17•8 years ago
|
||
Comment on attachment 8782124 [details] [diff] [review] Make old RDM UA emulation use emulation actor Review of attachment 8782124 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating it, seems to work well! ::: devtools/client/responsivedesign/responsivedesign.jsm @@ +274,5 @@ > } > this.client = new DebuggerClient(DebuggerServer.connectPipe()); > yield this.client.connect(); > let {tab} = yield this.client.getTab(); > + yield this.client.attachTab(tab.actor); Is `attachTab` actually needed still? @@ +971,5 @@ > * Change the user agent string > */ > changeUA: Task.async(function* () { > let value = this.userAgentInput.value; > + let valueChanged; Maybe just `changed` for brevity? @@ +976,3 @@ > if (value) { > + valueChanged = yield this.emulationFront.setCustomUserAgent( > + value Probably fits on one line? Unsure... 90 chars is our current length.
Attachment #8782124 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > Comment on attachment 8782039 [details] [diff] [review] > Add user agent emulation to new RDM > > Review of attachment 8782039 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems close, but there's a small regression left to fix. > > ::: devtools/client/responsive.html/components/device-selector.js > @@ +35,5 @@ > > if (target.value === OPEN_DEVICE_MODAL_VALUE) { > > onUpdateDeviceModalOpen(true); > > return; > > } > > + let selected; > > It seems like something here is causing a regression: > > STR: > > 1. Choose a device > 2. Use the corner resize gripper to move to some random size > > ER: > > Should go back to "no device selected" > > AR: > > Device name still in place, though the device UA is removed. > Looks like a leftover from Jaideep's patch, I'll take a look if I can remove this.
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17) > Comment on attachment 8782124 [details] [diff] [review] > Make old RDM UA emulation use emulation actor > > Review of attachment 8782124 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for updating it, seems to work well! > > ::: devtools/client/responsivedesign/responsivedesign.jsm > @@ +274,5 @@ > > } > > this.client = new DebuggerClient(DebuggerServer.connectPipe()); > > yield this.client.connect(); > > let {tab} = yield this.client.getTab(); > > + yield this.client.attachTab(tab.actor); > > Is `attachTab` actually needed still? Yes, otherwise waitForReload() doesn't work (as it relies on TabActor events).
Assignee | ||
Comment 20•8 years ago
|
||
MozReview-Commit-ID: KhxfaGnuuUd
Assignee | ||
Updated•8 years ago
|
Attachment #8782040 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
MozReview-Commit-ID: 9005ktskEVk
Attachment #8783263 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8782038 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8782039 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8782124 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8783262 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
MozReview-Commit-ID: EQawVubnMAp
Attachment #8783264 -
Flags: review?(jryans)
Assignee | ||
Comment 23•8 years ago
|
||
MozReview-Commit-ID: K56yifj9xfI
Attachment #8783265 -
Flags: review?(jryans)
Assignee | ||
Comment 24•8 years ago
|
||
MozReview-Commit-ID: KhxfaGnuuUd
Attachment #8783266 -
Flags: review?(jryans)
Assignee | ||
Comment 25•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa772214c90f
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 8783263 [details] [diff] [review] Move custom UA emulation to emulation actor Review of attachment 8783263 [details] [diff] [review]: ----------------------------------------------------------------- Great, looks good to me! Thanks for adjusting the names to fit my desire for parallel naming. :)
Attachment #8783263 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 27•8 years ago
|
||
Comment on attachment 8783264 [details] [diff] [review] Change old RDM to use emulation actor Review of attachment 8783264 [details] [diff] [review]: ----------------------------------------------------------------- Great, looks good to me, thanks for working on it! ::: devtools/client/responsivedesign/responsivedesign.jsm @@ +984,1 @@ > let reloaded = this.waitForReload(); Move this inside the `if (changed)` block
Attachment #8783264 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 28•8 years ago
|
||
Comment on attachment 8783266 [details] [diff] [review] Test for device list Review of attachment 8783266 [details] [diff] [review]: ----------------------------------------------------------------- Would be great to add the enhancement I mentioned, but looks good to me! ::: devtools/client/responsive.html/test/browser/browser_device_change.js @@ +23,5 @@ > + > + // Test device where UA field is blank > + switchDevice(ui, "Laptop (1366 x 768)"); > + yield waitForViewportResizeTo(ui, 1366, 768); > + yield testUserAgent(ui, DEFAULT_UA); It would be great to also check for the regression I noticed during the previous review in this test. So, something like choose a device, nudge the resize gripper (borrow helpers from browser_mouse_resize.js), verify the device selector label says "no device selected".
Attachment #8783266 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 29•8 years ago
|
||
Comment on attachment 8783265 [details] [diff] [review] Add user agent emulation to new RDM Review of attachment 8783265 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks for working on this! ::: devtools/client/responsive.html/components/resizable-viewport.js @@ +106,5 @@ > > // Update the viewport store with the new width and height. > this.props.onResizeViewport(width, height); > // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice({name: ""}); Nit: use `{ name: "" }` style (wish we could lint one way or the other, but I've tried to be consistent in new RDM at least...) ::: devtools/client/responsive.html/components/viewport-dimension.js @@ +113,5 @@ > return; > } > > // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice({name: ""}); Nit: use `{ name: "" }` style (wish we could lint one way or the other, but I've tried to be consistent in new RDM at least...)
Attachment #8783265 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 30•8 years ago
|
||
Also, feel free to file a follow up to discuss UX options with Helen to make it more obvious that a UA is applied.
Comment 31•8 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/fec37519e65f Move custom UA emulation to emulation actor. r=jryans https://hg.mozilla.org/integration/fx-team/rev/6f5b69e1d1a9 Change old RDM to use emulation actor. r=jryans https://hg.mozilla.org/integration/fx-team/rev/bba8077c6c0a Add user agent emulation to new RDM. r=jryans https://hg.mozilla.org/integration/fx-team/rev/0faec8f3a8b5 Test for device list. r=jryans
Comment 32•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fec37519e65f https://hg.mozilla.org/mozilla-central/rev/6f5b69e1d1a9 https://hg.mozilla.org/mozilla-central/rev/bba8077c6c0a https://hg.mozilla.org/mozilla-central/rev/0faec8f3a8b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
status-firefox49:
--- → wontfix
status-firefox50:
--- → wontfix
Reporter | ||
Comment 33•8 years ago
|
||
For testing this: 1. Start RDM 2. Choose a device * A new user agent is now applied 3. Open the DevTools console, and check "navigtor.userAgent" * It should have a new value that depends on the device 4. Reload the page with network monitor open * For each request, the User-Agent request header should match the new device value as well
Comment 35•8 years ago
|
||
I managed to reproduce this issue on Firefox 50.0a1 (2016-07-27), under Windows 10x64, using the STR from Comment 33. I've performed the same STR on Firefox 51.0a1 (2016-09-12) across platforms [1] and for a part of the devices, the User Agent matches the new device value, but for the other part of the devices, the default Firefox user agent is displayed. For eg. Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 user agent is displayed for the devices displayed below: - Laptop (1366x768) - Laptop (1920x1080) - Laptop (1920x1080) with touch - LG watch - LG watch R - Samsung Gear Live - 1080 Full HD Television - 4k Ultra HD Television - 720p HD Televisoin The same result is also displayed on Mac OS X and on Ubuntu. Is this an expected behavior? [1]Windows 10x64, Mac OS X 10.11.1, Ubuntu 16.04x64
QA Whiteboard: [qe-rdm]
Flags: needinfo?(jryans)
Assignee | ||
Comment 36•8 years ago
|
||
(In reply to Mihai Boldan, QA [:mboldan] from comment #35) > I managed to reproduce this issue on Firefox 50.0a1 (2016-07-27), under > Windows 10x64, using the STR from Comment 33. > > I've performed the same STR on Firefox 51.0a1 (2016-09-12) across platforms > [1] and for a part of the devices, the User Agent matches the new device > value, but for the other part of the devices, the default Firefox user agent > is displayed. For eg. Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) > Gecko/20100101 Firefox/51.0 user agent is displayed for the devices > displayed below: > - Laptop (1366x768) > - Laptop (1920x1080) > - Laptop (1920x1080) with touch That's expected, those devices have generic names. > - LG watch > - LG watch R > - Samsung Gear Live That sounds like a bug with the device definitions. Can you file an issue here: https://github.com/mozilla/simulated-devices/ ? > - 1080 Full HD Television > - 4k Ultra HD Television > - 720p HD Televisoin Seems expected as well, as the device definition considers them as Firefox OS devices.
Comment 37•8 years ago
|
||
I've logged Bug 1302964 and https://github.com/mozilla/simulated-devices/issues/16 based on Comment 36. Since there are no other found issues, I am marking this bug Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(jryans)
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
No longer blocks: multiple-viewports-v1
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•