Web Console should remember filter settings.

RESOLVED FIXED in Firefox 11

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: pjdkrunkt, Assigned: sonny)

Tracking

({regression})

Trunk
Firefox 11
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [console-1][good first bug][minotaur][fixed-in-fx-team])

Attachments

(3 attachments, 4 obsolete attachments)

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.
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.
Keywords: regression
(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
Status: UNCONFIRMED → NEW
Ever confirmed: true
It seems plausible that the toolbar change would have changed how (and whether!) filters were saved. cc'ing Patrick for an opinion on that.
Blocks: devtools4
While we wouldn't block on this, a small patch with tests would get my automatic approval, so marking this pre-approved.
Whiteboard: [pre-approved by beltzner]
I think I'd like to get this in for final. taking.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Whiteboard: [pre-approved by beltzner] → [pre-approved by beltzner][console-1]
Duplicate of this bug: 642113
Assignee: rcampbell → past
Change of priorities. I won't be working on this in the near future.
Assignee: past → nobody
Status: ASSIGNED → NEW
Whiteboard: [pre-approved by beltzner][console-1] → [console-1][good first bug]
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [console-1][good first bug] → [console-1][good first bug][minotaur]
Version: unspecified → Trunk
Assignee: nobody → getify
Assignee: getify → nobody
Assignee: nobody → sonny.piers
Sonny, let me know if you need any help.
I think we just need to store the buttons state in a pref (we already store a couple of prefs in devtools.webconsole.*).
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #574960 - Flags: review?(mihai.sucan)
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.
Blocks: 703235
Posted patch patch v1.1 (obsolete) — Splinter Review
Attachment #574960 - Attachment is obsolete: true
Attachment #574960 - Flags: review?(mihai.sucan)
Comment on attachment 575142 [details] [diff] [review]
patch v1.1

it just fixes a little typo error
Attachment #575142 - Flags: review?(mihai.sucan)
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.
Attachment #575142 - Flags: review?(mihai.sucan)
Status: NEW → ASSIGNED
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!
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.
Posted patch patch v1.2 (obsolete) — Splinter Review
Addressed Mihai's comments.
Added comments in the test.
Attachment #575142 - Attachment is obsolete: true
Attachment #575416 - Flags: review?(mihai.sucan)
BTW I have 25 tests failing (devtools/) but it happens with or without my patch. Probably comes from my configuration.
Posted patch patch v1.3Splinter Review
Attachment #575416 - Attachment is obsolete: true
Attachment #575418 - Flags: review?(mihai.sucan)
Attachment #575416 - Flags: review?(mihai.sucan)
(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.
Panos, yes it looks like. Thanks.
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).
Attachment #575418 - Flags: review?(mihai.sucan) → review+
Posted patch patch v1.4Splinter Review
Component: Developer Tools → Developer Tools: Console
QA Contact: developer.tools → developer.tools.console
Whiteboard: [console-1][good first bug][minotaur] → [console-1][good first bug][minotaur][land-in-fx-team]
Posted patch patch v.1.4.1 (obsolete) — Splinter Review
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.
(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.
Posted patch patch 1.4.2Splinter Review
Test failure fixed. Please double check. Thank you!
Attachment #575567 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/eb101044cb01
Whiteboard: [console-1][good first bug][minotaur][land-in-fx-team] → [console-1][good first bug][minotaur][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/eb101044cb01
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.