602572 - Log request and response bodies checkbox issues in Web Console

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

STR:

1. Open the Web Console.
2. Enable log request and response bodies (right-click).
3. Close the Web Console.
4. Reopen the Web Console.
5. Refresh the web page.
6. Click a network request log.

Expected result: the response body is not logged.

Actual result: the response body is logged. The menitem falsely shows the option  is disabled checkbox.

STR 2:

1. Open tabA and the Web Console inside it.
2. Open tabB and the Web Console inside it.
3. Enable logging of request and response bodies in tabA.
4. Refresh the Web Console inside tabB.
5. Click a network request log.

Expected result: the network response body is not logged.

Actual result: the network response body is logged.

Comment 1

[Kevin Dangoor]

I can see where this behavior comes from, but I agree that it's inconsistent and confusing. At the very least, the checkbox should show the true state of request/response body logging.

Also, if request/response body logging is tracked on a per site basis, we should turn it off when you close the console. Otherwise, people might have their memory consumed without knowing why (and the only way to stop it is to restart the browser or reopen the console on the right site and disable the logging...)

Comment 2

[Kevin Dangoor]

OK, looking at the patch in bug 598851, it appears that the logging is not tracked per site. So, we really just need to make sure that the check mark in the menu is always consistent.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 484652 [details] [diff] [review]
proposed patch

I don't see a corresponding menuPopup.removeEventListener in here. Do we do proper destruction of all these xul elements when we shutdown?

Comment 5

[Mihai Sucan [:msucan]]

(In reply to comment #4)
> Comment on attachment 484652 [details] [diff] [review]
> proposed patch
> 
> I don't see a corresponding menuPopup.removeEventListener in here. Do we do
> proper destruction of all these xul elements when we shutdown?

Yes, we do that in unregisterDisplay().

Comment 6

[Kevin Dangoor]

Requesting blocking for this bug. The "Log request and response bodies" checkbox in the context menu does not always show the true state of that logging, which would certainly be very confusing for the user.

Comment 7

[Dietrich Ayala (:dietrich)]

Blocking+ for broken UI.

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 484652 [details] [diff] [review]
proposed patch

yup

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. Rebased and made small fixes needed.

Thanks Robert for the feedback+!

Comment 10

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 11

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 491202 [details] [diff] [review]
updated patch

Normally this could be done with XUL observers, but I guess that's kind of awkward here since the DOM's constructed via JS.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch. Thanks for the review+!

Comment 14

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/1494caa22150