622303 - Web Console should remember filter settings.

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[patrickjdempsey]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20101231 Firefox/4.0b9pre

Web Console always opens with all filters turned on.  When testing pages, it would be useful if it remembered the user settings after being closed and opened again. This would make it far more useful as a debugging tool.  It's frustrating to have to sort through the filters every time and I think most people using this tool will be wanting to check the same sets of filters most of the time.

Reproducible: Always

Steps to Reproduce:
1. Open Web Console, all filters on.
2. Set custom filters.
3. Close Web Console.
4. Reopen Web Console, custom filter settings lost, all filters on.

Comment 1

[Kevin Dangoor]

This is a good suggestion. I don't think it likely this will land in Firefox 4, but we can tackle this in a follow up.

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> This is a good suggestion. I don't think it likely this will land in Firefox 4,
> but we can tackle this in a follow up.

This is a regression. We used to store all of the filter settings in the a storage object, i think that bit was forgotten in the ui re-write

Comment 3

[Kevin Dangoor]

It seems plausible that the toolbar change would have changed how (and whether!) filters were saved. cc'ing Patrick for an opinion on that.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

While we wouldn't block on this, a small patch with tests would get my automatic approval, so marking this pre-approved.

Comment 5

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

I think I'd like to get this in for final. taking.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Change of priorities. I won't be working on this in the near future.

Comment 8

[Paul Rouget [:paul]]

Sonny, let me know if you need any help.

Comment 9

[Paul Rouget [:paul]]

I think we just need to store the buttons state in a pref (we already store a couple of prefs in devtools.webconsole.*).

Comment 10

[Sonny Piers [:sonny]]

Attachment: patch v1

Comment 11

[Sonny Piers [:sonny]]

Comment on attachment 574960 [details] [diff] [review]
patch v1

So basically I reused the devtools.hud.display.filter.* prefs which were not used. I added them to the firefox.js pref file and deleted the code supposed to set the default prefs.
Also I've deleted devtools.hud.display.filter.global because it was not used at all.
Since it's my first patch I tried to keep this patch as simple as possible but I think we'll need some code improvements such as renaming the prefs to something like devtools.webconsole.filter.* since it's not only UI related anymore.

Comment 12

[Sonny Piers [:sonny]]

Attachment: patch v1.1

Comment 13

[Sonny Piers [:sonny]]

Comment on attachment 575142 [details] [diff] [review]
patch v1.1

it just fixes a little typo error

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 575142 [details] [diff] [review]
patch v1.1

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

Patch looks great! Thank you very much for your work!

There's only one issue: the new test breaks other tests. Please run the entire set of tests from the folder to see which tests fail.

The problem is that the test doesn't clear the user preferences. You need to call Services.prefs.clearUserPref() for each pref, at the end of the test. You may also need to do this in other tests that change the filter settings because those leave the Web Console in a state that breaks other tests that expect to find all of their messages.

Looking forward for the updated patch. Thank you very much!

More comments below.

::: browser/devtools/webconsole/HUDService.jsm
@@ +1643,5 @@
>    setFilterState: function HS_setFilterState(aHUDId, aToggleType, aState)
>    {
>      this.filterPrefs[aHUDId][aToggleType] = aState;
>      this.adjustVisibilityForMessageType(aHUDId, aToggleType, aState);
> +    Services.prefs.setBoolPref("devtools.hud.display.filter."+aToggleType, aState);

Please add spacing for the + sign. See:

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Also you can use the PREFS_PREFIX constant here.

::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_622303_persistent_filters.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function test() {
> +  let prefService = Services.prefs;
> +  const prefs = ['network', 'networkinfo', 'csserror', 'cssparser', 'exception', 'jswarn', 'error', 'warn', 'info', 'log'];

Coding style: please use double quotes. (through out the entire file)

@@ +17,5 @@
> +    is(hud.HUDBox.querySelector('menuitem[prefKey='+pref+']').getAttribute('checked'), 'true', 'menuitem for ' + pref + ' exists and is checked');
> +  });
> +
> +  prefs.forEach(function(pref) {
> +    hud.HUDBox.querySelector('menuitem[prefKey='+pref+']').click();

Please use EventUtils.synthesizeMouse() to generate the click event.

(grep within the tests folders and you'll find examples)

@@ +22,5 @@
> +  });
> +
> +  gBrowser.removeCurrentTab();
> +
> +  addTab('about:blank');

Do you need to close/reopen the tab? Just closing the Web Console and reopening it should be sufficient for your checks.

Comment 15

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

Comment on attachment 575142 [details] [diff] [review]
patch v1.1

I think we should avoid adding more prefs with 'hud' in the name. I suggest changing the prefs to: devtools.webconsole.filter.* (drop the .display as I don't think it's needed).

Update the test as per Mihai's instructions.

Thanks Sonny!

Comment 16

[Sonny Piers [:sonny]]

Rob, yep, agree that's what I was saying on comment 11.
If you are agree I'll open a new bug for code cleaning/prefs name.

I updated my patch to follow Mihai's instructions but I got troubles with tests, still need to figure out what's wrong.

I'll upload it tomorrow.

Comment 17

[Sonny Piers [:sonny]]

Attachment: patch v1.2

Addressed Mihai's comments.
Added comments in the test.

Comment 18

[Sonny Piers [:sonny]]

BTW I have 25 tests failing (devtools/) but it happens with or without my patch. Probably comes from my configuration.

Comment 19

[Sonny Piers [:sonny]]

Attachment: patch v1.3

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Sonny Piers [:sonny] from comment #18)
> BTW I have 25 tests failing (devtools/) but it happens with or without my
> patch. Probably comes from my configuration.

This may well be bug 670857.

Comment 21

[Sonny Piers [:sonny]]

Panos, yes it looks like. Thanks.

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 575418 [details] [diff] [review]
patch v1.3

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

This is great! Patch is ready to land! Thank you very much for your contributions!

A couple of nits: (1) the pref prefix could be changed here and now - even before we do other cleanups in another patch; (2) the test has some long lines (which go over the 80 chars soft limit).

I will push this patch to the try server, and if results are green, I will land this in fx-team (tomorrow).

Comment 23

[Mihai Sucan [:msucan]]

Try push results will show up here:

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

Comment 24

[Sonny Piers [:sonny]]

Attachment: patch v1.4

Comment 25

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

Attachment: patch v.1.4.1

made some slight tweaks per mihai's recommendations.

I had a test failure when running:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_597460_filter_scroll.js | Test timed out

I don't have time to look at this right now, but but if someone could, I can land tomorrow.

Comment 26

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #25)
> Created attachment 575567 [details] [diff] [review] [diff] [details] [review]
> patch v.1.4.1
> 
> made some slight tweaks per mihai's recommendations.
> 
> I had a test failure when running:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/
> browser_webconsole_bug_597460_filter_scroll.js | Test timed out
> 
> I don't have time to look at this right now, but but if someone could, I can
> land tomorrow.

No problem. Thank you for the update.

The problem:

  HUDService.setFilterState(hudId, "network", true);

hudId is undefined, the setFilterState() call throws. This needs to be:

  HUDService.setFilterState(hud.hudId, "network", true);

Will update the patch.

Comment 27

[Mihai Sucan [:msucan]]

Attachment: patch 1.4.2

Test failure fixed. Please double check. Thank you!

Comment 28

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

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

Comment 29

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

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