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

- dPR and media queries are now reflecting the selected device's dppx - added unit test Review commit: https://reviewboard.mozilla.org/r/61262/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61262/

I'd like to see how the discussion in the platform bug evolves before diving into this review.

Comment on attachment 8766284 [details] Bug 1254385 - Apply dPR from selected device; https://reviewboard.mozilla.org/r/61262/#review58624 ::: devtools/client/responsive.html/actions/viewports.js:15 (Diff revision 1) > + CHANGE_DPPX, > RESIZE_VIEWPORT, > ROTATE_VIEWPORT > } = require("./index"); > > +const e10s = require("../utils/e10s"); This seems unused. ::: devtools/client/responsive.html/components/browser.js:84 (Diff revision 1) > + > + if (dppx > 0 && this.refs.browserContainer) { > + let browser = this.refs.browserContainer.querySelector("iframe.browser"); > + let mm = browser.frameLoader.messageManager; > + > + e10s.emit(mm, "OverrideDPPX", dppx); I think rather than setting this by talking to the frame script directly, we instead want to do it through the DevTools protocol. This gives us the advantage of being prepared to control remote things in the future. For exmaple, Chrome's tools apply these settings through their protocol[1]. User agent already works this way (in the old RDM) and I'd like to continue along this path if we can. It does mean you'd be blocked on having a way to connect to the content (bug 1240907) and you also need access to a DevTools client (probably living in the manager). [1]: https://chromedevtools.github.io/debugger-protocol-viewer/tot/Emulation/

I should take another look at the call in the render() method here.

https://reviewboard.mozilla.org/r/61262/#review59420 ::: devtools/client/responsive.html/components/browser.js:132 (Diff revision 1) > } = this.props; > > // innerHTML expects & to be an HTML entity > location = location.replace(/&/g, "&"); > > + this.overrideDPPX(); I don't think this is the approach we want to use here. The main reason is we really don't want this specific component's `render()` method to re-run. Because other constraints here are forcing us to use `dangerouslySetInnerHTML` below, it means that re-running `render()` in this component recreates the iframe, destroying the state on the page. (This is not an issue for "normal" React components without `dangerouslySetInnerHTML` since they diff elements as you would expect). I think a better approach would be to move the "side effect" of setting the new DPPX into the action creator (sort of like was done with screenshots, though it probably doesn't need START and END states). We should probably update touch simulation to take the same approach I think (move it's `postMessage` out of app.js and into the action creator). IIRC there was an issue with XPCShell tests if you use `postMessage` from the action creator (since there is no window), but we could probably test for `if (window)` first or something.

Comment on attachment 8766284 [details] Bug 1254385 - Apply dPR from selected device; https://reviewboard.mozilla.org/r/61262/#review72178 Thanks for working on this! The only major thing is that we should push the navigation tracking down to the actor (more details in the line comments). This should reduce complexity by removing the work from the RDM UI and makes it easier to support non-local viewports in the future. Hopefully it will make the patch simpler as well! ::: devtools/client/responsive.html/manager.js:434 (Diff revision 2) > return; > } > > switch (event.data.type) { > + case "browser-loadstart": > + this.updateDPPX(this._cachedDPPX); I do think we should file a follow up to explore changing the DPPX override to persist across navigation (like the other overrides), but I think it's okay to have to set it manually on navigation for the moment. However, we should push this detail down into the emulation actor instead of handling it at the RDM layer. The emulation actor is passed the tab actor for the content it represents. The tab actor emits `will-navigate`[1] and `navigate`[2] events inside the server that other actors can listen to. You should be able to use one of these (not sure which is the right time for what you need to do) to let the actor reapply the DPPX override when necessary. [1]: https://dxr.mozilla.org/mozilla-central/rev/7963ebdd52b93f96b812eff2eab8d94097147b9c/devtools/server/actors/webbrowser.js#1933 [2]: https://dxr.mozilla.org/mozilla-central/rev/7963ebdd52b93f96b812eff2eab8d94097147b9c/devtools/server/actors/webbrowser.js#1984 ::: devtools/client/responsive.html/test/browser/browser_device_change.js:31 (Diff revision 2) > + "os": "custom", > + "featured": true, > +}; > + > +// Add the new device to the list > +addRDMTask(TEST_URL, function* ({ ui, manager }) { Is there a reason you want a separate `addRDMTask` block (which opens the tab, opens RDM, ..., closes RDM, closes tab) for this? ::: devtools/client/responsive.html/test/browser/browser_device_change.js:98 (Diff revision 2) > + return yield ContentTask.spawn(ui.getViewportBrowser(), {}, function* () { > + return content.devicePixelRatio; > + }); > +} > + > +function whenDevicePixelRatioChange(ui) { Let's call this `waitFor...` instead of `when...` to match other similar helpers. ::: devtools/shared/specs/emulation.js:60 (Diff revision 2) > } > }, > + > + setDPPXOverride: { > + request: { > + flag: Arg(0, "string") Isn't this a number? And `dppx` for the name?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > I do think we should file a follow up to explore changing the DPPX override > to persist across navigation (like the other overrides), but I think it's > okay to have to set it manually on navigation for the moment. I'm investigating on one thing on platform side about that, if it's not quick – e.g. couple of hours – I'll try to use navigate / will-navigate from the actor as said, and file a follow up bug. > > +// Add the new device to the list > > +addRDMTask(TEST_URL, function* ({ ui, manager }) { > > Is there a reason you want a separate `addRDMTask` block (which opens the > tab, opens RDM, ..., closes RDM, closes tab) for this? I think we have already in some other tests, I just copied from it. Basically `AddDevice` is called from a previous tests, probably because RDM needs to be closed to get updated. I did the same here; it separates the adding device tasks from the real test. I think I could just close the RDM in the same test and then reopen, but it doesn't seems cleaner to me. If you have any suggestion, let me know! > > +function whenDevicePixelRatioChange(ui) { > > Let's call this `waitFor...` instead of `when...` to match other similar > helpers. I used a different naming exactly because it's not similar to the other helpers. The other helpers "waits" – we use directly the helpers with yield – where in that case we want to setup some listeners and then use the value "when". The name is on purpose to make this distinction, calling in the same way could lead to the same usage – and it could not work as expected. > > + setDPPXOverride: { > > + request: { > > + flag: Arg(0, "string") > > Isn't this a number? And `dppx` for the name? My bad, thanks to spot this out! It was a leftover.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #8) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > > > I do think we should file a follow up to explore changing the DPPX override > > to persist across navigation (like the other overrides), but I think it's > > okay to have to set it manually on navigation for the moment. > > I'm investigating on one thing on platform side about that, if it's not > quick – e.g. couple of hours – I'll try to use navigate / will-navigate from > the actor as said, and file a follow up bug. As discussed on IRC, if there is a quick fix, then let's try to make it happen. Either in the bug or platform. If it looks like a larger change that will block this work, then a separate bug would be better. > > > +// Add the new device to the list > > > +addRDMTask(TEST_URL, function* ({ ui, manager }) { > > > > Is there a reason you want a separate `addRDMTask` block (which opens the > > tab, opens RDM, ..., closes RDM, closes tab) for this? > > I think we have already in some other tests, I just copied from it. > Basically `AddDevice` is called from a previous tests, probably because RDM > needs to be closed to get updated. I did the same here; it separates the > adding device tasks from the real test. I think I could just close the RDM > in the same test and then reopen, but it doesn't seems cleaner to me. If you > have any suggestion, let me know! Ah, I see, didn't realize it appears in other tests too. My guess is we don't _have_ to close but could instead use `waitUntilState` or similar to wait until the new device has been applied. I am also okay leaving it as is, I don't feel strongly. > > > +function whenDevicePixelRatioChange(ui) { > > > > Let's call this `waitFor...` instead of `when...` to match other similar > > helpers. > > I used a different naming exactly because it's not similar to the other > helpers. The other helpers "waits" – we use directly the helpers with yield > – where in that case we want to setup some listeners and then use the value > "when". The name is on purpose to make this distinction, calling in the same > way could lead to the same usage – and it could not work as expected. Hmm, but that's all based on whether the caller uses `yield` right away or not, correct? I guess in your case you are also getting the changed value, not just waiting for an event and proceeding... Okay, I am fine either way. :)

Sorry for the mess, I thought I could actually push two separate commit and be considered as two separate review (since also the reviewer was different), but MozReview seems not working in that way. Also, I didn't want to commit yet the JS part. I'm going to file a separate bug for platform part, it's probably faster – but I would also ask in #mozreview how to deal with multi commit things.

ryan, since you didn't have strong feeling for the `AddDevice` and the `whenPixelRatioChange`, I kept those two things; just fixes the rest (I can also try to explain better on IRC if you want the conceptual difference between our prefix "wait" for other utility function).

Comment on attachment 8766284 [details] Bug 1254385 - Apply dPR from selected device; https://reviewboard.mozilla.org/r/61262/#review73282 Great, this version looks good to me! http://mediaqueriestest.com was a helpful test page to verify it was working as expected.

Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b3a981bc077&selectedJob=26654755 The oranges doesn't seem related to this patch.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #17) > The oranges doesn't seem related to this patch. This one sure does: TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_device_modal_submit.js | Got expected number of devices in device selector. - Got 17, expected 18

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18) > (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #17) > > The oranges doesn't seem related to this patch. > > This one sure does: > TEST-UNEXPECTED-FAIL | > devtools/client/responsive.html/test/browser/browser_device_modal_submit.js > | Got expected number of devices in device selector. - Got 17, expected 18 I thought it was an intermittent – this test has a lot of issues –; but apparently is not. I will check, thank you!

Looks like `AddDevice` just adds to a temporary local set, but then it's probably not cleared between test runs. I would think adding a `ClearLocalDevices` or something to reset this that is called in a test cleanup function should do it...?

We could as well replace the nokia device with this fake phone in the test directory devices.json

I decided to add a `RemoveDevice` method, to pair with `AddDevice`: since it seems from the comment that such method could also be used from an add-on, we should have a method to remove the devices added once the add-on is disabled. It looks to me a bit more safer. By the way, I was wondering why the methods in this module are capitalized; they're not following our standard naming convention aren't they? Or I'm missed something?

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #22) > I decided to add a `RemoveDevice` method, to pair with `AddDevice`: since it > seems from the comment that such method could also be used from an add-on, > we should have a method to remove the devices added once the add-on is > disabled. It looks to me a bit more safer. Thanks, that sounds reasonable! > By the way, I was wondering why the methods in this module are capitalized; > they're not following our standard naming convention aren't they? Or I'm > missed something? There's no reason that I know of... it bothers me too, feel free to change the casing!

Comment on attachment 8766284 [details] Bug 1254385 - Apply dPR from selected device; https://reviewboard.mozilla.org/r/61262/#review75046 ::: devtools/client/responsive.html/test/browser/browser_device_change.js:58 (Diff revision 6) > - yield waitForViewportResizeTo(ui, 320, 533); > - yield testUserAgent(ui, NOKIA_UA); > + yield waitForViewportResizeTo(ui, addedDevice.width, addedDevice.height); > + yield testUserAgent(ui, addedDevice.userAgent); > + > + // Test device with custom pixelRatio > + testDevicePixelRatio(yield waitingPixelRatio, addedDevice.pixelRatio); > + waitingPixelRatio = whenDevicePixelRatioChange(ui); How do you feel about using onceDevicePixelRatioChanged ? It's more consistent with our naming.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23) > > By the way, I was wondering why the methods in this module are capitalized; > > they're not following our standard naming convention aren't they? Or I'm > > missed something? > > There's no reason that I know of... it bothers me too, feel free to change > the casing! Will do! (In reply to Tim Nguyen :ntim (use needinfo?) from comment #24) > How do you feel about using onceDevicePixelRatioChanged ? It's more > consistent with our naming. Sounds better! Thanks Tim!

here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=001d868bab61&selectedJob=27171935 I fixed in the new push on mozreview the ES lint errors. The other oranges don't seem to be related – I was afraid about | devtools/client/webide/test/test_simulators.html, but they're intermittent that happens also locally before this patch.

Comment on attachment 8766284 [details] Bug 1254385 - Apply dPR from selected device; https://reviewboard.mozilla.org/r/61262/#review76446 ::: devtools/client/shared/test/browser_devices.js:4 (Diff revision 7) > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > > -var { GetDevices, GetDeviceString, AddDevice } = require("devtools/client/shared/devices"); > +const { nit: trailing space (this directory isn't linted unfortunately)

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/6d149586b34a Apply dPR from selected device; r=jryans

(In reply to Pulsebot from comment #29) > https://hg.mozilla.org/integration/fx-team/rev/6d149586b34a Fixed the trailing space.

The issue is no longer reproducible on Firefox 51.0a1 (2016-09-15). The tests were performed under Windows 10x64, Mac OS X 10.11.1 and under Ubuntu 16.04x64. I am marking this issue Verified Fixed.