1005755 - A button to pause/stop the HTTP traffic in the network monitor view

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Karl Dubost💡 :karlcow]

When inspecting the HTTP traffic of a Web site in the "Network Monitor" view, there are moments you would like to pause all future network transactions. Providing a button to pause or stop the traffic would be excellent.

For example, Huffington Mobile Web site provides a beacon loaded through XHR pinging the mother site every x seconds. When inspecting, you need to have a stable view which is not evolving anymore.

Comment 1

[:Harald Kirschner :digitarald]

Honza, to estimate priority vs cost; do you have a sense for how feasible this is? Maybe we can build on event queue from Quantum DOM and could pause the page's event queue?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to :Harald Kirschner :digitarald from comment #1)
> Honza, to estimate priority vs cost; do you have a sense for how feasible
> this is? Maybe we can build on event queue from Quantum DOM and could pause
> the page's event queue?
Not sure how this would affect the rest of the page behavior and e.g. debugging.

Tom, could we use the throttling API for this somehow?

Honza

Comment 3

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)

> Tom, could we use the throttling API for this somehow?

I think so -- or at least it is worth a try -- depending on what the goal is.

With throttling, the network communication still happens.  However we
just buffer the results and release them back to content more slowly (or
in this case hold them indefinitely).  So, if you want to avoid the connection
and transfer of data entirely, then throttling won't work.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Tom Tromey :tromey from comment #3)
> With throttling, the network communication still happens.  However we
> just buffer the results and release them back to content more slowly (or
> in this case hold them indefinitely).  So, if you want to avoid the
> connection
> and transfer of data entirely, then throttling won't work.
I see, thanks for the update.

(In reply to Karl Dubost :karlcow from comment #0)
> For example, Huffington Mobile Web site provides a beacon loaded through XHR
> pinging the mother site every x seconds. When inspecting, you need to have a
> stable view which is not evolving anymore.
So, we can just stop the monitoring.

What about introducing a toggle button on the Net panel's toolbar allowing:

1) Stop recording network log
2) Record network log

If disabled, the page would continue sending/receiving HTTP request/responses, but
the Network panel would not be listening. The view would be stable then.

Honza

Comment 5

[Karl Dubost💡 :karlcow]

> So, we can just stop the monitoring.

yup that would work in the beacon case.

That would not work in the case where you get a JS or html (meta) reloading the full page (like some news Web sites).

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1005755 - Introduce pause/resume button;

Review commit: https://reviewboard.mozilla.org/r/185032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185032/

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1005755 - Stop using Connector as a singleton;

Review commit: https://reviewboard.mozilla.org/r/185034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185034/

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1005755 - Update tests;

Review commit: https://reviewboard.mozilla.org/r/185036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185036/

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1005755 - Update Net logging in the Console;

Review commit: https://reviewboard.mozilla.org/r/185038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185038/

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

1) Bug 1005755 - Introduce pause/resume button;
@Ricky: Introducing new toolbar button. There is a new action, reducer and selector. The button will be used to pause/resume the recording.

2) Bug 1005755 - Stop using Connector as a singleton;
@Ricky: This is quite a few changes, but essentially it's all about passing 'connector' object down the component hierarchy. This patch stops using the Connector as a global singleton and it rather treats it as a property to components.

3) Bug 1005755 - Update tests;
@Ricky: updating existing tests

4) Bug 1005755 - Update Net logging in the Console;
@Nicolas: This is just a small update on the Console side. A connector object (mostly empty) must be passed down the hierarchy since it's mandatory prop. It's currently used only to open stack frame in the debugger, which should fix Bug 1403898.

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Karl Dubost :karlcow from comment #5)
> > So, we can just stop the monitoring.
> 
> yup that would work in the beacon case.
> 
> That would not work in the case where you get a JS or html (meta) reloading
> the full page (like some news Web sites).
We wanted to have the pause/resume feature and I used this bug report for it, hope it's ok :-)

We might need another bug for the thing you mentioned in comment #5
Currently, reloading the page resumes the recording.
(I can do that report if you want me)

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Harald: I used pause/resume icons from the debugger for that button. It looks good, but we might want to use something specific. Just let me know what you think.

Honza

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=928c73cd035a67adc0f2fbd353aca88c1d35e17c

Honza

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

https://reviewboard.mozilla.org/r/185038/#review190132

This looks good to me, although i'm not sure how this is related to the bug, but I trust you :)
Sonce you'are adding viewSourceInDebugger, i think we need a mochitest to test that in the console context.

Comment 15

[:Harald Kirschner :digitarald]

Honza, LGTM on a UI level! Just to confirm an affordance requirement: If the panel is in paused state, would it start recording when on user-initiated refresh?

Addressing Karl's feedback: breaking on network requests is often requested and would solve the second issue of pausing the whole site state.

Comment 16

[[e-mail]]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

https://reviewboard.mozilla.org/r/185032/#review190216

looks good, need to keep declaration in alphabet orders.

::: devtools/client/netmonitor/src/actions/requests.js:112
(Diff revision 1)
>    clearRequests,
>    cloneSelectedRequest,
>    removeSelectedCustomRequest,
>    sendCustomRequest,
>    updateRequest,
> +  toggleRecording

should be in alphabet order under sendCustomRequest, and keep the trail `,`

::: devtools/client/netmonitor/src/components/toolbar.js:22
(Diff revision 1)
>  const {
>    getDisplayedRequestsSummary,
>    getRequestFilterTypes,
>    getTypeFilteredRequests,
>    isNetworkDetailsToggleButtonDisabled,
> +  getRecordingState,

should in alphabet order

::: devtools/client/netmonitor/src/constants.js:8
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const actionTypes = {
> +  TOGGLE_RECORDING: "TOGGLE_RECORDING",

please keep the alphabet order

::: devtools/client/netmonitor/src/reducers/requests.js:10
(Diff revision 1)
>  "use strict";
>  
>  const I = require("devtools/client/shared/vendor/immutable");
>  const { getUrlDetails } = require("../utils/request-utils");
>  const {
> +  TOGGLE_RECORDING,

alphabet order

::: devtools/client/netmonitor/src/reducers/requests.js:171
(Diff revision 1)
>          st.lastEndedMillis = lastEndedMillis;
>        });
>      }
>      case CLEAR_REQUESTS: {
> -      return new Requests();
> +      return new Requests({
> +        recording: state.recording

If user cleared all requests, he might want to start over again. Why we need to just keep the current recording state?

::: devtools/client/netmonitor/src/selectors/requests.js:146
(Diff revision 1)
>    getDisplayedRequestsSummary,
>    getRequestById,
>    getSelectedRequest,
>    getSortedRequests,
>    getTypeFilteredRequests,
> +  getRecordingState,

alphabet order

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

https://reviewboard.mozilla.org/r/185032/#review190440

LGTM for the pause/resume button.

::: devtools/client/netmonitor/src/actions/requests.js:15
(Diff revision 1)
>    CLEAR_REQUESTS,
>    CLONE_SELECTED_REQUEST,
>    REMOVE_SELECTED_CUSTOM_REQUEST,
>    SEND_CUSTOM_REQUEST,
>    UPDATE_REQUEST,
> +  TOGGLE_RECORDING

nit: alphabetical order

::: devtools/client/netmonitor/src/components/toolbar.js:153
(Diff revision 1)
>  
> +    // Calculate class-list for toggle recording button. The button
> +    // has two states: pause/play.
> +    let toggleButtonClassList = [
> +      "devtools-button",
> +      recording ? "devtools-pause-icon" : "devtools-play-icon"

nit: tailing comma at the end of array.

::: devtools/client/netmonitor/src/components/toolbar.js:232
(Diff revision 1)
>      persistentLogsEnabled: state.ui.persistentLogsEnabled,
>      browserCacheDisabled: state.ui.browserCacheDisabled,
>      requestFilterTypes: getRequestFilterTypes(state),
>      filteredRequests: getTypeFilteredRequests(state),
>      summary: getDisplayedRequestsSummary(state),
> +    recording: getRecordingState(state),

nit: alphabetical order

::: devtools/client/netmonitor/src/components/toolbar.js:236
(Diff revision 1)
>      summary: getDisplayedRequestsSummary(state),
> +    recording: getRecordingState(state),
>    }),
>    (dispatch) => ({
>      clearRequests: () => dispatch(Actions.clearRequests()),
> +    toggleRecording: () => dispatch(Actions.toggleRecording()),

nit: alphabetical order

::: devtools/client/netmonitor/src/middleware/recording.js:17
(Diff revision 1)
> + * Start/stop HTTP traffic recording.
> + */
> +function recordingMiddleware(store) {
> +  return next => action => {
> +    const res = next(action);
> +    if (action.type == TOGGLE_RECORDING) {

nit: === instead of ==

::: devtools/client/netmonitor/src/reducers/requests.js:101
(Diff revision 1)
>    });
>  }
>  
>  function requestsReducer(state = new Requests(), action) {
>    switch (action.type) {
> +    case TOGGLE_RECORDING: {

nit: swtich case should be order in alphabetical order.

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8913639 [details]
Bug 1005755 - Stop using Connector as a singleton;

https://reviewboard.mozilla.org/r/185034/#review190448

I'm not seeing major issues from this patch. Please address all nits like naming or alphabetical order...etc. Thanks!

I also see some code improvement from this patch. Well done!

::: devtools/client/netmonitor/index.html:67
(Diff revision 1)
>              top.openUILinkIn(link, "tab");
>            };
>  
> -          const App = createFactory(require("./src/components/app"));
> +          // Render the root Application component.
>            const sourceMapService = toolbox.sourceMapURLService;
> -          const app = App({ sourceMapService, openLink });
> +          const app = App({ sourceMapService, openLink, connector });

nit: alphabetical order

::: devtools/client/netmonitor/index.html:85
(Diff revision 1)
> +         * Selects the specified request in the waterfall and opens the details view.
> +         *
> +         * @param {string} requestId The actor ID of the request to inspect.
> +         * @return {object} A promise resolved once the task finishes.
> +         */
> +        inspectRequest(requestId) {

inspectRequest is only using by Firefox internal devtools. (Link to netmonitor from other panel)

Moving from firefox-connector to index.html looks fine to me.

At least we can add a comment here to explain this is a firefox toolbox specific API, which providing an ablity to inspect a network request directly from other internal toolbox panel.

::: devtools/client/netmonitor/src/components/app.js:35
(Diff revision 1)
>    return (
>      div({ className: "network-monitor" },
> -      !statisticsOpen ? MonitorPanel({ openLink, sourceMapService }) : StatisticsPanel()
> +      !statisticsOpen ? MonitorPanel({
> +        openLink,
> +        sourceMapService,
> +        connector,

nit: alphabetical order

::: devtools/client/netmonitor/src/components/app.js:37
(Diff revision 1)
> -      !statisticsOpen ? MonitorPanel({ openLink, sourceMapService }) : StatisticsPanel()
> +      !statisticsOpen ? MonitorPanel({
> +        openLink,
> +        sourceMapService,
> +        connector,
> +      }) : StatisticsPanel({
> +        connector

nit: it's okay to write it as one line pattern if there is only one property.

```
StatisticsPanel({ connector })
```

::: devtools/client/netmonitor/src/components/monitor-panel.js:147
(Diff revision 1)
>      );
>    }
>  });
>  
>  module.exports = connect(
> -  (state) => ({
> +  (state, props) => ({

nit: remove `props` since it is unused here.

::: devtools/client/netmonitor/src/components/network-details-panel.js:42
(Diff revision 1)
>  
>    return (
>      div({ className: "network-details-panel" },
>        !request.isCustom ?
>          TabboxPanel({
> +          connector,

nit: alphabetical order

::: devtools/client/netmonitor/src/components/request-list-content.js:38
(Diff revision 1)
>   */
>  const RequestListContent = createClass({
>    displayName: "RequestListContent",
>  
>    propTypes: {
> +    connector: PropTypes.object.isRequired,

nit: alphabetical order

::: devtools/client/netmonitor/src/components/request-list-content.js:57
(Diff revision 1)
> +      getLongString: connector.getLongString,
> +      getTabTarget: connector.getTabTarget,

nit: alphabetical order

::: devtools/client/netmonitor/src/components/request-list.js:24
(Diff revision 1)
>  const { div } = DOM;
>  
>  /**
>   * Request panel component
>   */
> -function RequestList({ isEmpty }) {
> +function RequestList({ isEmpty, connector }) {

nit: define properties as multi-line pattern.

::: devtools/client/netmonitor/src/components/tabbox-panel.js:39
(Diff revision 1)
> -/*
> +/**
>   * Tabbox panel component
>   * Display the network request details
>   */
>  function TabboxPanel({
> +  connector,

nit: alphabetical order

::: devtools/client/netmonitor/src/components/tabbox-panel.js:110
(Diff revision 1)
>  }
>  
>  TabboxPanel.displayName = "TabboxPanel";
>  
>  TabboxPanel.propTypes = {
> +  connector: PropTypes.object.isRequired,

nit: alphabetical order

::: devtools/client/netmonitor/src/connector/firefox-connector.js:44
(Diff revision 1)
>      this.dataProvider = new FirefoxDataProvider({
>        webConsoleClient: this.webConsoleClient,
>        actions: this.actions,
>      });
>  
> -    this.tabTarget.on("will-navigate", this.willNavigate);
> +    this.addListeners();

nit:

How about registerListeners / setupListeners

::: devtools/client/netmonitor/src/connector/firefox-connector.js:67
(Diff revision 1)
>      if (this.tabTarget.getTrait("documentLoadingMarkers") && this.timelineFront) {
>        this.timelineFront.off("doc-loading", this.onDocLoadingMarker);
>        await this.timelineFront.destroy();
>      }
>  
> -    this.tabTarget.off("will-navigate");
> +    this.removeListeners();

nit:

How about unregisterListener / teardownListeners?

::: devtools/client/netmonitor/src/connector/index.js:15
(Diff revision 1)
> + * to the client type.
> + */
> +class Connector {
> +  constructor() {
> +    this.connector = null;
> +

Do we need to bind `connect` and `disconnect` APIs as well?

::: devtools/client/netmonitor/src/har/test/browser_net_har_throttle_upload.js:19
(Diff revision 1)
>    let { tab, monitor } = yield initNetMonitor(
>      HAR_EXAMPLE_URL + "html_har_post-data-test-page.html");
>  
>    info("Starting test... (actuallyThrottle = " + actuallyThrottle + ")");
>  
> -  let { store, windowRequire } = monitor.panelWin;
> +  let { store, connector, windowRequire } = monitor.panelWin;

nit: alphabetical order

::: devtools/client/netmonitor/src/middleware/recording.js:30
(Diff revision 1)
> +
> +    // Pause/resume HTTP monitoring according to
> +    // the user action.
>      if (action.type == TOGGLE_RECORDING) {
> -      // TODO connect/disconnect the backend.
> +      let state = store.getState();
> +      let recording = getRecordingState(state);

nit: define state as a variable but doens't use in other places looks weird.

`getRecordingState(store.getState())` is enough.

::: devtools/client/netmonitor/src/request-list-context-menu.js:26
(Diff revision 1)
> +  getLongString,
> +  getTabTarget,

nit: alphabetical order

::: devtools/client/netmonitor/src/request-list-context-menu.js:31
(Diff revision 1)
> +  getLongString,
> +  getTabTarget,
>    cloneSelectedRequest,
>    openStatistics,
>  }) {
> +  this.getLongString = getLongString;

nit: alphabetical order

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

https://reviewboard.mozilla.org/r/185036/#review190466

LGTM. Thank you Honza :)

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185032/diff/1-2/

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913639 [details]
Bug 1005755 - Stop using Connector as a singleton;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185034/diff/1-2/

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185036/diff/1-2/

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185038/diff/1-2/

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> This looks good to me, although i'm not sure how this is related to the bug,
> but I trust you :)
See the comment #0: "When inspecting the HTTP traffic of a Web site in the "Network Monitor" view, there are moments you would like to pause all future network transactions. Providing a button to pause or stop the traffic would be excellent."

> Sonce you'are adding viewSourceInDebugger, i think we need a mochitest to
> test that in the console context.
Agree, I am working on it.

(In reply to Fred Lin [:gasolin] from comment #16)
> If user cleared all requests, he might want to start over again. Why we need
> to just keep the current recording state?
I think that auto-start is what we want (also to avoid case where the user forget about it and is confused that the net monitor doesn't work). Btw. Chrome DT is doing the same. 

(In reply to Ricky Chien [:rickychien] from comment #18)
> How about registerListeners / setupListeners
I kept 'add/removeListeners' since searching existing DT code base shows that this is more consistent


(In reply to :Harald Kirschner :digitarald from comment #15)
> Honza, LGTM on a UI level! Just to confirm an affordance requirement: If the
> panel is in paused state, would it start recording when on user-initiated
> refresh?
Yes

@Ricky, Fred, Nicolas: thanks for the review!

Honza

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1005755 - Test for pause/stop button;

Review commit: https://reviewboard.mozilla.org/r/185582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185582/

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185582/diff/1-2/

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #24)
> (In reply to Fred Lin [:gasolin] from comment #16)
> > If user cleared all requests, he might want to start over again. Why we need
> > to just keep the current recording state?
> I think that auto-start is what we want (also to avoid case where the user
> forget about it and is confused that the net monitor doesn't work). Btw.
> Chrome DT is doing the same. 
Sorry, my comment was related to re-load. Not sure if clear should also auto-start
and at least Chrome DT doesn't do it.

Honz

Comment 28

[[e-mail]]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

https://reviewboard.mozilla.org/r/185032/#review191718

::: devtools/client/netmonitor/src/actions/requests.js:14
(Diff revision 2)
>    ADD_REQUEST,
>    CLEAR_REQUESTS,
>    CLONE_SELECTED_REQUEST,
>    REMOVE_SELECTED_CUSTOM_REQUEST,
>    SEND_CUSTOM_REQUEST,
> +  TOGGLE_RECORDING

trail comma is required,

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185032/diff/2-3/

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913639 [details]
Bug 1005755 - Stop using Connector as a singleton;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185034/diff/2-3/

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185036/diff/2-3/

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185038/diff/2-3/

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185582/diff/2-3/

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Fred Lin [:gasolin] from comment #28)
> > +  TOGGLE_RECORDING
> 
> trail comma is required,
Fixed

Honza

Comment 35

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push looks good,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e17ecc3133b853055b49957f7537bd868c2f4a

Honza

Comment 36

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am planning two follow-ups for the current work:

* Resume automatically after page reload
* Test for 'View source in Debugger' feature (Console panel).

Honza

Comment 37

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

https://reviewboard.mozilla.org/r/185582/#review194344

::: devtools/client/netmonitor/test/browser_net_pause.js:11
(Diff revision 3)
> +/**
> + * Tests if the pause/resume button works.
> + */
> +add_task(function* () {
> +  let { tab, monitor } = yield initNetMonitor(CUSTOM_GET_URL);
> +  info("Starting test... ");

nit: s/.../…

::: devtools/client/netmonitor/test/browser_net_pause.js:14
(Diff revision 3)
> +add_task(function* () {
> +  let { tab, monitor } = yield initNetMonitor(CUSTOM_GET_URL);
> +  info("Starting test... ");
> +
> +  let { document, store, windowRequire } = monitor.panelWin;
> +  let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");

nit: feels a bit weird to have a variable capitalized without being a component/class

::: devtools/client/netmonitor/test/browser_net_pause.js:31
(Diff revision 3)
> +  // Click pause again to resume monitoring. Load a third request
> +  // and make sure they will show up.
> +  EventUtils.sendMouseEvent({ type: "click" }, pauseButton);
> +  yield performRequestAndWait(tab, monitor);
> +  assertRequestCount(store, 2);

maybe it would make sense to resume, test that we still have only 1 request (i.e. that the second one was indeed not listened), then do the third request and ensure we have 2 requests in the store.

::: devtools/client/netmonitor/test/browser_net_pause.js:67
(Diff revision 3)
> +  // A new request would definitely appear in 500 ms in the UI.
> +  // Declare victory if there is no request in this amount of time.
> +  yield waitForTime(500);

this looks error prone to me. don't we have other events that we can still listen to even when paused that would assert the request was sent back ? something like adding a one time listener on a client for example.


i acknowledge it might not be doable at all :) but it would be nice to be able to rely on something else than waiting for some time

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185032/diff/3-4/

Comment 39

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913639 [details]
Bug 1005755 - Stop using Connector as a singleton;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185034/diff/3-4/

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185036/diff/3-4/

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185038/diff/3-4/

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185582/diff/3-4/

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #37)
> > +  info("Starting test... ");
> nit: s/.../…
> nit: feels a bit weird to have a variable capitalized without being a
> component/class
I kept this the same since all the other tests use it too. Let's have
it consistent for now. But, I am not against to fix all at once as
part of another bug.

> maybe it would make sense to resume, test that we still have only 1 request
> (i.e. that the second one was indeed not listened), then do the third
> request and ensure we have 2 requests in the store.
I added on new assert.

> this looks error prone to me.
You are right, Timeouts are always evil. I found another solution
see the new version.

Thanks!
Honza

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push is green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44069e0da607bdc9873ec12319a175b33545c595

Honza

Comment 45

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

https://reviewboard.mozilla.org/r/185582/#review194732

Looks good, thanks Honza

::: devtools/client/netmonitor/test/browser_net_pause.js:44
(Diff revision 4)
> +  assertRequestCount(store, 1);
> +
> +  // Click pause again to resume monitoring. Load a third request
> +  // and make sure they will show up.
> +  EventUtils.sendMouseEvent({ type: "click" }, pauseButton);
> +  assertRequestCount(store, 1);

not sure if we really need this one, we already have the test in l.39

::: devtools/client/netmonitor/test/browser_net_pause.js:85
(Diff revision 4)
> +
> +/**
> + * Listen for events fired by the console client since the Firefox
> + * connector (data provider) is paused.
> + */
> +function waitForPausedRequest(connector) {

nit: could we find a better name, i find this one to be a bit misleading

Comment 46

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185032/diff/4-5/

Comment 47

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913639 [details]
Bug 1005755 - Stop using Connector as a singleton;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185034/diff/4-5/

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185036/diff/4-5/

Comment 49

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185038/diff/4-5/

Comment 50

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185582/diff/4-5/

Comment 51

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #45)
> Comment on attachment 8914263 [details]
> Bug 1005755 - Test for pause/stop button;
> not sure if we really need this one, we already have the test in l.39
Removed

> nit: could we find a better name, i find this one to be a bit misleading
I renamed it to `waitForWebConsoleNetworkEvent`

Thanks,
Honza

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f56f7ba748d
Introduce pause/resume button; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/2bc7565e6890
Stop using Connector as a singleton; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/a417d046f2f9
Update tests; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/99b7794639c8
Update Net logging in the Console; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1d56f718253d
Test for pause/stop button; r=nchevobbe

Comment 53

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Two follow ups:
Bug 1408964 - The Network panel should be auto resumed on reload
Bug 1408970 - Introduce test for view source in debugger

Honza

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing clipboard's devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js:

https://hg.mozilla.org/integration/autoland/rev/8468f493cf232c317677feb6a063643ad87aaade
https://hg.mozilla.org/integration/autoland/rev/b62adcd475fe96a7b028d6b63214cd7dc837cadb
https://hg.mozilla.org/integration/autoland/rev/af10498153bcaa4dd770eac98b658761e7038b73
https://hg.mozilla.org/integration/autoland/rev/b8bbfb934bc3f0d3d936718720ffbd4640da20c9
https://hg.mozilla.org/integration/autoland/rev/19322b88b4fc477316e1ff2f9fc8a8a98d21ad9a

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1d56f718253d6b74af4b73b65a3b8c8ce221be86&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
The test run on more platforms for the next push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=806b195734ee21a234ddf861cad843f215dc60f5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137207635&repo=autoland

[task 2017-10-16T10:53:13.967Z] 10:53:13     INFO - > Network events progress: 12/13, 0/0, got NetMonitor:NetworkEventUpdating:ResponseContent for server1.conn0.child1/netEvent28
[task 2017-10-16T10:53:13.968Z] 10:53:13     INFO - > Network events progress: 13/13, 0/0, got NetMonitor:NetworkEventUpdated:ResponseContent for server1.conn0.child1/netEvent28
[task 2017-10-16T10:53:13.969Z] 10:53:13     INFO - > Network events progress: 13/13, 0/0, got NetMonitor:PayloadReady for server1.conn0.child1/netEvent28
[task 2017-10-16T10:53:13.970Z] 10:53:13     INFO - Buffered messages finished
[task 2017-10-16T10:53:13.971Z] 10:53:13     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/request-list-context-menu.js:390 - TypeError: this.getTabTarget is not a function
[task 2017-10-16T10:53:13.971Z] 10:53:13     INFO - Stack trace:
[task 2017-10-16T10:53:13.972Z] 10:53:13     INFO -     getDefaultHarOptions@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/request-list-context-menu.js:390:16
[task 2017-10-16T10:53:13.973Z] 10:53:13     INFO -     copyAllAsHar@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/request-list-context-menu.js:375:29
[task 2017-10-16T10:53:13.974Z] 10:53:13     INFO -     @chrome://mochitests/content/browser/devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js:30:9
[task 2017-10-16T10:53:13.975Z] 10:53:13     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
[task 2017-10-16T10:53:13.975Z] 10:53:13     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
[task 2017-10-16T10:53:13.976Z] 10:53:13     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-10-16T10:53:13.977Z] 10:53:13     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
[task 2017-10-16T10:53:13.977Z] 10:53:13     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
[task 2017-10-16T10:53:13.978Z] 10:53:13     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-10-16T10:53:13.979Z] 10:53:13     INFO - Leaving test bound 
[task 2017-10-16T10:53:13.980Z] 10:53:13     INFO - Removing tab.
[task 2017-10-16T10:53:13.981Z] 10:53:13     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2017-10-16T10:53:13.981Z] 10:53:13     INFO - Got event: 'TabClose' on [object XULElement].
[task 2017-10-16T10:53:13.982Z] 10:53:13     INFO - Tab removed and finished closing

Comment 55

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913638 [details]
Bug 1005755 - Introduce pause/resume button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185032/diff/5-6/

Comment 56

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913639 [details]
Bug 1005755 - Stop using Connector as a singleton;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185034/diff/5-6/

Comment 57

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185036/diff/5-6/

Comment 58

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185038/diff/5-6/

Comment 59

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185582/diff/5-6/

Comment 60

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #54)
> Backed out for failing clipboard's
> devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js:
Ah, sorry about that!

Patch updated, issue fixed. Trying to land again.

Honza

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0ccae3172fc
Introduce pause/resume button; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/819c504f437a
Stop using Connector as a singleton; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/4cc8ddaca5bd
Update tests; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/463a6a947862
Update Net logging in the Console; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f384dd566e7f
Test for pause/stop button; r=nchevobbe

Comment 62

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures in browser_net_har_post_data.js and browser_net_har_post_data_on_get.js: trailing spaces not allowed:

https://hg.mozilla.org/integration/autoland/rev/816ac915cdea753ad78824382035a656d60029f0
https://hg.mozilla.org/integration/autoland/rev/7847d84440530a046d686d6b5b88b273a345fbd3
https://hg.mozilla.org/integration/autoland/rev/9c6453e78967c93d284ce61601ed96f5a05b5a3b
https://hg.mozilla.org/integration/autoland/rev/136ae00ec9380f5d7a8c8f77ba7aa1b7a2e54d88
https://hg.mozilla.org/integration/autoland/rev/5bad7469726ec3de1cd7ff7e6562f0941672cf27

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137225251&repo=autoland
 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/har/test/browser_net_har_post_data.js:20:1 | Trailing spaces not allowed. (no-trailing-spaces)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/har/test/browser_net_har_post_data_on_get.js:20:1 | Trailing spaces not allowed. (no-trailing-spaces)

Comment 63

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913640 [details]
Bug 1005755 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185036/diff/6-7/

Comment 64

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913641 [details]
Bug 1005755 - Update Net logging in the Console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185038/diff/6-7/

Comment 65

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8914263 [details]
Bug 1005755 - Test for pause/stop button;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185582/diff/6-7/

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34e578e0d20e
Introduce pause/resume button; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/c44d05db5e64
Stop using Connector as a singleton; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/00992de316f0
Update tests; r=rickychien
https://hg.mozilla.org/integration/autoland/rev/0d8950ca1882
Update Net logging in the Console; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f22d51b93cf8
Test for pause/stop button; r=nchevobbe

Comment 67

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/34e578e0d20e
https://hg.mozilla.org/mozilla-central/rev/c44d05db5e64
https://hg.mozilla.org/mozilla-central/rev/00992de316f0
https://hg.mozilla.org/mozilla-central/rev/0d8950ca1882
https://hg.mozilla.org/mozilla-central/rev/f22d51b93cf8

Comment 68

[Sebastian Zartner [:sebo]]

Please don't forget to set the dev-doc-needed keyword for new features like this, so they are considered in the documentation.

Sebastian

Comment 69

[Chris Mills [:cmills]]

I've documented this, adding a section here:

https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Pausing_and_resume_network_traffic_recording

And adding a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#Developer_Tools

Let me know if this is OK. I really ought to update all the screenshots to show the new button, but that would be a nightmare and I really don't have the time right now ;-|

Comment 70

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #69)> Let me know if this is OK. I really ought to update all the screenshots to
> show the new button, but that would be a nightmare and I really don't have
> the time right now ;-|

Slightly off-topic, could this be something we automate using headless Firefox ? I guess it would save us quite some time, and we could have ever-green screenshots

Comment 71

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #69)
> Let me know if this is OK. I really ought to update all the screenshots to
> show the new button, but that would be a nightmare and I really don't have
> the time right now ;-|
Looks good to me.
Thanks for the update!
Honza

Comment 72

[Chris Mills [:cmills]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #70)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #69)>
> Let me know if this is OK. I really ought to update all the screenshots to
> > show the new button, but that would be a nightmare and I really don't have
> > the time right now ;-|
> 
> Slightly off-topic, could this be something we automate using headless
> Firefox ? I guess it would save us quite some time, and we could have
> ever-green screenshots

That is a good idea, certainly worth investigating. I will make a note in my to do list.