Closed Bug 971798 Opened 10 years ago Closed 10 years ago

Do not enable CSS reflow logging when clicking the CSS category

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: msucan, Assigned: thomas)

Details

(Whiteboard: [mentor=msucan][lang=js,xul])

Attachments

(1 file, 1 obsolete file)

It is too easy to enable CSS reflow logging by clicking the CSS category button in the web console. We should not auto-enable CSS > Log when the button is clicked. I also recommend we rename 'log' to 'reflow', since the CSS > Log menu item only does reflow logging.
Maybe it's time to move reflows logging somewhere else. But i'm not sure where.
Whiteboard: [mentor=msucan][lang=js,xul]
Mihai
If there is consensus for fixing this I would love to work on this one.

> I also recommend we rename 'log' to 'reflow'
Should it say 'reflows' (plural) like in Errors and Warnings?

Thomas
(In reply to Thomas Andersen from comment #2)
> Mihai
> If there is consensus for fixing this I would love to work on this one.

Sure! Thanks!

I want this bug fixed, irrespective of where CSS reflow logging will end up, because it's too easy to enable reflow listeners in the console, causing people to complain pages are slow. Until then, a patch to fix this bug is very much welcome.


> > I also recommend we rename 'log' to 'reflow'
> Should it say 'reflows' (plural) like in Errors and Warnings?

Sure, makes sense.
Assignee: nobody → thomas
Status: NEW → ASSIGNED
Priority: -- → P3
Attached patch 971798.patch (obsolete) — Splinter Review
Mihai

This is an early preview patch. Should only fix the ui part of the issue.

I am not sure if the logic should be in WebConsoleFrame._setMenuState, but I thought it was cleaner to have it as early as possible.
Let me know if I miss anything.

I think we should refactor code occurrences of 'csslog' to 'reflows' in webconsole.xul, webconsole.js, test/, browser/app/profile/firefox.js etc. too.
If there are no upgrade scripts this would break the existing user preference after a browser upgrade. I guess the most critical issue regarding this would be add-ons/tools etc. that uses that pref.

And of-course the browser/devtools/webconsole/test/browser_webconsole_bug_601667_filter_buttons.js test should be adjusted in order to pass now.

Thomas
Attachment #8395201 - Flags: review?(mihai.sucan)
Comment on attachment 8395201 [details] [diff] [review]
971798.patch

This is looking good. I dont think we need to update the prefs, severity, etc. Let's keep the patch minimal.

Giving f+. We need the test fixes, then r+.

Thanks for your patch!
Attachment #8395201 - Flags: review?(mihai.sucan) → feedback+
Attached patch 971798.patchSplinter Review
ok, here is a new patch. I tried to make it as simple as possible ;)

All webconsole mochittests passes for me now.

thomas
Attachment #8395201 - Attachment is obsolete: true
Attachment #8396042 - Flags: review?(mihai.sucan)
Comment on attachment 8396042 [details] [diff] [review]
971798.patch

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

Thank you!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c56baf6680e9

We will land the patch once the try results are green.
Attachment #8396042 - Flags: review?(mihai.sucan) → review+
Try push looks good enough.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1746a5b54d12
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=msucan][lang=js,xul] → [mentor=msucan][lang=js,xul][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1746a5b54d12
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=msucan][lang=js,xul][fixed-in-fx-team] → [mentor=msucan][lang=js,xul]
Target Milestone: --- → Firefox 31
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: