Switch to using WebIntentPickerModel, and bring picker closer to mocks.

Switch to using WebIntentPickerModel, and bring picker closer to mocks.

BUG=90458, 112925
TEST=WebIntentBubbleControllerTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120691

[Nico, 2012-02-03 23:27:07]

Looks pretty good.

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/web_intent_bubble_controller.mm (right):

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_bubble_controller.mm:39: const CGFloat
kServiceButtonWidth = 300;
Does this localize?

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_bubble_controller.mm:230:
atOffset:(CGFloat)offset {
yOffset:? Makes this sound less like an index too.

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_bubble_controller.mm:285: // Create the dummy
view that uses flipped coordinates.
nit: add 1 indent to comment

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:55: // Expect 1
subview - the flip view
.

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:64: NSUInteger
cws_index = 2 + icon_count;
You don't really use this. Maybe remove, and below the "// Verify" comment, do:

  NSButton* button = static_cast<NSButton*>([views lastObject]);
  EXPECT_TRUE([button isKindOfClass:[NSButton class]]);
  // ...rest as before.

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:82:
CheckServiceButton(button, i);
Can't you fold this loop into the previous loop?

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:58:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
this macro is only needed for msvc i think

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:99: // Check to see if a
layout has already been scheduled.
indent only 2

https://chromiumcodereview.appspot.com/9310074/diff/6/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:111: // can be delivered.
This looks familiar. Can this code be factored out somehow instead of copying it
over?

[groby-ooo-7-16, 2012-02-06 22:30:19]

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
File chrome/browser/ui/cocoa/web_intent_bubble_controller.mm (right):

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_bubble_controller.mm:39: const CGFloat
kServiceButtonWidth = 300;
No, but neither do the strings in that UI for now. That's still on the list.
On 2012/02/03 23:27:07, Nico wrote:
> Does this localize?

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_bubble_controller.mm:230:
atOffset:(CGFloat)offset {
It would be inconsistent with all other bubble controllers then, though. (This
is the standard pattern, it seems)
On 2012/02/03 23:27:07, Nico wrote:
> yOffset:? Makes this sound less like an index too.

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_bubble_controller.mm:285: // Create the dummy
view that uses flipped coordinates.
On 2012/02/03 23:27:07, Nico wrote:
> nit: add 1 indent to comment

Done.

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:55: // Expect 1
subview - the flip view
On 2012/02/03 23:27:07, Nico wrote:
> .

Done.

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:64: NSUInteger
cws_index = 2 + icon_count;
On 2012/02/03 23:27:07, Nico wrote:
> You don't really use this. Maybe remove, and below the "// Verify" comment,
do:
> 
>   NSButton* button = static_cast<NSButton*>([views lastObject]);
>   EXPECT_TRUE([button isKindOfClass:[NSButton class]]);
>   // ...rest as before.

Done.

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:82:
CheckServiceButton(button, i);
I didn't want to fold it up since technically the test blows up completely if
the classes don't match. Changed the class check to ASSERT_TRUE to make that
explicit.

On 2012/02/03 23:27:07, Nico wrote:
> Can't you fold this loop into the previous loop?

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:58:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
On 2012/02/03 23:27:07, Nico wrote:
> this macro is only needed for msvc i think

Done.

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:99: // Check to see if a
layout has already been scheduled.
On 2012/02/03 23:27:07, Nico wrote:
> indent only 2

Done.

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:111: // can be delivered.
It _is_ familiar. The PageInfoBubble does _almost_ the same thing. 

Except for a different delay, a different selector that's ultimately invoked,
and the fact that one is part of the bubble class and the other isn't.

They're just different enough that I couldn't find a good way to pull this out
without making it overkill. 

On 2012/02/03 23:27:07, Nico wrote:
> This looks familiar. Can this code be factored out somehow instead of copying
it
> over?

[Nico, 2012-02-06 22:39:38]

lgtm

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9310074/diff/6/chrome/browser/ui/cocoa/web_int...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:111: // can be delivered.
On 2012/02/06 22:30:19, groby wrote:
> It _is_ familiar. The PageInfoBubble does _almost_ the same thing. 
> 
> Except for a different delay, a different selector that's ultimately invoked,
> and the fact that one is part of the bubble class and the other isn't.
> 
> They're just different enough that I couldn't find a good way to pull this out
> without making it overkill. 

:-/

http://codereview.chromium.org/9310074/diff/7001/chrome/browser/ui/cocoa/web_...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/9310074/diff/7001/chrome/browser/ui/cocoa/web_...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:69:
ASSERT_TRUE([[views lastObject] isKindOfClass:[NSButton class]]);
Can you move this three down? Then you don't need to call lastObject twice, you
can just do

  ASSERT_TRUE([button isKindOfClass:[NSButton class]]);

(Casting to NSButton* before asserting this looks a bit funny, but is fairly
common in our code, and isn't wrong.)

[commit-bot: I haz the power, 2012-02-06 23:55:41]

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

[commit-bot: I haz the power, 2012-02-06 23:55:44]

Can't apply patch for file chrome/browser/ui/cocoa/web_intent_picker_cocoa.h.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/cocoa/web_intent_picker_cocoa.h
Hunk #2 FAILED at 56.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/cocoa/web_intent_picker_cocoa.h.rej

[commit-bot: I haz the power, 2012-02-07 00:09:43]

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

[commit-bot: I haz the power, 2012-02-07 02:41:07]

Change committed as 120691