925275 - Make Network Monitor log requests persistent

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect)

Description

[Aissen]

Every time a new webpage is loaded (manually or through javascript), the network monitor log is cleared, which is annoying to debug a succession of webpages.

In firebug, there's a "record" button to make those persistent.

In the Firefox devtools, there's an option Web Console -> Enable persistent logs. The Network Monitor should probably honor this preference.

Comment 1

[Victor Porof [:vporof][:vp]]

Good idea!

Comment 3

[Chris]

I'd be interested in tackling this as a way to learn more about how the network monitor works.

Comment 4

[Victor Porof [:vporof][:vp]]

Hi Chris! Thanks for the interest :D Let me know if you need help with building Firefox from source [0] and running browser chrome mochitests [1].

The Network Monitor's code lives here: [2]. The tool is currently clearing its views every time a page navigation starts happening. This is done in the controller, here: [3].

This bug is about respecting the "Enable persistent logs" pref (devtools.webconsole.persistlog), used only by the WebConsole for now. It would be nice if the Network Monitor would also honor this pref.

Pref management in the Network Monitor is also done in the controller here: [4], using our shared helper [5]. You can use that to access the pref.

Afterwards, a test should be added, here: [5]. Browse through the existing tests to see how things work; you can duplicate an existing test or start writing one from scratch. Here's a very small template: [7].

Ideally, the entry in the Options panel should be changed as well, since the WebConsole won't be the only tool using the pref anymore. But I don't have any good ideas of how to structure it. What do you think we should do about that?

[0] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[1] https://developer.mozilla.org/en/docs/Browser_chrome_tests
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor
[3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-controller.js#309
[4] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-controller.js#586
[5] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#354
[6] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test
[7] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/browser_net_aaa_leaktest.js

Comment 5

[Chris]

Thanks for the info, I'll start looking into it.  I think the ideal solution would be to have two separate checkboxes under two separate headings, each of which would enable persistent logs for their own respective controls.  But that sounds more like a feature that could be requested later.  What I would like to do is to change Web Console to something like "Common Settings" or "Log Preferences" and possibly move it to be in between the JavaScript Profiler heading and the Advanced Settings heading.

Comment 6

[Victor Porof [:vporof][:vp]]

Sounds good to me!

Comment 7

[Chris]

I'm able to get the tests specific to the network monitor to pass, but I'm having problems getting the chrome test suite to pass.  There's about 300 tests that are failing and I know that 4 of them are related to fonts and there are a large number that are failing due to the datepicker popup not getting the right field.
Some of them are failing due to menu movement.
I'm currently building with out any options by simply using ./mach build and my development environment is Ubuntu running in Virtual Box.  I'll look on the introduction IRC to see if I can get some help with the tests failing.

Comment 8

[Victor Porof [:vporof][:vp]]

Attach a WIP patch and I'll have a look! The other failing tests might have nothing to do with your change.

Comment 9

[Chris]

They are failing regardless of whether or not any changes have been applied so I'm inclined to believe that there's an issue with my development environment.  However none of tests in the network monitor test are failing so I'm going to go ahead with development.

Comment 10

[Victor Porof [:vporof][:vp]]

(In reply to Chris from comment #9)

Yes! That's a better plan :)

At most, just run the developer tool tests once you've finished. We'll push to the try servers at that point to verify that nothing else breaks globally.

Comment 11

[Chris]

Attachment: bug-925275-WIP1.patch

Uploading this WIP patch, I will finish off the TODOs in the next two to three days when I sit down to work on it again.

Comment 12

[Chris]

Attachment: bug-925275-WIP2.patch

Everything is finished except for the unit tests.

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 8346891 [details] [diff] [review]
bug-925275-WIP2.patch

Review of attachment 8346891 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good so far!
Make sure you don't leave trailing whitespace in the edited files.

Comment 14

[Chris]

Thanks for the feedback.  I'll go back and double check for trailing whitespace.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Chris, are you still working on this bug?  It's fine if you are! :) Just double checking, in case others may want to give it a shot.

Comment 16

[Chris]

Sorry for taking so long, I've been bogged down with work.  I'll try to get around to finish it this weekend.  After that if I still don't get to it then if someone else wants to take over the only thing left is to just write the test case and fix that one extra trailing whitespace that I put in by accident in /browser/locales/en-US/chrome/browser/devtools/toolbox.dtd

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, no worries, keep working on it for sure!  Just wanted to double check. :)

Comment 18

[Chris]

The way I was thinking about going about doing this is to:
1: turn on the preference (Or perform some equivalent action)
2: Log some data by loading a sample web page.
3: Navigate to a separate sample web page.
4: Check the log to make sure that the data is still there (I'll probably just check for a string that should appear in the logs from the old webpage but not the new one.)

Right now I'm just manually turning on the preference, is there some other way I should be going about this?  I've noticed that none of the other tests seem to be actually setting the preferences outside of head.js.

Please let me know if there's anything else I should be testing with this.

Comment 19

[Victor Porof [:vporof][:vp]]

(In reply to Chris from comment #18)

Don't feel rushed into fixing this :) Thanks for still being interested.

> The way I was thinking about going about doing this is to:
> 1: turn on the preference (Or perform some equivalent action)
> 2: Log some data by loading a sample web page.
> 3: Navigate to a separate sample web page.
> 4: Check the log to make sure that the data is still there (I'll probably
> just check for a string that should appear in the logs from the old webpage
> but not the new one.)
> 
> Right now I'm just manually turning on the preference, is there some other
> way I should be going about this?  I've noticed that none of the other tests
> seem to be actually setting the preferences outside of head.js.
> 
> Please let me know if there's anything else I should be testing with this.

Manual testing is good, but to land this new feature we need an automated test. Basically, all the steps described above should be automated in a mochitest. See the other netmonitor tests for some inspiration; you can clone one of them and modify it to do what you wrote above.

Comment 20

[Manish Goregaokar [:manishearth]]

There may be some merit to making this a per-panel pref instead of a global pref. Chrome does this, for example.


If I get time I may be able to help with the testcase (I'm new to writing tests as well).

Comment 21

[Chris]

I'm going to take another look at it this weekend to see if I can get a working test case.  Is it okay to make this a per-panel preference as a part of this bug or should that be done as apart of another bug?

Comment 22

[Manish Goregaokar [:manishearth]]

I would say that it's best to continue with your current patch, and we can look into making a separate panelwise button later. (I'm not your mentor on this, though, you may wish to wait for him)

I think you can go ahead with the testcase though, if you do switch the functionality or add new functionality it would be easy to extend the textcase to work.

Comment 23

[Victor Porof [:vporof][:vp]]

(In reply to Chris from comment #21)
> I'm going to take another look at it this weekend to see if I can get a
> working test case.  Is it okay to make this a per-panel preference as a part
> of this bug or should that be done as apart of another bug?

Either way is fine by me. Best thing is to hack something that's landable, and file a followup bug if there's anything else that needs fixing.

Comment 24

[Chris]

Manish you can take over for this if you'd like.

Comment 25

[Manish Goregaokar [:manishearth]]

Extremely busy at the moment, got a lot of coursework. Will look at later if you've not finished it.

Comment 26

[Christos Stathis]

Attachment: 925275.patch

Hello,

I 've managed to put a test together and updated the patch.

Comment 27

[Victor Porof [:vporof][:vp]]

Comment on attachment 8388219 [details] [diff] [review]
925275.patch

Review of attachment 8388219 [details] [diff] [review]:
-----------------------------------------------------------------

Woooo! This is awesome! Thank you Christos! I just want you to fix a few small nits, and this should be good to go.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5e6773371123

r+ with the changes below and a green try run.

::: browser/devtools/framework/toolbox-options.xul
@@ +75,5 @@
>            <checkbox label="&options.showPlatformData.label;"
>                      tooltiptext="&options.showPlatformData.tooltip;"
>                      data-pref="devtools.profiler.ui.show-platform-data"/>
>          </vbox>
> +        <label value="&options.commonPrefs.label;"/>

How about making the common prefs appear immediately after the "Theme" option, instead of here? Would that take a lot of work or is it straightforward. If it's easy, do the change here please.

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +417,5 @@
>    _onTabNavigated: function(aType, aPacket) {
>      switch (aType) {
>        case "will-navigate": {
>          // Reset UI.
> +        if (!Services.prefs.getBoolPref("devtools.webconsole.persistlog")) {

Please use a Prefs instance to interact with preferences [0] in this file.
Place the following below [0]:

let WebConsolePrefs = new ViewHelpers.Prefs("devtools.webconsole", {
  persistLog: ["Bool", "persistlog"],
});

and use it here:

if (!WebConsolePrefs.persistLog) {
  ...
}

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-controller.js#732

@@ +424,4 @@
>  
> +          // Switch to the default network traffic inspector view.
> +          if (NetMonitorController.getCurrentActivity() == ACTIVITY_TYPE.NONE) {
> +            NetMonitorView.showNetworkInspectorView();

This stuff should be outside the above conditional. Switching to the network inspector view should always happen on refresh.

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
@@ +144,5 @@
> +<!ENTITY options.commonPrefs.label           "Common Preferences">
> +
> +<!-- LOCALIZATION NOTE (options.enablePersistentLogging.label): This is the
> +  -  label for the checkbox that toggles persistent logs in the Web Console and
> +  -  network monitor,  i.e. devtools.webconsole.persistlog a boolean preference in 

Nit: please remove the whitespace here.

Comment 28

[Christos Stathis]

> ::: browser/devtools/netmonitor/netmonitor-controller.js
> @@ +417,5 @@
> >    _onTabNavigated: function(aType, aPacket) {
> >      switch (aType) {
> >        case "will-navigate": {
> >          // Reset UI.
> > +        if (!Services.prefs.getBoolPref("devtools.webconsole.persistlog")) {
> 
> Please use a Prefs instance to interact with preferences [0] in this file.
> Place the following below [0]:
> 
> let WebConsolePrefs = new ViewHelpers.Prefs("devtools.webconsole", {
>   persistLog: ["Bool", "persistlog"],
> });
> 
> and use it here:
> 
> if (!WebConsolePrefs.persistLog) {
>   ...
> }
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/
> netmonitor-controller.js#732

After this change, the WebConsolePrefs.persistLog always remember the initial value it had when the browser is loaded, ignoring any change afterwards. I don't know why this is happening. It makes the test fail. I am attaching the patch for investigation.

Comment 29

[Christos Stathis]

Attachment: 925275.patch

Comment 30

[Victor Porof [:vporof][:vp]]

Comment on attachment 8391852 [details] [diff] [review]
925275.patch

Review of attachment 8391852 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, you're right. I forgot about that. I seems premature to make Prefs handle updated preferences, so go ahead and keep using Services.prefs for now :)

Comment 31

[Chris]

Thanks for writing up the test case for this Christos, It'll be nice to see this finally getting added.

Comment 32

[Christos Stathis]

Attachment: 925275.patch

Reverted back to version using Services.prefs for accessing preferences.

Comment 33

[Victor Porof [:vporof][:vp]]

Comment on attachment 8395347 [details] [diff] [review]
925275.patch

Review of attachment 8395347 [details] [diff] [review]:
-----------------------------------------------------------------

Alright! Thank you! I'll push to try, and if everything is green, please add a checkin-needed keyword.

Comment 34

[Victor Porof [:vporof][:vp]]

https://tbpl.mozilla.org/?tree=Try&rev=d08a4e5395ad

Comment 35

[Victor Porof [:vporof][:vp]]

Chris, this seems to be failing, so we can't land it just yet. Can you please take a look at this?

Comment 36

[Chris]

I'll run it through the unit tests on my box and see what's failing.

Comment 37

[Chris]

Nevermind, I just saw which test was failing, I'll take a look and see if I can fix it.

Comment 38

[Chris]

Attachment: Turned Persistent preference off before exiting browser_net_persistent_logs test

It looks like persistent prefs just needed to be turned off prior to exiting the new test case, all netmonitor tests are passing on my box as of now.

Comment 39

[Chris]

Attachment: 925275_fix.patch

Turned off persistent logs before exiting browser_net_persistent_logs test.

Comment 40

[Victor Porof [:vporof][:vp]]

Thanks!

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/aaeff6d3bb3b

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aaeff6d3bb3b

Comment 43

[Oana Botisan, Desktop QA]

Starting with 2017-09-01, the Log Persistence checkbox was implemented in Firefox 57, so we can consider this bug fixed and verified across platforms.