Closed Bug 1408964 Opened 2 years ago Closed 2 years ago

The Network panel should be auto resumed on reload

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox57 wontfix, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(1 file)

This is a follow up for bug 1005755

Paused network panel should be automatically resumed when the page is reloaded.

Honza
Duplicate of this bug: 1412017
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment on attachment 8924605 [details]
Bug 1408964 - The Network panel should be auto resumed on reload;

https://reviewboard.mozilla.org/r/195830/#review201242

Hey Honza,

This new behavior makes more sense to me. I like that!

Only one major concern about the usage of `this.isPaused` property. Please see below.

::: devtools/client/netmonitor/src/connector/firefox-connector.js:30
(Diff revision 1)
>  
>      // Internals
>      this.getLongString = this.getLongString.bind(this);
>      this.getNetworkRequest = this.getNetworkRequest.bind(this);
> +
> +    this.isPaused = true;

I already have a `state.requests.recording` [1], so it's reusable in this case.

P.S. this.getState seems unsed in firefox-connector. It's time to use it since it's necessary for accessing `state.requests.recording`. 

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/requests.js#75

::: devtools/client/netmonitor/src/connector/firefox-connector.js:51
(Diff revision 1)
> +    // Resume happens on reload/navigation, and so `will-navigate`
> +    // listener needs to be there all the time.

nit: I can understand what's your purpose from the bug title, but a bit confused by this comment. Perhaps we can make the comment more explicit.

For instance:
Paused network panel should be automatically resumed when page reload, so `will-navigate` listener needs to be there all the time.
Attachment #8924605 - Flags: review?(rchien) → review-
(In reply to Ricky Chien [:rickychien] from comment #3)
> Comment on attachment 8924605 [details]
> I already have a `state.requests.recording` [1], so it's reusable in this
> case.
Good point, done.

> nit: I can understand what's your purpose from the bug title, but a bit
> confused by this comment. Perhaps we can make the comment more explicit.
Done

Thanks,
Honza
Comment on attachment 8924605 [details]
Bug 1408964 - The Network panel should be auto resumed on reload;

https://reviewboard.mozilla.org/r/195830/#review201352

Cool! This patch looks great to me. Thank you Honza.
Attachment #8924605 - Flags: review?(rchien) → review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/628b2f8e23b7
The Network panel should be auto resumed on reload; r=rickychien
https://hg.mozilla.org/mozilla-central/rev/628b2f8e23b7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.