717476 - [10.7] on background windows: background of bookmark toolbar is visibly darker than the main toolbar

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[philippe (part-time)]

Attachment: screen shot background window

The bookmark bar doesn't change colors, whereas the main toolbar becomes much lighter.
main toolbar - bottom pixel color value:
foreground window: 185~187
background window: 224~226
(those backgrounds have a grainy texture)

The colors of the gradient in the bookmark bar are hardcoded (top to bottom : 217 - 196 according to digital color meter)

Comment 1

[philippe (part-time)]

Attachment: screen Mail background window

As a reference, a Mail window in the background.
Mail uses a gradient that goes from top of the titlebar to the bottom of the 'bookmark' toolbar

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

BookmarkToolbar's drawBackgroundInRect: will need to be rearchitected slightly to handle 10.7 && !isMainWindow separately.  Shouldn't be too difficult, though, just a bit of a pain refactoring.

http://mxr.mozilla.org/camino/source/camino/src/bookmarks/BookmarkToolbar.mm#166

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Draw a lighter gradient for non-main windows on 10.7

Actually, the refactoring doesn't seem like it was that much of a pain after all; the pain is just unrelated in my head.

This patch uses values philippe sampled from Safari and Mail.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Draw a lighter gradient for non-main windows on 10.7, v1.1

Note to self: don't write patches with a headache.

Comment 5

[philippe (part-time)]

Comment on attachment 587933 [details] [diff] [review]
Draw a lighter gradient for non-main windows on 10.7, v1.1

This builds correctly and looks good on 10.7.

Comment 6

[philippe (part-time)]

Attachment: screenshot + patch default bm bar

Default bookmark bar, background window

Comment 7

[philippe (part-time)]

Attachment: screenshot + patch multiple rows bm bar

multiple rows of bookmarks, assorted favicons, background window

Comment 8

[philippe (part-time)]

Attachment: screenshot + patch background and foreground window

2 windows overlapping

Comment 9

[Stuart Morgan]

Comment on attachment 587933 [details] [diff] [review]
Draw a lighter gradient for non-main windows on 10.7, v1.1

>+#if MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_6
>+  return YES;
>+#else

This will almost certainly break the build on older machines; Apple doesn't (or at least hasn't in the past) pre-declared versions that don't exist yet, so this probably only builds on 10.6+. We'll have to leave out the short-circuit until older build configs are not supported.

Comment 10

[Stuart Morgan]

Comment on attachment 587933 [details] [diff] [review]
Draw a lighter gradient for non-main windows on 10.7, v1.1

sr=smorgan other than that.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #9)

> >+#if MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_6
> >+  return YES;
> >+#else
> 
> This will almost certainly break the build on older machines; Apple doesn't
> (or at least hasn't in the past) pre-declared versions that don't exist yet,
> so this probably only builds on 10.6+. We'll have to leave out the
> short-circuit until older build configs are not supported.

Er, sorry, Bugzilla didn't warn me that you'd commented when I asked for sr.

I did wonder about that, but the patch does build for me on 10.5--I made several typos on the way to getting it right!  Also, +isLeopardOrHigher does the exact same thing, but I guess it happens to use the MAC_OS_X_VERSION_ of the lowest supported build host OS.

However, +isTigerOrHigher (bug 314602 / http://hg.mozilla.org/camino/rev/649a098446ca) dated from the 1.0.x era, and I know we could still build on 10.3 way back then!

So based on that, I *think* we're OK.  I can land it as-is and remove the short-circuit if anything burns, though.

Comment 12

[Stuart Morgan]

Hm, weird. I know I've run into cases where, e.g., we had to use
  > VERSION_10_(x-1)
instead of
  >= VERSION_10_x
because the latter wouldn't compile on older toolchains.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/3046212f6847 as-is; builds succeeded on all four tinderboxen.