Last Comment Bug 682541 - [10.7] Give search fields in window chrome the new on-chrome style on 10.7
: [10.7] Give search fields in window chrome the new on-chrome style on 10.7
Status: RESOLVED FIXED
: polish
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Markus Stange [:mstange]
:
:
Mentors:
Depends on:
Blocks: 681974
  Show dependency treegraph
 
Reported: 2011-08-27 03:46 PDT by Markus Stange [:mstange]
Modified: 2011-09-08 14:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (7.06 KB, patch)
2011-08-27 03:46 PDT, Markus Stange [:mstange]
jaas: review+
Details | Diff | Splinter Review

Description Markus Stange [:mstange] 2011-08-27 03:46:15 PDT
Created attachment 556251 [details] [diff] [review]
v1

(In reply to Stephen Horlander from bug 681974 comment #0)
> Textfields on unified toolbars in Lion have a slightly different appearance
> consisting of:
> 
> - Lighter border
> - Subtle dark inner gradient
> - Subtle inner-shadow differences
Comment 1 Josh Aas 2011-09-06 06:49:38 PDT
Comment on attachment 556251 [details] [diff] [review]
v1

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

::: widget/src/cocoa/nsNativeThemeCocoa.mm
@@ +234,5 @@
> +{
> +  mContext = aContext;
> +}
> +
> +static BOOL IsWindowChrome(nsIFrame* aFrame)

I'd expect this to query the frame's content for chrome (privileged) status. This doesn't do that, it assumes certain elements are chrome. Theoretically that could be wrong, and it won't return true for all privileged content. Can you come up with a less confusing name?

@@ +262,5 @@
> +  // the searchfield is on top of of window chrome. This function is called on
> +  // 10.7 during drawing in order to determine which style to use.
> +  for (nsIFrame* frame = mContext; frame; frame = frame->GetParent()) {
> +    if (IsWindowChrome(frame))
> +      return YES;

I prefer {} around all blocks.

@@ +897,5 @@
>    DrawCellWithSnapping(cell, cgContext, inBoxRect, searchFieldSettings,
>                         VerticalAlignFactor(aFrame), mCellDrawView,
>                         IsFrameRTL(aFrame));
>  
> +  [cell setContext:nsnull];

Works for me but you might want to document this ptr management where you declare the class in order to avoid usage that leads to a dangling pointer.
Comment 2 Markus Stange [:mstange] 2011-09-08 06:34:13 PDT
Renamed IsWindowChrome to IsToolbarStyleContainer and added a comment.

http://hg.mozilla.org/integration/mozilla-inbound/rev/9943ca23ec8a

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