Last Comment Bug 622303 - Web Console should remember filter settings.
: Web Console should remember filter settings.
Status: RESOLVED FIXED
[console-1][good first bug][minotaur]...
: regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 11
Assigned To: Sonny Piers [:sonny]
:
: Brian Grinstead [:bgrins]
Mentors:
: 642113 (view as bug list)
Depends on:
Blocks: devtools4 703235
  Show dependency treegraph
 
Reported: 2010-12-31 15:50 PST by patrickjdempsey
Modified: 2011-11-24 08:19 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (10.08 KB, patch)
2011-11-16 12:17 PST, Sonny Piers [:sonny]
no flags Details | Diff | Splinter Review
patch v1.1 (10.06 KB, patch)
2011-11-17 04:12 PST, Sonny Piers [:sonny]
no flags Details | Diff | Splinter Review
patch v1.2 (10.33 KB, patch)
2011-11-18 03:16 PST, Sonny Piers [:sonny]
no flags Details | Diff | Splinter Review
patch v1.3 (10.31 KB, patch)
2011-11-18 03:27 PST, Sonny Piers [:sonny]
mihai.sucan: review+
Details | Diff | Splinter Review
patch v1.4 (10.47 KB, patch)
2011-11-18 10:27 PST, Sonny Piers [:sonny]
no flags Details | Diff | Splinter Review
patch v.1.4.1 (12.31 KB, patch)
2011-11-18 14:32 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
patch 1.4.2 (12.57 KB, patch)
2011-11-19 01:27 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description patrickjdempsey 2010-12-31 15:50:08 PST
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 2011-01-02 20:44:25 PST
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 2011-01-03 08:59:01 PST
(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 2011-01-04 19:48:37 PST
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] 2011-02-26 10:13:30 PST
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) 2011-02-26 10:17:18 PST
I think I'd like to get this in for final. taking.
Comment 6 David Dahl :ddahl 2011-03-16 06:59:22 PDT
*** Bug 642113 has been marked as a duplicate of this bug. ***
Comment 7 Panos Astithas [:past] 2011-06-16 04:09:41 PDT
Change of priorities. I won't be working on this in the near future.
Comment 8 Paul Rouget [:paul] 2011-10-28 13:56:54 PDT
Sonny, let me know if you need any help.
Comment 9 Paul Rouget [:paul] 2011-10-28 20:45:19 PDT
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] 2011-11-16 12:17:33 PST
Created attachment 574960 [details] [diff] [review]
patch v1
Comment 11 Sonny Piers [:sonny] 2011-11-17 03:05:24 PST
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] 2011-11-17 04:12:03 PST
Created attachment 575142 [details] [diff] [review]
patch v1.1
Comment 13 Sonny Piers [:sonny] 2011-11-17 04:37:31 PST
Comment on attachment 575142 [details] [diff] [review]
patch v1.1

it just fixes a little typo error
Comment 14 Mihai Sucan [:msucan] 2011-11-17 08:26:00 PST
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) 2011-11-17 15:16:04 PST
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] 2011-11-17 17:15:59 PST
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] 2011-11-18 03:16:12 PST
Created attachment 575416 [details] [diff] [review]
patch v1.2

Addressed Mihai's comments.
Added comments in the test.
Comment 18 Sonny Piers [:sonny] 2011-11-18 03:22:46 PST
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] 2011-11-18 03:27:37 PST
Created attachment 575418 [details] [diff] [review]
patch v1.3
Comment 20 Panos Astithas [:past] 2011-11-18 03:39:54 PST
(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] 2011-11-18 06:08:38 PST
Panos, yes it looks like. Thanks.
Comment 22 Mihai Sucan [:msucan] 2011-11-18 08:44:39 PST
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] 2011-11-18 09:14:04 PST
Try push results will show up here:

https://tbpl.mozilla.org/?tree=Try&rev=8b92d9c06a02
Comment 24 Sonny Piers [:sonny] 2011-11-18 10:27:54 PST
Created attachment 575505 [details] [diff] [review]
patch v1.4
Comment 25 Rob Campbell [:rc] (:robcee) 2011-11-18 14:32:56 PST
Created attachment 575567 [details] [diff] [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.
Comment 26 Mihai Sucan [:msucan] 2011-11-19 01:24:39 PST
(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] 2011-11-19 01:27:13 PST
Created attachment 575635 [details] [diff] [review]
patch 1.4.2

Test failure fixed. Please double check. Thank you!
Comment 28 Rob Campbell [:rc] (:robcee) 2011-11-23 04:50:46 PST
https://hg.mozilla.org/integration/fx-team/rev/eb101044cb01
Comment 29 Rob Campbell [:rc] (:robcee) 2011-11-24 08:19:42 PST
https://hg.mozilla.org/mozilla-central/rev/eb101044cb01

Note You need to log in before you can comment on or make changes to this bug.