Closed Bug 1349559 Opened 3 years ago Closed 2 years ago

Introduce UI for network throttling in the Net panel

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(relnote-firefox 61+, firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
relnote-firefox --- 61+
firefox61 --- verified
firefox62 --- verified

People

(Reporter: Honza, Assigned: Honza)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [netmonitor])

Attachments

(2 files)

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
Priority: -- → P2
Hi Honza, is this bug a dependency of the new LaunchPad meta?
Flags: needinfo?(odvarko)
(In reply to Marco Mucci [:MarcoM] from comment #1)
> Hi Honza, is this bug a dependency of the new LaunchPad meta?
yes

Thanks!
Honza
Flags: needinfo?(odvarko)
Flags: qe-verify?
Whiteboard: [netmonitor]
Summary: Introduce UI for network thorttling in the Net panel → Introduce UI for network throttling in the Net panel
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Since Bug 1350228 is done, we can move network throttle UI to netmonitor
Depends on: 1350228
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.
Severity: normal → enhancement
Duplicate of this bug: 1418050
Duplicate of this bug: 1421948
Assignee: nobody → odvarko
@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
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
@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
5) Fixed v-centering of the label+icon on Mac (OSX specific CSS rule, not sure if there is a better way).

Honza
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?
Attachment #8970804 - Flags: review?(jryans) → review+
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.)
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
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
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)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daac5f4217b6
Introduce UI for network throttling in the Net panel; r=jryans
Test fixed, trying to land again.
Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14dc1b26fe73
Introduce UI for network throttling in the Net panel; r=jryans
https://hg.mozilla.org/mozilla-central/rev/14dc1b26fe73
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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
relnote-firefox: --- → ?
MDN doc should be updated to reflect this new toolbar button.
https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor
Honza
Keywords: dev-doc-needed
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.
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.
Flags: needinfo?(ryanvm)
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
Flags: needinfo?(odvarko)
@jryans: Please see the previous comment #32. Is there a doc or an existing STR for testing network throttling in RDM?

Honza
Flags: needinfo?(odvarko) → needinfo?(jryans)
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.
Flags: needinfo?(jryans) → needinfo?(mihai.boldan)
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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(mihai.boldan)
Hi , Sorry I took the Need Info request for Ryan by accident.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
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.
Flags: needinfo?(odvarko)
Flags: needinfo?(jryans)
(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
Flags: needinfo?(odvarko)
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
Looks great to me, thanks!

Honza
Flags: needinfo?(odvarko)
Depends on: 1485785
You need to log in before you can comment on or make changes to this bug.