[rAC] added popup highlighting

[rAC] added popup highlighting

Also added a shared protocol for text fields and popups so main logic
can access fields without differentiating by type.

R=sail@chromium.org
BUG=157274
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209330

[sail, 2013-06-28 03:08:56]

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_input_field.h (right):

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_input_field.h:16: //- (BOOL)isInvalid;
remove?

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_input_field.h:18: @property(copy,
nonatomic) NSString* fieldValue;
(nonatomic, copy)

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h (right):

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h:14: @property(assign,
nonatomic) BOOL invalid;
don't need to declare this since it's declared in the AutofillInputField
protocol

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h:23: @property(assign,
nonatomic) BOOL invalid;
same, don't need to declare this

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm (right):

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:9: @implementation
AutofillPopUpCell
Move below implementation of AutofillPopUpButton to match declaration order?

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:15: if (invalid_) {
Do early return?
Also comment why you don't need to do any drawing if invalid_ is not set?

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:24: [super
drawBezelWithFrame:localFrame inView:[NSView focusView]];
can't you just pass controlView here?

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:28:
base::scoped_nsobject<NSImage> finalImage([bezelImage copyWithZone:nil]);
I don't think you need this. You can do everything you're doing below directly
into the view.

Actually, I don't think you even need to use any images. You might be able to do
something like this:

drawBezel();
drawRed();
CGContextBeginTransparencyLayer();
CGContextSetBlendMode(kCGBlendModePlusDarker)
drawBezel()
CGContextEndTransparencyLayer()

[groby-ooo-7-16, 2013-06-28 03:22:38]

Just a quick comment on the "you don't need this" part - I can only draw into
the control directly if there's nothing else already there. And I believe there
are items already drawn at that point - the whole point of the images is to have
a clean bezel that I can then NSCompositeSourceOver the existing image.

Am I missing something?

[sail, 2013-06-28 03:27:24]

On 2013/06/28 03:22:38, groby wrote:
> Just a quick comment on the "you don't need this" part - I can only draw into
> the control directly if there's nothing else already there. And I believe
there
> are items already drawn at that point - the whole point of the images is to
have
> a clean bezel that I can then NSCompositeSourceOver the existing image.
> 
> Am I missing something?

I'm just guessing here but I think CGContextBeginTransparencyLayer() does what
you want.

It basically creates a new clean layer that all drawing gets sent to. It's
basically an alternative to drawing offscreen like you're doing.

[groby-ooo-7-16, 2013-06-28 18:49:52]

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_input_field.h (right):

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_input_field.h:16: //- (BOOL)isInvalid;
On 2013/06/28 03:08:56, sail wrote:
> remove?

Done.

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_input_field.h:18: @property(copy,
nonatomic) NSString* fieldValue;
On 2013/06/28 03:08:56, sail wrote:
> (nonatomic, copy)

Done.

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h (right):

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h:14: @property(assign,
nonatomic) BOOL invalid;
On 2013/06/28 03:08:56, sail wrote:
> don't need to declare this since it's declared in the AutofillInputField
> protocol

Done.

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h:23: @property(assign,
nonatomic) BOOL invalid;
On 2013/06/28 03:08:56, sail wrote:
> same, don't need to declare this

Done.

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm (right):

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:9: @implementation
AutofillPopUpCell
On 2013/06/28 03:08:56, sail wrote:
> Move below implementation of AutofillPopUpButton to match declaration order?

Done.

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:15: if (invalid_) {
On 2013/06/28 03:08:56, sail wrote:
> Do early return?
> Also comment why you don't need to do any drawing if invalid_ is not set?

Because I stupidly only tested the invalid path - fixed, and thanks for
catching!

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:24: [super
drawBezelWithFrame:localFrame inView:[NSView focusView]];
On 2013/06/28 03:08:56, sail wrote:
> can't you just pass controlView here?

It's moot with the new rendering code, but the docs advise against it.
(Sometimes, not in all draw functions)

To quote e.g. drawWithFrame:
    "This method draws the cell in the currently focused view, which can be
different from the controlView passed in. Taking advantage of this behavior is
not recommended, however."

If you're aware of docs that are more clear on this, please point me there -
it's a bit of voodoo to me if I can or cannot just use the controlView, based on
this.

https://codereview.chromium.org/18112007/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.mm:28:
base::scoped_nsobject<NSImage> finalImage([bezelImage copyWithZone:nil]);
I originally misunderstood the grouping here - yes, this works. Thanks for
pointing it out!
On 2013/06/28 03:08:56, sail wrote:
> I don't think you need this. You can do everything you're doing below directly
> into the view.
> 
> Actually, I don't think you even need to use any images. You might be able to
do
> something like this:
> 
> drawBezel();
> drawRed();
> CGContextBeginTransparencyLayer();
> CGContextSetBlendMode(kCGBlendModePlusDarker)
> drawBezel()
> CGContextEndTransparencyLayer()

[sail, 2013-06-28 19:23:34]

lgtm

[commit-bot: I haz the power, 2013-06-28 21:23:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/18112007/8001

[commit-bot: I haz the power, 2013-06-29 20:35:02]

Change committed as 209330