Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Description • 7 years ago

The Network panel should offer its own UI for network throttling.

This features is already supported by the backend and is available in RDM. See bug 1244227 for more details.

See also this thread:
https://groups.google.com/a/mozilla.com/forum/?utm_medium=email&utm_source=footer#!msg/netmonitor/FH3v29Hh1Js/J9Ysq2MZAgAJ

Honza

	Marco Mucci [:MarcoM]
	Comment 1 • 7 years ago

Hi Honza, is this bug a dependency of the new LaunchPad meta?

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 2 • 7 years ago

(In reply to Marco Mucci [:MarcoM] from comment #1)
> Hi Honza, is this bug a dependency of the new LaunchPad meta?
yes

Thanks!
Honza

	gasolin@mozilla.com
	Comment 3 • 7 years ago

Since Bug 1350228 is done, we can move network throttle UI to netmonitor

	:Harald Kirschner :digitarald
	Comment 4 • 7 years ago

https://twitter.com/Melvin2204/status/914881760273555457 shows how users have a hard time finding the throttling in RDM.

+1 on P2, seems a good priority for this.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 8 • 6 years ago

@jryans: I am working on reusing the existing throttling UI in the Network panel.

First (wip) patch attached:

1) If you'd like to try the patch you also need patches from bug 1403530 and bug 1456129 (in this order). They are close to landing.

2) The patch is reusing the following modules:
const Types = require("devtools/client/responsive.html/types");
const NetworkThrottlingSelector = createFactory(require("devtools/client/responsive.html/components/NetworkThrottlingSelector"));
const { changeNetworkThrottling } = require("devtools/client/responsive.html/actions/network-throttling");

And reducer:

const networkThrottling = require("devtools/client/responsive.html/reducers/network-throttling");

Do you think that we should move these things into: client/shared/throttling dir?

3) The missing piece is reusing the ResponsiveUIManager (see my comment in Toolbar.js in onChangeNetworkThrottling method)

Is the entire ResponsiveUIManager module needed or should we extract `updateNetworkThrottling` + related API from it and reuse? The manager doesn't have to exist all the time, so the Net panel can't just post a message to it, right?

I am also attaching a screenshot of the UI in the Net panel (wip).

Honza

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 9 • 6 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> @jryans: I am working on reusing the existing throttling UI in the Network
> panel.

Aha, quite exciting!
 
> 2) The patch is reusing the following modules:
> const Types = require("devtools/client/responsive.html/types");
> const NetworkThrottlingSelector =
> createFactory(require("devtools/client/responsive.html/components/
> NetworkThrottlingSelector"));
> const { changeNetworkThrottling } =
> require("devtools/client/responsive.html/actions/network-throttling");
> 
> And reducer:
> 
> const networkThrottling =
> require("devtools/client/responsive.html/reducers/network-throttling");
> 
> Do you think that we should move these things into: client/shared/throttling
> dir?

Yes, let's move them to some kind of shared dir.  Maybe devtools/client/shared/components/throttling?  We seem to be stuffing React things in the "components" sub-dir.

We should also extract the CSS and strings from RDM so they can also move to a shared place.

> 3) The missing piece is reusing the ResponsiveUIManager (see my comment in
> Toolbar.js in onChangeNetworkThrottling method)
> 
> Is the entire ResponsiveUIManager module needed or should we extract
> `updateNetworkThrottling` + related API from it and reuse? The manager
> doesn't have to exist all the time, so the Net panel can't just post a
> message to it, right?

I think the cleanest approach would be to extract the `ResponsiveUIManager#updateNetworkThrottling` method to a new shared module that will create its own emulation front for this purpose.  You'd have to pass a debugger client and tab form when instanciating this new module so it can create the front.  Then both RDM and your code would use this new module as needed.

Maybe this new module would be devtools/client/shared/emulation.js?  It might make sense to move all `ResponsiveUIManager#update*` methods that use the emulation front into this module, even if only throttling is currently used outside RDM.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 11 • 6 years ago

@jryans: Thanks for the feedback!

I put together a patch, some comments:

1) The entire component (including actions, reducers, etc.) is shared here:
devtools/client/shared/components/throttling

2) There is a new file for strings: network-throttling.properties

3) The Network panel reuses the EmulationFront only (since it needs just small amount of code).

4) In the end I didn't share CSS since the Network panel Toolbar is quite different from RDM toolbar and there is not much to share.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 13 • 6 years ago

5) Fixed v-centering of the label+icon on Mac (OSX specific CSS rule, not sure if there is a better way).

Honza

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8970804 [details]
Bug 1349559 - Introduce UI for network throttling in the Net panel;

https://reviewboard.mozilla.org/r/239580/#review246234

Thanks!  This all looks good to me! :)

::: devtools/client/netmonitor/src/connector/firefox-connector.js:82
(Diff revision 3)
>      // reload, so `will-navigate` listener needs to be there all the time.
>      if (this.tabTarget) {
>        this.tabTarget.on("will-navigate", this.willNavigate);
>        this.tabTarget.on("navigate", this.navigate);
> +
> +      // Initialize Emulation front-end for network throttling.

Nit: I believe we typically refer to these as just "fronts".

::: devtools/client/netmonitor/src/connector/firefox-connector.js:100
(Diff revision 3)
>        this.actions.batchReset();
>      }
>  
>      await this.removeListeners();
>  
> +    if (this.emulationFront) {

I assume this code path means the toolbox is closing?  We shouldn't need to explicitly clear throttling in that case.  When the actor is destroyed, it should take care of it of automatically without being told to do so from the client, so we should be able to remove this.  (Though please test it to make sure...)

(I noticed that RDM currently _does_ clear on close like this, but it may not be needed there either.)

::: devtools/client/responsive.html/manager.js:511
(Diff revision 3)
>  
>      switch (event.data.type) {
>        case "change-device":
>          this.onChangeDevice(event);
>          break;
> -      case "change-network-throtting":
> +      case "change-network-throttling":

Haha, thanks for fixing this! :D

::: devtools/client/shared/components/throttling/moz.build:9
(Diff revision 3)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DevToolsModules(
> +    'actions.js',
> +    'network-throttling-profiles.js',

Maybe rename this to just `profiles`, since the directory gives away the purpose?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 15 • 6 years ago
mozreview-review-reply

Comment on attachment 8970804 [details]
Bug 1349559 - Introduce UI for network throttling in the Net panel;

https://reviewboard.mozilla.org/r/239580/#review246234

> I assume this code path means the toolbox is closing?  We shouldn't need to explicitly clear throttling in that case.  When the actor is destroyed, it should take care of it of automatically without being told to do so from the client, so we should be able to remove this.  (Though please test it to make sure...)
> 
> (I noticed that RDM currently _does_ clear on close like this, but it may not be needed there either.)

(Please don't change RDM's clear on close behavior in this bug though...  It may need more investigation to prove it's safe to remove.)

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 17 • 6 years ago

Thanks for the review Ryan!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Apr 30 - May 1) from comment #14)
> > +      // Initialize Emulation front-end for network throttling.
> 
> Nit: I believe we typically refer to these as just "fronts".
Fixed

> ::: devtools/client/netmonitor/src/connector/firefox-connector.js:100
> > +    if (this.emulationFront) {
> 
> I assume this code path means the toolbox is closing?  We shouldn't need to
> explicitly clear throttling in that case.  When the actor is destroyed, it
> should take care of it of automatically without being told to do so from the
> client, so we should be able to remove this.  (Though please test it to make
> sure...)
Removed

Testing looks good (opening the Toolbox, throttling, closing the Toolbox, throttling automatically reset)

> > +DevToolsModules(
> > +    'actions.js',
> > +    'network-throttling-profiles.js',
> 
> Maybe rename this to just `profiles`, since the directory gives away the
> purpose?
Good point, renamed.

> (Please don't change RDM's clear on close behavior in this bug though...  It
> may need more investigation to prove it's safe to remove.)
OK


Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 19 • 6 years ago

Eslint failures fixed and front destroyed on toolbox close in order to fix test failure:  devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js

Honza

	Mozilla Autoland
	Comment 20 • 6 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bfb621d242ec7f171602036858788a8703a8b2db -d ae08735a4830: rebasing 461121:bfb621d242ec "Bug 1349559 - Introduce UI for network throttling in the Net panel; r=jryans" (tip)
merging devtools/client/netmonitor/src/assets/styles/Toolbar.css
warning: conflicts while merging devtools/client/netmonitor/src/assets/styles/Toolbar.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 22 • 6 years ago

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daac5f4217b6
Introduce UI for network throttling in the Net panel; r=jryans

	Andrei Ciure[:aciure]
	Comment 23 • 6 years ago

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=daac5f4217b6a5c8b0607dcf5bdd713e68fc8529&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=ad861c124e5d793c3cb776ec3e50088a4786d547&selectedJob=176409162&filter-searchStr=Linux+opt+Xpcshell+tests+test-linux32%2Fopt-xpcshell-1+X%28X1%29

https://treeherder.mozilla.org/logviewer.html#?job_id=176409162&repo=autoland

backout: https://hg.mozilla.org/integration/autoland/rev/7ea290696af200867042c95b1dfbe7b44ce931fa

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 25 • 6 years ago

Test fixed, trying to land again.
Honza

	Pulsebot
	Comment 26 • 6 years ago

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14dc1b26fe73
Introduce UI for network throttling in the Net panel; r=jryans

	Cosmin Sabou [:CosminS]
	Comment 27 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/14dc1b26fe73

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 28 • 6 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: There is a new UI in DevTools Network panel
[Affects Firefox for Android]: no
[Suggested wording]: The Network panel has a new drop down toolbar button that can be used to throttle network bandwith.
[Links (documentation, blog post, etc)]:

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 29 • 6 years ago

MDN doc should be updated to reflect this new toolbar button.
https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor
Honza

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 6 years ago

The more I think about this, the more I think this is better suited for the developer notes on MDN and not the overall Fx61 release notes. Sorry for the confusion.

	:Harald Kirschner :digitarald
	Comment 31 • 6 years ago

Ryan, it might seem trivial, but was is a reoccurring feature request from users as the only other way for throttling was using Responsive Design Mode; which does a lot more to a website than just throttle it. This is also the way other browsers offer throttling and will make network performance work much easier.

It would be great if we could spread the word appropriately that this is finally addressed.

	Rares Doghi, Desktop QA
	Comment 32 • 6 years ago

Hi Jan, can you please help us with some guidance on testing this feature , some documentation or maybe some Steps that would help testing it. Thank you

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 33 • 6 years ago

@jryans: Please see the previous comment #32. Is there a doc or an existing STR for testing network throttling in RDM?

Honza

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 34 • 6 years ago

I am not sure what exists as far as test plans, etc.

The RDM bug that added this feature is bug 1283453.  The RDM version is documented at https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Network_throttling.

Mihai Boldan worked on QE for this RDM feature, so he may know more.

	Rares Doghi, Desktop QA
	Comment 35 • 6 years ago

Hi everyone, after talking to Mihai i retested this feature using the latest version of FireFox Nightly (61.0a1 (2018-05-06)) on a simple 10MB web page http://sms.playstation.com/  testing the Loading time for each Throttling mode and here are the results :
        
    - No Throttling                                          - DOMContentLoaded: 2.17 s load: 5.53 s
    - GPRS         -   50kbs - 20kbs (500ms)                 - DOMContentLoaded: 4.40 min
    - Regular 2G -   250 kbs - 50 kbs (300ms)                - DOMContentLoaded: 1.36 min
    - Good 2G     -   450 kbs - 150 kbs (150ms)              - DOMContentLoaded: 41.05 s
    - Regular 3G -   750 kbs - 250 kbs (100ms)               - DOMContentLoaded: 24.94 s
    - Good 3G     - 1.5 Mbs - 750 kbs (40ms)                 - DOMContentLoaded: 12.66 s
    - Regular 4G / LTE - 4 Mbs - 3Mbs (20ms)                 - DOMContentLoaded: 5.67 s
    - DSL             -     2 Mbs - 1Mbs (5ms)               - DOMContentLoaded: 10.11 s
    - Wifi              -     30 Mbs - 15 Mbs (2ms)          - DOMContentLoaded: 2.32 s load: 5.40 s

The Throttling functionality seems to be working correctly and from a UI point of view the "Separating Line" under "No Throttling" seems to be missing from Dev Tools - Network tab, it seems that it's only displayed for RDM.
Please let me know if i should log a defect for this issue and also if i missed something while testing the functionality of the Throttling, otherwise i'm ok closing this issue and mark it as VERIFIED.

	Rares Doghi, Desktop QA
	Comment 36 • 6 years ago

Hi , Sorry I took the Need Info request for Ryan by accident.

	Rares Doghi, Desktop QA
	Comment 37 • 6 years ago

Hey guys, can you take a look at Comment 35 , and let me know if i should add a separate defect for the "separating line" issue, or i can just mark this as Verified.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 38 • 6 years ago

(In reply to Rares Doghi from comment #37)
> Hey guys, can you take a look at Comment 35 , and let me know if i should
> add a separate defect for the "separating line" issue, or i can just mark
> this as Verified.

The separator line should be fixed in bug 1459207

Honza

	Rares Doghi, Desktop QA
	Comment 39 • 6 years ago

Thanks Guys, i will mark this as Verified since i rechecked this issue and it looks good from a UI point of view as well in Nightly 62.0a1 (2018-05-16).

	Chris Mills [:cmills]
	Comment 40 • 6 years ago

I've documented this one too:

https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools
https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Throttling

Let me know if this reads OK. Thanks!

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 41 • 6 years ago

Looks great to me, thanks!

Honza