[WebIntents, OSX] Clean up inline disposition to match mock.

[WebIntents, OSX] Clean up inline disposition to match mock.

R=thakis@chromium.org
BUG=145622
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157616

[Nico, 2012-09-05 04:32:32]

https://chromiumcodereview.appspot.com/10917077/diff/1/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

https://chromiumcodereview.appspot.com/10917077/diff/1/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:89: return [NSDictionary
dictionaryWithObjectsAndKeys:
nit:

return @{
  NSForegroundColorAttributeName: [self textColor],
  ...
};

https://chromiumcodereview.appspot.com/10917077/diff/1/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:94: nil
So this is overridden to remove the underline? Why not be consistent with the
rest of chrome's ui instead?

[groby-ooo-7-16, 2012-09-06 02:43:48]

Addressing your review issues plus some additional design feedback.

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:89: return [NSDictionary
dictionaryWithObjectsAndKeys:
On 2012/09/05 04:32:33, Nico wrote:
> nit:
> 
> return @{
>   NSForegroundColorAttributeName: [self textColor],
>   ...
> };

Done.

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:94: nil
Because UI directive is to look like WebUI, even if it's native.

(This will eventually become "underline on hover", btw)
On 2012/09/05 04:32:33, Nico wrote:
> So this is overridden to remove the underline? Why not be consistent with the
> rest of chrome's ui instead?

[Nico, 2012-09-06 02:48:21]

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:94: nil
On 2012/09/06 02:43:48, groby wrote:
> Because UI directive is to look like WebUI, even if it's native.
> 
> (This will eventually become "underline on hover", btw)
> On 2012/09/05 04:32:33, Nico wrote:
> > So this is overridden to remove the underline? Why not be consistent with
the
> > rest of chrome's ui instead?
> 

Can you add a comment to that end? (And maybe remove the "increment a by 1"
style comment that this class currently has :-) )

[groby-ooo-7-16, 2012-09-07 00:05:21]

Commentified.

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/1/chrome/browser/ui/cocoa/web_in...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:94: nil
On 2012/09/06 02:48:21, Nico wrote:
> On 2012/09/06 02:43:48, groby wrote:
> > Because UI directive is to look like WebUI, even if it's native.
> > 
> > (This will eventually become "underline on hover", btw)
> > On 2012/09/05 04:32:33, Nico wrote:
> > > So this is overridden to remove the underline? Why not be consistent with
> the
> > > rest of chrome's ui instead?
> > 
> 
> Can you add a comment to that end? (And maybe remove the "increment a by 1"
> style comment that this class currently has :-) )

Done.

[Nico, 2012-09-07 16:50:21]

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:94: [self
setTextColor:gfx::SkColorToDeviceNSColor(linkColor)];
What's wrong with NSColor colorWithDeviceR:g:b:a? (If you keep this, add a space
in 0x55,0xcc

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:102: return
@{NSForegroundColorAttributeName: [self textColor],
break after @{:

  return @{
    foo: ...,
    bar: ...,
  };

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:820: const CGFloat
kButtonFuzz = 4.0; // whitespace inside button frame.
in css speak this is called "padding", so call it padding

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:829: [closeButton_
setAutoresizingMask:NSViewMaxYMargin|NSViewMinXMargin];
y grows too the top, so this anchors to lower right. (…I guess this isn't
necessary then?)

[groby-ooo-7-16, 2012-09-08 00:29:39]

PTAL

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:94: [self
setTextColor:gfx::SkColorToDeviceNSColor(linkColor)];
On 2012/09/07 16:50:21, Nico wrote:
> What's wrong with NSColor colorWithDeviceR:g:b:a? (If you keep this, add a
space
> in 0x55,0xcc

Done.

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:102: return
@{NSForegroundColorAttributeName: [self textColor],
On 2012/09/07 16:50:21, Nico wrote:
> break after @{:
> 
>   return @{
>     foo: ...,
>     bar: ...,
>   };

Done.

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:820: const CGFloat
kButtonFuzz = 4.0; // whitespace inside button frame.
On 2012/09/07 16:50:21, Nico wrote:
> in css speak this is called "padding", so call it padding

Done.

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:829: [closeButton_
setAutoresizingMask:NSViewMaxYMargin|NSViewMinXMargin];
It's inside a flipped view. so it's actually anchored to top right :)

On 2012/09/07 16:50:21, Nico wrote:
> y grows too the top, so this anchors to lower right. (…I guess this isn't
> necessary then?)

[Nico, 2012-09-08 00:30:45]

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:829: [closeButton_
setAutoresizingMask:NSViewMaxYMargin|NSViewMinXMargin];
Might be worth a comment then.

On 2012/09/08 00:29:39, groby wrote:
> It's inside a flipped view. so it's actually anchored to top right :)
> 
> On 2012/09/07 16:50:21, Nico wrote:
> > y grows too the top, so this anchors to lower right. (…I guess this isn't
> > necessary then?)
>

[groby-ooo-7-16, 2012-09-08 00:48:38]

PTAL

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/5002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:829: [closeButton_
setAutoresizingMask:NSViewMaxYMargin|NSViewMinXMargin];
On 2012/09/08 00:30:45, Nico wrote:
> Might be worth a comment then.
> 
> On 2012/09/08 00:29:39, groby wrote:
> > It's inside a flipped view. so it's actually anchored to top right :)
> > 
> > On 2012/09/07 16:50:21, Nico wrote:
> > > y grows too the top, so this anchors to lower right. (…I guess this isn't
> > > necessary then?)
> > 
> 

Done.

[Nico, 2012-09-11 06:56:35]

lgtm

(Sorry. Please ping me when I ignore a CL for days.)

http://codereview.chromium.org/10917077/diff/3002/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/3002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:100: [[NSParagraphStyle
defaultParagraphStyle] mutableCopy]);
indent 2 more

http://codereview.chromium.org/10917077/diff/3002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:987: [[button cell]
setFont:[NSFont controlContentFontOfSize:0]];
nit: i think you replaced "0" with the explicit form somewhere else. If you
prefer 0, it'd be nice to use that consistently in this file.

[groby-ooo-7-16, 2012-09-12 22:39:10]

http://codereview.chromium.org/10917077/diff/3002/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_sheet_controller.mm (right):

http://codereview.chromium.org/10917077/diff/3002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:100: [[NSParagraphStyle
defaultParagraphStyle] mutableCopy]);
On 2012/09/11 06:56:36, Nico wrote:
> indent 2 more

Done.

http://codereview.chromium.org/10917077/diff/3002/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_sheet_controller.mm:987: [[button cell]
setFont:[NSFont controlContentFontOfSize:0]];
On 2012/09/11 06:56:36, Nico wrote:
> nit: i think you replaced "0" with the explicit form somewhere else. If you
> prefer 0, it'd be nice to use that consistently in this file.

Done.

[commit-bot: I haz the power, 2012-09-12 22:39:22]

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

[commit-bot: I haz the power, 2012-09-12 23:01:47]

Try job failure for 10917077-8004 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-12 23:05:39]

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

[commit-bot: I haz the power, 2012-09-13 01:20:16]

Try job failure for 10917077-8004 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-13 01:25:58]

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

[commit-bot: I haz the power, 2012-09-13 04:56:36]

Failed to apply patch for chrome/app/generated_resources.grd:
While running patch -p1 --forward --force;
patching file chrome/app/generated_resources.grd
Hunk #1 FAILED at 17299.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/app/generated_resources.grd.rej

[commit-bot: I haz the power, 2012-09-18 20:17:36]

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

[commit-bot: I haz the power, 2012-09-18 20:30:47]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-18 21:11:23]

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

[commit-bot: I haz the power, 2012-09-18 21:24:34]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-18 22:52:40]

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

[commit-bot: I haz the power, 2012-09-18 23:09:07]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-19 18:51:29]

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

[commit-bot: I haz the power, 2012-09-19 19:03:38]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-19 21:31:16]

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

[commit-bot: I haz the power, 2012-09-19 21:45:51]

Retried try job too often for step(s) build