Web Console should remember filter settings.

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: patrickjdempsey, Assigned: sonny)

Tracking

({regression})

Trunk
Firefox 11
regression
Points:
---
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)

(Reporter)

Description

7 years ago
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

7 years ago
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.

Updated

7 years ago
Keywords: regression

Comment 2

7 years ago
(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

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

7 years ago
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: 593956
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

Updated

6 years ago
Whiteboard: [pre-approved by beltzner] → [pre-approved by beltzner][console-1]

Updated

6 years ago
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

Updated

6 years ago
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

Updated

6 years ago
Assignee: nobody → getify

Updated

6 years ago
Assignee: getify → nobody

Updated

6 years ago
Assignee: nobody → sonny.piers

Comment 8

6 years ago
Sonny, let me know if you need any help.

Comment 9

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

Comment 10

6 years ago
Created attachment 574960 [details] [diff] [review]
patch v1
Attachment #574960 - Flags: review?(mihai.sucan)
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 703235
(Assignee)

Comment 12

6 years ago
Created attachment 575142 [details] [diff] [review]
patch v1.1
Attachment #574960 - Attachment is obsolete: true
Attachment #574960 - Flags: review?(mihai.sucan)
(Assignee)

Comment 13

6 years ago
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)

Updated

6 years ago
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!
(Assignee)

Comment 16

6 years ago
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.
(Assignee)

Comment 17

6 years ago
Created attachment 575416 [details] [diff] [review]
patch v1.2

Addressed Mihai's comments.
Added comments in the test.
Attachment #575142 - Attachment is obsolete: true
Attachment #575416 - Flags: review?(mihai.sucan)
(Assignee)

Comment 18

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

Comment 19

6 years ago
Created attachment 575418 [details] [diff] [review]
patch v1.3
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.
(Assignee)

Comment 21

6 years ago
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+
Try push results will show up here:

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

Comment 24

6 years ago
Created attachment 575505 [details] [diff] [review]
patch v1.4

Updated

6 years ago
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]
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.
(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.
Created attachment 575635 [details] [diff] [review]
patch 1.4.2

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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
You need to log in before you can comment on or make changes to this bug.