Comment on attachment 339595 [details] [diff] [review] patch, rev1 This patch only works together with the patch in bug 456216.

I'll request review when I update the patch for bug 398928.

Comment on attachment 344109 [details] [diff] [review] v4 New patch coming soon.

I removed two unnecessary !importants. The only ones left are "color" rules on toolbarbuttons; these are necessary because toolbarbutton.css specifies color: buttontext !important. I don't know why it does that but I don't want to change it in this bug.

Comment on attachment 350577 [details] [diff] [review] v5.1 It's not obvious what "segm" means... please rename. > <!DOCTYPE window [ > <!ENTITY % pageInfoDTD SYSTEM "chrome://browser/locale/pageInfo.dtd"> > %pageInfoDTD; >+ <!ENTITY % global SYSTEM "chrome://global/locale/global.dtd"> %global; You should follow the prevailing style and put %global; on the next line. >+#console-toolbox[chromedir="ltr"] #Console\:modeAll, >+#console-toolbox[chromedir="rtl"] #Console\:modeMessages { >+ border-width: 0 4px 0 6px; >+ -moz-border-image: url("chrome://global/skin/toolbar/segm-left.png") 0 4 0 6 repeat; > } The chromedir attributes should be on the buttons. >+#ToolbarMode > #Console\:clear { Why "#ToolbarMode >"? >+++ b/toolkit/themes/pinstripe/global/viewbuttons.css >+window:not([active]) #viewGroup > radio { Use :root:not([active]) so that this works for arbitrary root elements?

(In reply to comment #9) > (From update of attachment 350577 [details] [diff] [review]) > It's not obvious what "segm" means... please rename. > You should follow the prevailing style and put %global; on the next line. > The chromedir attributes should be on the buttons. > Use :root:not([active]) so that this works for arbitrary root elements? Fixed. > >+#ToolbarMode > #Console\:clear { > > Why "#ToolbarMode >"? To make the rule more specific than "#ToolbarMode toolbarbutton", but I guess !important makes the intention clearer.

Thanks Dão! It's really great to have a responsive theme reviewer.

Oh, viewbutton-middle-selected.png and viewbutton-middle-inactive.png are missing in this patch...

Would it be possible to reuse viewbuttons.css in console.css? I.e. wrap the toolbarbuttons in <hbox id="viewGroup" chromedir="&locale.dir"></hbox>, use #viewGroup > * instead of #viewGroup > radio and add [checked] similar to [selected]?

I'll try that. By the way, do I need a license header in viewbuttons.css?

I guess we could do without the license header there. Then again, it can't hurt to add it.

(In reply to comment #13) > use #viewGroup > * instead of #viewGroup > radio This would result in a styled toolbarseparator (and probably in a slower selector, but I don't know if that matters much). In this patch I'm just duplicating all the selectors for toolbarbuttons, but now I realize that I could just set display:none on the toolbarseparator - do you want me to do that? (In reply to comment #15) > I guess we could do without the license header there. Then again, it can't hurt > to add it. I added it.

Is it necessary that #viewGroup contains #Console:clear and the toolbarseparator? I had this structure in mind: #viewGroup #Console:modeAll #Console:modeErrors #Console:modeWarnings #Console:modeMessages toolbarseparator #Console:clear

Ah, that makes a lot more sense.

pushed: http://hg.mozilla.org/mozilla-central/rev/5090a2e23f6b

Comment on attachment 350782 [details] [diff] [review] v8 a191=beltzner

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081213 Minefield/3.2a1pre ID:20081213020516 Markus, is this patch ready for 1.9.1 check-in?

I'm not sure. Sometimes I can see missing button backgrounds, so maybe we should wait for bug 468473 before landing this on 1.9.1. STR: 1. Start / restart Firefox. 2. Load this page in the active tab. 3. Click on the favicon in the location bar. 4. Click "More information…". Now the "Media" button's background is missing in the page info dialog.

The patch in bug 468473 fixes the problem in comment 23.

Comment on attachment 350782 [details] [diff] [review] v8 >+++ b/toolkit/themes/pinstripe/global/viewbuttons.css >+ * The Original Code is the nsSessionStore component. Can you please fix this on trunk and before landing on branch?

Markus, will this get into branch?

Bug 468473 has been fixed. Means we can go for 1.9.1. Markus please take care of comment 25. It also has to be corrected on mozilla-central. I've started a tryserver build and will check if everything works well.

Somehow the patch wasn't applied to the tryserver build. So stopped for now. In any way the patch doesn't apply cleanly on 1.9.1: patching file toolkit/themes/pinstripe/global/console/console.css Hunk #2 succeeded at 183 (offset 1 line). patching file toolkit/themes/pinstripe/global/jar.mn Hunk #1 succeeded at 46 (offset 1 line). Hunk #2 succeeded at 205 (offset 4 lines).

(In reply to comment #25) > (From update of attachment 350782 [details] [diff] [review]) > >+++ b/toolkit/themes/pinstripe/global/viewbuttons.css > > >+ * The Original Code is the nsSessionStore component. > > Can you please fix this on trunk and before landing on branch? Oops, thanks. http://hg.mozilla.org/mozilla-central/rev/793f8f182fb2 I'll land the patch on branch later today.

Or not. I'll wait for bug 470181 because I don't want to break SeaMonkey.

(In reply to comment #30) > Or not. I'll wait for bug 470181 because I don't want to break SeaMonkey. Thanks for waiting :-) There's now a fix in bug 470181 - I'll land it after you've landed your patch on 191.

Landed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e3f8301acef5

verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090108 Shiretoko/3.1b3pre. Adding keyword.