Closed
Bug 1064458
Opened 10 years ago
Closed 9 years ago
Remove 'Log request and response bodies' preference
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: bgrins, Assigned: linclark)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
STR:
Open Console
Right click in output and check "Log Request and Response Bodies"
Close and reopen toolbox
Right click in console output
Expected:
"Log Request and Response Bodies" is already checked
Actual:
"Log Request and Response Bodies" is unchecked
This should be stored as a user preference and possibly also shown in the options panel.
I too would prefer it to save this state when closing and re-opening the developer console, or better yet make it a setting in about:config that toggles whenever you enable/disable it from the developer console (thereby making the tickmark permanently set or unset).
Back when the option was always enabled I never noticed any issues with RAM usage or anything, which may be because I have a fairly high-end laptop. Being able to permanently enable it would be a convenience for me, it was really annoying when it first started to not keep request/response bodies by default. Can't count the number of times I've had to redo an action because it was, again, not logging the request/response bodies. By now I'm more or less used to doing this extra step every time, but I'd still like a permanent tickmark setting.
Reporter | ||
Comment 2•10 years ago
|
||
Looks like maybe there could be a pref for the frontend that calls setSaveRequestAndResponseBodies(true) once the webConsoleClient is attached if it is true. Then we can also just toggle that pref anytime the option is changed (in reverseSaveBodiesPref).
Reporter | ||
Comment 3•10 years ago
|
||
(no tests, just a WIP). Matt, can you check and see if this does what you need it to?
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(MattN+bmo)
Comment 4•10 years ago
|
||
Comment on attachment 8517040 [details] [diff] [review]
webconsole-save-request-response-WIP.patch
Review of attachment 8517040 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/devtools/webconsole/webconsole.js
@@ +477,5 @@
> }).then(() => {
> let id = WebConsoleUtils.supportsString(this.hudId);
> +
> + if (Services.prefs.getBoolPref("devtools.webconsole.filter.network.bodies")) {
> + this.setSaveRequestAndResponseBodies(true);
This doesn't seem to work for the Browser Console.
Updated•10 years ago
|
Flags: needinfo?(MattN+bmo)
Reporter | ||
Comment 5•9 years ago
|
||
Whiteboard: [firebug-gap]
Reporter | ||
Updated•9 years ago
|
Blocks: firebug-gaps
Whiteboard: [firebug-gap]
Comment 6•9 years ago
|
||
Brian, would you have time to work on this?
Florent
Comment 7•9 years ago
|
||
This is very annoying, I use for work the FFDev Edition and this is a flag that can be enabled on that release as default nad in the other version disabled for the problem explained on the comment #1.
Reporter | ||
Comment 8•9 years ago
|
||
I believe this isn't needed anymore since clicking on requests now goes to the netmonitor, so we can remove the pref and the UI for controlling it
Summary: Log request and response bodies preference is not remembered between sessions → Remove 'Log request and response bodies' preference
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lclark
Assignee | ||
Comment 9•9 years ago
|
||
This is WIP (to run against try).
Assignee | ||
Comment 10•9 years ago
|
||
The patch above just removed the option from the UI. This leaves a lot of cruft... for example, the preference is sent between client and actor.
After a discussion with bgrins, this is the current plan:
- Expose the pref in about:config
- Currently, the pref is sent between client and server using WebConsoleClient.setPreference. Instead, just have the actor access the pref directly.
- In our code, WebConsoleClient.setPreference is only used for this preference. It should be removed. However, because there is a small chance addons use WebConsoleClient.setPreference to manage their own preferences, we won't remove it in this issue. It will either be deprecated or we'll remove it after discussing on the mailing list.
Comment 11•9 years ago
|
||
Just to note that there is bug 1211525, that should allow to expand HTTP events in the Console panel (and even in the Browser Console) - in order to see the details (including the response body). Not sure how that matches the plan here.
Honza
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> Just to note that there is bug 1211525, that should allow to expand HTTP
> events in the Console panel (and even in the Browser Console) - in order to
> see the details (including the response body). Not sure how that matches the
> plan here.
For that case we will need to make sure that saveRequestAndResponseBodies is true by default (and not only when the netmonitor panel is opened), which fits in perfectly with the plan listed in Comment 10.
I thought over this plan some more and realized that by changing the default to true we would be storing bodies for the Browser Console which is wasteful since there's no way to view them from there (and we also are logging all requests that happen for the entire browser). So at least for now I think we need to keep the setPreferences machinery around and set it to false from the client side in the Browser Console case. Probably once the connection is initialized if this.owner._browserConsole around here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#5096. If we later add the ability to view response bodies from the browser console directly (bug 1211525) then we can revisit this part.
Assignee | ||
Comment 13•9 years ago
|
||
It turns out that this.owner._browserConsole doesn't exist in WebConsoleConnectionProxy._onAttachConsole even when it is opened by the Browser Console.
The function signature for the constructor is WebConsoleConnectionProxy(aWebConsole, aTarget). The comment says that the first param should be a WebConsole instance. However, when it is created, the parameter passed in for aWebConsole is a WebConsoleFrame instance.
https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#494
This WebConsoleFrame instance is then assigned to this.owner.
There seems to be a mismatch between the expectation and what the code is doing. Is this possibly technical debt?
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Lin Clark from comment #13)
> The function signature for the constructor is
> WebConsoleConnectionProxy(aWebConsole, aTarget). The comment says that the
> first param should be a WebConsole instance. However, when it is created,
> the parameter passed in for aWebConsole is a WebConsoleFrame instance.
>
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> webconsole.js#494
>
> This WebConsoleFrame instance is then assigned to this.owner.
>
> There seems to be a mismatch between the expectation and what the code is
> doing. Is this possibly technical debt?
For future reference, we discussed this on irc and figured out that the header comment / arg name for WebConsoleConnectionProxy is wrong
Assignee | ||
Comment 15•9 years ago
|
||
Here's a WIP patch. I am currently debugging the test that it adds.
Attachment #8517040 -
Attachment is obsolete: true
Attachment #8694421 -
Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 16•9 years ago
|
||
This patch sets netlogging to true by default. If the console is a browser console, it turns off netlogging.
It adds a test for the browser console case. There is another issue to transition the test to add_task(), Bug #1234287.
Attachment #8700685 -
Attachment is obsolete: true
Attachment #8700707 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 17•9 years ago
|
||
Comment on attachment 8700707 [details] [diff] [review]
Bug1064458.patch
Review of attachment 8700707 [details] [diff] [review]:
-----------------------------------------------------------------
First pass - it looks good to me. I'll take another look at it tomorrow.
::: devtools/client/netmonitor/netmonitor-controller.js
@@ +222,5 @@
> this.tabClient = this._target.activeTab;
> }
>
> let connectWebConsole = () => {
> let deferred = promise.defer();
The 3 lines dealing with `deferred` in this function can be removed so this can be a one-liner (I guess may as well leave it as a named function since that helps explain what it's doing)
::: devtools/client/webconsole/webconsole.js
@@ +366,5 @@
> },
>
> _destroyer: null,
>
> // Used in tests.
This comment isn't really accurate anymore - no tests use this, it's just used to keep some state that isn't queried anywhere.
@@ +882,4 @@
> let prefKey = target.getAttribute("prefKey");
> this.setFilterState(prefKey, state);
>
> // Disable the log response and request body if network logging is off.
Looks like this entire if block can be removed
@@ +5039,5 @@
> + // There is no way to view response bodies from the Browser Console, so do
> + // not waste the memory.
> + let saveBodies = !this.webConsoleFrame.owner._browserConsole;
> + this.webConsoleFrame.setSaveRequestAndResponseBodies(saveBodies).then(() => {
> + this.webConsoleClient.on("networkEvent", this._onNetworkEvent);
Does everything here need to happen inside the resolution of setSaveRequestAndResponseBodies? It looks independent to me, like it could all happen before the call (I could be missing something here).
Attachment #8700707 -
Flags: review?(bgrinstead) → feedback+
Assignee | ||
Comment 18•9 years ago
|
||
Thanks for the review and for catching all of that. After stepping through it more, I'm pretty sure that you're correct about not needing the promise chain after setSaveRequestAndResponseBodies.
This patch fixes everything from the previous review.
Attachment #8700707 -
Attachment is obsolete: true
Attachment #8700905 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 19•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f7d7d3a638
Including a talos push to see if this regresses perf: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=88f7d7d3a638
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 20•9 years ago
|
||
Comment on attachment 8700905 [details] [diff] [review]
Bug1064458.patch
Review of attachment 8700905 [details] [diff] [review]:
-----------------------------------------------------------------
This all looks good to me. Let's see what talos says once that build finishes (hopefully not much)
::: devtools/client/webconsole/test/browser_console_netlogging.js
@@ +6,5 @@
> +// Tests that network log messages bring up the network panel.
> +
> +"use strict";
> +
> +const TEST_URI = "data:text/html;charset=utf-8,Web Console network " +
Were you planning to update this to use add_task as a separate patch on this bug or a follow up bug?
Attachment #8700905 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Thanks for catching that comment, I'll update it.
I'm going to switch to add_task in Bug #1234287.
Reporter | ||
Comment 22•9 years ago
|
||
There is a failure in devtools/shared/webconsole/test/test_network_get.html ('oth' in https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f7d7d3a638)
Assignee | ||
Comment 23•9 years ago
|
||
I fixed the failures. But I think I caught one that passed which should fail.
This is what it is currently, which passes:
> ok(!aResponse.postData.text, "no request POST data");
But I think the NOT should be removed. When I do remove the NOT, then it fails because there is no postData.
I don't think this signals a bug in the code. The browser_webconsole_netlogging.js test checks postData and passes. I think it might be a flaw in the test.
Brian, do you have any ideas?
Flags: needinfo?(bgrinstead)
Reporter | ||
Comment 24•9 years ago
|
||
(In reply to Lin Clark from comment #23)
> Created attachment 8704868 [details] [diff] [review]
> Bug1064458.patch
>
> I fixed the failures. But I think I caught one that passed which should fail.
>
>
> This is what it is currently, which passes:
>
> > ok(!aResponse.postData.text, "no request POST data");
>
> But I think the NOT should be removed. When I do remove the NOT, then it
> fails because there is no postData.
>
> I don't think this signals a bug in the code. The
> browser_webconsole_netlogging.js test checks postData and passes. I think it
> might be a flaw in the test.
>
> Brian, do you have any ideas?
The fact that the changed !aResponse.postDataDiscarded is working makes me think that there isn't actually any POST data (so aResponse.postData.text is false). I believe that's expected, since this test is checking GET requests. So I think that the test should assert:
ok(!aResponse.postData.text, "no request POST data");
ok(!aResponse.postDataDiscarded, "request POST data was not discarded");
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 25•9 years ago
|
||
Good call. You're right, I wasn't looking at the method. I was just looking at the .postDataDiscarded flag. It shouldn't have POST data on a GET. We test for that in the other test too.
The test passes with this patch.
Attachment #8700905 -
Attachment is obsolete: true
Attachment #8704868 -
Attachment is obsolete: true
Attachment #8705665 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 26•9 years ago
|
||
Comment on attachment 8705665 [details] [diff] [review]
Bug1064458.patch
Review of attachment 8705665 [details] [diff] [review]:
-----------------------------------------------------------------
Alright, looks good to me
::: devtools/client/netmonitor/netmonitor-controller.js
@@ +226,2 @@
> this.webConsoleClient = this._target.activeConsole;
> + return promise.defer().resolve();
I think we don't need to return anything here.. or if we do then promise.resolve() should work fine
Attachment #8705665 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 27•9 years ago
|
||
Thanks for the review! You're right about that return. I removed it.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68aade8426c6
Attachment #8705665 -
Attachment is obsolete: true
Reporter | ||
Comment 28•9 years ago
|
||
(In reply to Lin Clark from comment #27)
> Created attachment 8706720 [details] [diff] [review]
> Bug1064458.patch
>
> Thanks for the review! You're right about that return. I removed it.
>
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=68aade8426c6
I have one last talos push: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=5acc2a44834c&newProject=try&newRevision=8f3fe80cfd00&framework=1&showOnlyImportant=0. Looks like a minor regression on linux (small enough to not worry about it), let's see how Windows pans out.
Reporter | ||
Comment 29•9 years ago
|
||
DAMP from the try push looks fine - I'd say this is good to land
Comment 31•9 years ago
|
||
Keywords: checkin-needed
Comment 32•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Reporter | ||
Updated•9 years ago
|
Comment 33•9 years ago
|
||
[bugday-20160323]
Status: RESOLVED,FIXED --> VERIFIED
Component:
Name Firefox
Version 46.0b4
Build ID 20160322075646
Update Channel beta
User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS Windows 7 SP1 x86_64
Actual Results:
As expected
Expected Results:
No option is there like "log request and response bodies" while right click in output area.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•