Last Comment Bug 588027 - Clean up places library code
: Clean up places library code
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: 677417
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-17 07:20 PDT by Robert Kaiser
Modified: 2012-03-19 05:58 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: apply cleanups brought up in bug 585601 (24.00 KB, patch)
2010-08-18 08:47 PDT, Robert Kaiser
mak77: review-
Details | Diff | Splinter Review
v1.1: revert Mac and Windows scope bar theming (24.44 KB, patch)
2010-09-02 13:00 PDT, Robert Kaiser
dao+bmo: review-
Details | Diff | Splinter Review
v1.2: leave toolbarbutton (22.47 KB, patch)
2010-12-01 17:23 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.3: unbitrotted (22.21 KB, patch)
2011-05-21 06:32 PDT, Robert Kaiser
mak77: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-17 07:20:26 PDT
Neil brought up some good possibilities for cleaning up the places library code when commenting on the SeaMonkey version in bug 585601. I have worked up a patch for making all those cleanups on the Firefox side, but this probably needs some good testing and UI-review on the various platforms supported with Firefox default themes.
Comment 1 Robert Kaiser 2010-08-18 08:47:44 PDT
Created attachment 467026 [details] [diff] [review]
v1: apply cleanups brought up in bug 585601

This patch applies the cleanups from bug 585601 that seem fitting for the places library as well.
All in all, this should be removing some historic cruft (spurious additional boxes in the markup), and also make the search scope bar fit in better with OS native theming.

Marco, please mark the needed ui-reviews and such, I'm not 100% sure what we need there.
Comment 2 Stefan [:stefanh] 2010-08-22 14:30:41 PDT
At least on mac, the toolbarbuttons in the scope bar are styled as "Scope bar buttons" (see the css you remove), so if you want to switch to regular push buttons, you'll need to re-style them so they fit in the scope bar.
Comment 3 Marco Bonardo [::mak] 2010-08-30 07:49:35 PDT
Regarding the scope bar, I don't like your new styling on Windows 7, and I guess Mac is bad too based on comment 2. Id you drop current styling, then you'll have to figure out a similar scope bar ui in the various OSes and restyle them accordingly.
Comment 4 Marco Bonardo [::mak] 2010-08-30 07:51:56 PDT
other changes looks fine, so you could split patch leaving organizer scope bar styling (including small button) alone.
Comment 5 Robert Kaiser 2010-08-30 12:48:57 PDT
(In reply to comment #4)
> other changes looks fine, so you could split patch leaving organizer scope bar
> styling (including small button) alone.

Probably not soon, and the scope bar stuff probably dies with your comments here, as I have no possibility to do any styling for anything but GTK2 with a very skewed theme I have running for that GNOME crap here.
Comment 6 Marco Bonardo [::mak] 2010-08-30 12:50:16 PDT
so, file a new bug to correctly make the scope styling stuff native on tier platforms
Comment 7 Marco Bonardo [::mak] 2010-09-01 02:05:35 PDT
Comment on attachment 467026 [details] [diff] [review]
v1: apply cleanups brought up in bug 585601

forgot to set the flag
Comment 8 Robert Kaiser 2010-09-02 13:00:30 PDT
Created attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

OK, here's a patch that should revert Mac and Windows scope bar theming to what it was before, and still apply all the other cleanup, including that on the scope bar itself.
Comment 9 Marco Bonardo [::mak] 2010-09-03 00:17:21 PDT
why toolbarbuttons in the scopebar toolbar have to become buttons?
Comment 10 Robert Kaiser 2010-09-03 05:58:00 PDT
(In reply to comment #9)
> why toolbarbuttons in the scopebar toolbar have to become buttons?

Neil, that was your proposal, why exactly did you want that?
Comment 11 neil@parkwaycc.co.uk 2010-09-03 06:16:40 PDT
Ordinary buttons blend in better with communicator themes (Modern, winstripe, gnomestripe) which conveniently meant we could get rid of a bunch of theme CSS.
Comment 12 Marco Bonardo [::mak] 2010-09-08 05:11:37 PDT
hm, but toolbarbuttons are buttons in toolbars, and these are buttons in a toolbar. So, i'm not sure I understand the need for the change, if the theme is wrongly managing toolbarbuttons changing them to simple buttons does not look like a solution.
Comment 13 Stefan [:stefanh] 2010-09-08 05:57:24 PDT
For pinstripe, see the css in my patch in bug 586026 - that should make the scope bar buttons look the same as before (just switching from toolbarbutton to button in the css file wasn't enough iirc).
Comment 14 Marco Bonardo [::mak] 2010-09-08 06:05:34 PDT
Comment on attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

Mano, what do you think of the toolbabutton->button change in the scope toolbar? the rest of the patch looks fine to me.
Comment 15 Marco Bonardo [::mak] 2010-09-15 09:13:01 PDT
Comment on attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

I'm directly moving review to mano, I have an hard time evaluating this subtle differences in xul widgets, so I prefer moving to his experience.
Apart the toolbarbutton->button change the rest of the patch is fine for me.
Comment 16 Dão Gottwald [:dao] 2010-11-06 03:49:13 PDT
Comment on attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

>-#organizerScopeBar > toolbarbutton > .toolbarbutton-icon {
>+#organizerScopeBar > button > .toolbarbutton-icon {
>   padding: 0;
>   margin: 0;
> }
> 
>-#organizerScopeBar > toolbarbutton > .toolbarbutton-text {
>+#organizerScopeBar > button > .toolbarbutton-text {
>    margin: 0;
>    padding: 2px 5px;
> }

This doesn't work.

I'd suggest avoiding the toolbarbutton->button change. I don't think we want this for Firefox.
Comment 17 Robert Kaiser 2010-12-01 13:13:37 PST
(In reply to comment #16)
> I'd suggest avoiding the toolbarbutton->button change. I don't think we want
> this for Firefox.

In that case, we'll need a good part of CSS added for gnomestripe, where IMHO we want them to look like a <button> actually.
Comment 18 Robert Kaiser 2010-12-01 17:23:08 PST
Created attachment 494581 [details] [diff] [review]
v1.2: leave toolbarbutton

OK, here's a patch that just leaves those buttons as toolbarbuttons. I decided I don't care if gnomestripe would look more fitting with actual buttons or not, but I really care about Firefox getting this cleanup (though I start to wonder if it would even be allowed for FF4 still - if not, it's still good to have after branching).
Comment 19 Robert Kaiser 2011-05-21 06:32:55 PDT
Created attachment 534222 [details] [diff] [review]
v1.3: unbitrotted

Here's an unbitrotted version. Can anyone else than mano review this or was it forgotten? I'd love to get this off my local queue and into the tree...
Comment 20 Marco Bonardo [::mak] 2011-05-23 02:57:48 PDT
I should have gone back to this after our talk at the all-hands, but looks like I forgot, sorry.  Will look at it soon.
Comment 21 Marco Bonardo [::mak] 2011-05-24 08:10:34 PDT
Comment on attachment 534222 [details] [diff] [review]
v1.3: unbitrotted

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

I tested on Win7 and WinXP, results seem consistent, the scope bar save button is larger but that's not a big deal (all styling of that bar is fancy, this only makes it less fancy).
Menus and trees are fine afaict, so let's proceed while we are early in the cycle :)
Comment 22 Dão Gottwald [:dao] 2011-05-25 00:36:12 PDT
http://hg.mozilla.org/mozilla-central/rev/87ce91536b59
Comment 23 Jorge Villalobos [:jorgev] 2011-08-09 08:11:45 PDT
This is probably worth documenting. It breaks a couple of extensions and maybe some themes as well. See bug 677417.
Comment 24 Florian Scholz [:fscholz] (MDN) 2012-03-19 05:58:15 PDT
Added a note to
https://developer.mozilla.org/en/Firefox_7_for_developers#Other_Changes

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