Once we have the back end to throttle data (bug 1244227), we'll need to implement some UI to toggle it and adjust the settings. The user story (bug 1110207) gives a high level summary of what's desired and bug 1283445 will work out more details about the UX.

Comment on attachment 8799057 [details] Bug 1283453 - Add network throttling UI to RDM. https://reviewboard.mozilla.org/r/84350/#review83008 ::: devtools/client/responsive.html/data/network-throttling-profiles.js:18 (Diff revision 1) > +/** > + * Predefined network throttling profiles. > + * Speeds are in bytes per second. Latency is in ms. > + */ > +/* eslint-disable key-spacing */ > +module.exports = [ I would put this in devtools/client/shared/ for consistency with devices.js. It would be even better if we had devtools/client/shared/emulation-profiles.js that takes care of managing devices and network throttling. ::: devtools/client/responsive.html/index.css:84 (Diff revision 1) > > +select { > + -moz-appearance: none; > + background-color: var(--theme-toolbar-background); > + background-image: var(--viewport-selection-arrow); > + background-position: 100% 52%; nit: background-position: 100% calc(50% + 3.5px) would be better (3.5px is half of the background-size). ::: devtools/client/responsive.html/index.css:89 (Diff revision 1) > + background-position: 100% 52%; > + background-repeat: no-repeat; > + background-size: 7px; > + border: none; > + color: var(--viewport-color); > + padding: 0 8px 0 8px; nit: padding: 0 8px; ::: devtools/client/responsive.html/index.css:93 (Diff revision 1) > + color: var(--viewport-color); > + padding: 0 8px 0 8px; > + text-align: center; > + text-overflow: ellipsis; > + font-size: 11px; > + width: -moz-fit-content; nit: width: -moz-fit-content is the default value for selects, it was probably set before to override width: 150px; that was accidently set. ::: devtools/client/responsive.html/index.css:117 (Diff revision 1) > +select > option:hover, > +select:hover > option:hover { nit: I know it was there before, but select:hover > option:hover is redundant with select > option:hover ::: devtools/client/responsive.html/index.css:122 (Diff revision 1) > +select > option:hover, > +select:hover > option:hover { > + color: var(--viewport-active-color); > +} > + > +select > option.divider { nit: I would add pointer-events: none; to make sure this is un-clickable.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #4) > Comment on attachment 8799057 [details] > Bug 1283453 - Add network throttling UI to RDM. > > https://reviewboard.mozilla.org/r/84350/#review83008 > > ::: devtools/client/responsive.html/data/network-throttling-profiles.js:18 > (Diff revision 1) > > +/** > > + * Predefined network throttling profiles. > > + * Speeds are in bytes per second. Latency is in ms. > > + */ > > +/* eslint-disable key-spacing */ > > +module.exports = [ > > I would put this in devtools/client/shared/ for consistency with devices.js. > > It would be even better if we had > devtools/client/shared/emulation-profiles.js that takes care of managing > devices and network throttling. meant network throttling profiles.

Comment on attachment 8799056 [details] Bug 1283453 - Add network throttling to emulation actor. https://reviewboard.mozilla.org/r/84348/#review83150 Thank you. This looks good to me. ::: devtools/server/actors/emulation.js:22 (Diff revision 1) > + * needed because it's possible to have multiple copies of this actor for a single page. > + * When some instance of this actor changes a property, we want it to be able to restore > + * that property to the way it was found before the change. > + * > + * A subtle aspect of the code below is that all get* methods must return non-undefined > + * values, so that the absence of a previous value can be distinguised from the value for Typo, "distinguished"

Comment on attachment 8799057 [details] Bug 1283453 - Add network throttling UI to RDM. https://reviewboard.mozilla.org/r/84350/#review83088 ::: devtools/client/responsive.html/components/network-throttling-selector.js:15 (Diff revision 1) > +const Types = require("../types"); > +const { getStr } = require("../utils/l10n"); > +const throttlingProfiles = require("../data/network-throttling-profiles"); > + > +module.exports = createClass({ > + displayName: "NetworkThrottlingSelector", We seem to either start with a empty new line or none in this case among our components. We should probably stick with one style for everything. No particular preference. ::: devtools/client/responsive.html/components/network-throttling-selector.js:29 (Diff revision 1) > + onSelectChange({ target }) { > + let { > + onChangeNetworkThrottling, > + } = this.props; > + > + for (let profile of throttlingProfiles) { We should probably have a check to see if "No throttling" is the value and do onChangeNetworkThrottling(false, ""); and return early before looping through the throttling profiles. ::: devtools/client/responsive.html/index.css:106 (Diff revision 1) > +select:hover { > + background-image: var(--viewport-selection-arrow-hovered); > + color: var(--viewport-hover-color); > +} > + > +select:focus { We should pair select.selected, select:focus since the properties are both the same. ::: devtools/client/responsive.html/manager.js:377 (Diff revision 1) > this.tab = null; > this.inited = null; > this.toolWindow = null; > this.swap = null; > > // Close the debugger client used to speak with emulation actor Should add a period to the end of the comment. ::: devtools/client/responsive.html/manager.js:431 (Diff revision 1) > if (event.origin !== "chrome://devtools") { > return; > } > > switch (event.data.type) { > - case "change-viewport-device": > + case "change-network-throtting": { Haven't seen this style of using switch cases with embedding the logic of the case statements in { } break;. Why did we decide to switch the styles? ::: devtools/client/responsive.html/manager.js:469 (Diff revision 1) > + this.emulationFront.clearDPPXOverride(); > } > }, > > - updateTouchSimulation: Task.async(function* (enabled) { > + updateNetworkThrottling: Task.async(function* (enabled, profile) { > if (enabled) { I would've preferred if we did: if (!enabled) { yield this.emulationFront.clearNetworkThrottling(); } and keep the reminder of the updateNetworkThrottling logic un-indented to make it look a bit cleaner. This could be done in updateDPPX, updateTouchSimulation and updateUserAgent ::: devtools/client/responsive.html/types.js:117 (Diff revision 1) > +/** > + * Network throttling. > + */ > +exports.networkThrottling = { > + > + // Whether or not touch simulation is enabled Whether or not networking throlling is enabled.

Comment on attachment 8799057 [details] Bug 1283453 - Add network throttling UI to RDM. https://reviewboard.mozilla.org/r/84350/#review83008 > I would put this in devtools/client/shared/ for consistency with devices.js. > > It would be even better if we had devtools/client/shared/emulation-profiles.js that takes care of managing devices and network throttling. Ah, good idea. I can imagine we'd want to use this from other tools as well, so makes sense! > nit: background-position: 100% calc(50% + 3.5px) would be better (3.5px is half of the background-size). After some quick tests, it seems like background-position already considers the size, so 50% might actually be what is desired? In any case, calc(50% + 3.5px) did not appear to work for me. > nit: padding: 0 8px; Thanks, fixed. > nit: width: -moz-fit-content is the default value for selects, it was probably set before to override width: 150px; that was accidently set. Removed, thanks! > nit: I know it was there before, but > select:hover > option:hover is redundant with select > option:hover Thanks, fixed! > nit: I would add pointer-events: none; to make sure this is un-clickable. Docs of the disabled attribute say "If it is disabled, it does not accept clicks.", so I think it's okay as is.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > Comment on attachment 8799057 [details] > Bug 1283453 - Add network throttling UI to RDM. > > https://reviewboard.mozilla.org/r/84350/#review83008 > > > nit: background-position: 100% calc(50% + 3.5px) would be better (3.5px is half of the background-size). > After some quick tests, it seems like background-position already considers > the size, so 50% might actually be what is desired? In any case, calc(50% + > 3.5px) did not appear to work for me. I'm fine with 50%, as long as we don't have any arbitrary values like 52%. > > nit: I would add pointer-events: none; to make sure this is un-clickable. > > Docs of the disabled attribute say "If it is disabled, it does not accept > clicks.", so I think it's okay as is. Oh, I didn't notice the disabled attribute, sorry! Should be fine without pointer-events then.

Comment on attachment 8799057 [details] Bug 1283453 - Add network throttling UI to RDM. https://reviewboard.mozilla.org/r/84350/#review83088 > We seem to either start with a empty new line or none in this case among our components. We should probably stick with one style for everything. No particular preference. I think I like the empty line, so I added it. > We should probably have a check to see if "No throttling" is the value and do onChangeNetworkThrottling(false, ""); and return early before looping through the throttling profiles. Okay, seems fine. > We should pair select.selected, select:focus since the properties are both the same. The current ordering seems to be intentional, since all 3 states stack on top of each other: 1. select.selected when some item is chosen and the feature is active (uses select.selected block) 2. select:hover when the select.selected is hovered (select:hover block takes precidence) 3. select:focus when the select.selected is focused (select:focus block takes precidence) Now, I can't really remember if the visual effect this causes is actually intentional or not... In any case, I'll add a comment about the ordering. > Should add a period to the end of the comment. Added. > Haven't seen this style of using switch cases with embedding the logic of the case statements in { } break;. Why did we decide to switch the styles? Yeah, I don't think we've used it before, I borrowed it from a C style I've seen. It was needed because there are now two `enabled` variables in different parts of the switch statement, and case clauses don't count as separate blocks by default. Adding braces makes the separate blocks. I guess a more normal approach would be to make separate functions for each case clause... I guess I'll change to that. > I would've preferred if we did: > > if (!enabled) { > yield this.emulationFront.clearNetworkThrottling(); > } > > and keep the reminder of the updateNetworkThrottling logic un-indented to make it look a bit cleaner. > > This could be done in updateDPPX, updateTouchSimulation and updateUserAgent Okay, seems fine to me too. Updated all of them. > Whether or not networking throlling is enabled. Fixed typo.

Comment on attachment 8799056 [details] Bug 1283453 - Add network throttling to emulation actor. https://reviewboard.mozilla.org/r/84348/#review83150 > Typo, "distinguished" Thanks, fixed.

Comment on attachment 8799057 [details] Bug 1283453 - Add network throttling UI to RDM. This looks really good! I have one small request: Currently the space between the label and the "Responsive Design Mode |" looks a lot wider on one side than it does on the other. Can we tighten that? Screenshot of current build vs. what I'd like: https://cl.ly/2U0A1s05261o

Thanks for the review! Attached image shows how it looks now after I made some adjustments to the CSS.

Updated build-only try run for easier UI review: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed91133762d97506dafd2d75228669a1d113603

Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f289545ab7d Add network throttling to emulation actor. r=tromey https://hg.mozilla.org/integration/mozilla-inbound/rev/56b16d2eaa77 Add network throttling UI to RDM. r=gl

https://hg.mozilla.org/mozilla-central/rev/9f289545ab7d https://hg.mozilla.org/mozilla-central/rev/56b16d2eaa77

I still see the old RDM UI even with devtools.responsive.html.enabled set to true. Is there something else that needs to be done to see the new UI? Sebastian

Ah, my fault. I tried it on my e10s-disabled profile, which I have to keep Firebug running. With e10s enabled it works fine. Sebastian

I managed to perform a few test around this bug and noticed some potential issues: - On Ubuntu and Windows OS, the Network Throttling selection and the drop down button are not vertically aligned. - on Ubuntu OS, the size of the text is not the same with the size of the "Responsive Design Mode" string, as on the other 2 OSs. Here is a screenshot with the issues mentioned above: https://i.imgur.com/6WFikTY.png So just to be clear, the Edit button and the throttling options will be added/modified accordingly in Bug 1283445? Please let me know if I should verify anything else around this fix. The tests were performed on Firefox 52.0a1 (2016-10-20), under Windows 10x 64, Mac OS X 10.11.6 and under Ubuntu 16.04 x86.

(In reply to Mihai Boldan, QA [:mboldan] from comment #23) > I managed to perform a few test around this bug and noticed some potential > issues: > - On Ubuntu and Windows OS, the Network Throttling selection and the drop > down button are not vertically aligned. > - on Ubuntu OS, the size of the text is not the same with the size of the > "Responsive Design Mode" string, as on the other 2 OSs. > Here is a screenshot with the issues mentioned above: > https://i.imgur.com/6WFikTY.png Okay, let's file a bug for these visual issues. Probably one bug for both is enough. > So just to be clear, the Edit button and the throttling options will be > added/modified accordingly in Bug 1283445? We don't have immediate plans to add the edit button and custom throttling yet. There's a UX spec in bug 1283445 for it, but some new bug would be filed for the implementation. For the moment, those options aren't available. Thanks for testing!

Bug 1312672 was filled for the issues described in Comment 23. Since that the found issues were logged separately, I am marking this issue Verified Fixed.

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