[Web Intents] Refactor picker to use WebIntentPickerModel.

[Web Intents] Refactor picker to use WebIntentPickerModel.
Change the Web Intent picker to be vertical instead of horizontal, and add titles.
Also removed WebIntentPickerControllerTest. Using WebIntentPickerModelTest instead.

BUG=90458, 110417
TEST=WebIntentPickerModelTest.*

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

[binji, 2012-01-25 18:58:29]

-jhawkins: OoO
+groby: refactor review

[groby-ooo-7-16, 2012-01-25 22:03:13]

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:117: if (item.favicon.get()
!= NULL) {
Can skip the "get" part - scoped_ptr has operator!=

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:120: ResourceBundle& rb =
ResourceBundle::GetSharedInstance();
I'd suggest moving that to the model instead - that's consistent behavior across
all platforms. (Also, at least on Cocoa, we should remove default icon handling
from the controller, too. Don't know what the other platform controllers do)

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:159: 
Nit: delete empty line

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:123: :
wrapper_(wrapper),
nit: Shouldn't this only be indented 4 spaces?

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:225: // TODO(binji):
tab_contents should never be NULL; it is in this case
nit: web_contens

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2011-2012

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:83: void
set_picker(WebIntentPicker* picker) { picker_ = picker; }
Why add methods? Tests are friends of ours, so they can reach in directly to the
member vars.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:130: bool
picker_shown_;
I might be wrong, but as far as I can tell, this is equivalent to "picker_ !=
NULL". No need to track extra state

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:43:
virtual void OnModelChanged(WebIntentPickerModel* model) {
nit: OVERRIDE, here and elsewhere

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_delegate.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_delegate.h:27: // Callback called
when the picker has created WebContents to use for inline
nit: Callback called when -> Callback when

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.cc (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.cc:76: items_.clear();
Already invoked by STLDeleteElements

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:48:
DISALLOW_COPY_AND_ASSIGN(Item);
This macro only works in private: section

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:85: size_t
inline_disposition_index() const { return inline_disposition_index_; }
That is an odd thing to have on the model - shouldn't the picker provide that?

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:95:
WebIntentPickerModelObserver* observer_;
The observer should probably be a WeakPtr<> - that saves you from having to
remove it on destruction of observer. (To get weak ptrs of the PickerModel, have
it inherit from SupportsWeakPtr<>)

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model_observer.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model_observer.h:16: // Called to
notify that |model| has changed.
nit: Called to notify that -> Callback when

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/views/web...
File chrome/browser/ui/views/web_intent_picker_view.cc (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/views/web...
chrome/browser/ui/views/web_intent_picker_view.cc:181:
button->SetIcon(rb.GetImageNamed(IDR_DEFAULT_FAVICON));
Favicons should probably be handled by the model. (So that it's not necessary
for every view to do this)

[binji, 2012-01-26 00:27:41]

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:117: if (item.favicon.get()
!= NULL) {
On 2012/01/25 22:03:13, groby wrote:
> Can skip the "get" part - scoped_ptr has operator!=

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:120: ResourceBundle& rb =
ResourceBundle::GetSharedInstance();
On 2012/01/25 22:03:13, groby wrote:
> I'd suggest moving that to the model instead - that's consistent behavior
across
> all platforms. (Also, at least on Cocoa, we should remove default icon
handling
> from the controller, too. Don't know what the other platform controllers do)

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:159: 
On 2012/01/25 22:03:13, groby wrote:
> Nit: delete empty line

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:123: :
wrapper_(wrapper),
On 2012/01/25 22:03:13, groby wrote:
> nit: Shouldn't this only be indented 4 spaces?

Looks like you're right. The line-wrap rules are the hardest for me to
extrapolate in cases like this.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:225: // TODO(binji):
tab_contents should never be NULL; it is in this case
On 2012/01/25 22:03:13, groby wrote:
> nit: web_contens

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/01/25 22:03:13, groby wrote:
> 2011-2012

I don't see any examples of this in chrome.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:83: void
set_picker(WebIntentPicker* picker) { picker_ = picker; }
On 2012/01/25 22:03:13, groby wrote:
> Why add methods? Tests are friends of ours, so they can reach in directly to
the
> member vars.

True, though I thought it was a little clearer here which member variables are
exposed to tests (even though technically they all are.)

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:130: bool
picker_shown_;
On 2012/01/25 22:03:13, groby wrote:
> I might be wrong, but as far as I can tell, this is equivalent to "picker_ !=
> NULL". No need to track extra state

I suppose it is a little tricky. The picker can be set externally (only for
tests), which means that it is not yet shown, but it is also not NULL. Maybe a
comment is necessary?

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:43:
virtual void OnModelChanged(WebIntentPickerModel* model) {
On 2012/01/25 22:03:13, groby wrote:
> nit: OVERRIDE, here and elsewhere

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_delegate.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_delegate.h:27: // Callback called
when the picker has created WebContents to use for inline
On 2012/01/25 22:03:13, groby wrote:
> nit: Callback called when -> Callback when

Looks like "Called when" is most common.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.cc (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.cc:76: items_.clear();
On 2012/01/25 22:03:13, groby wrote:
> Already invoked by STLDeleteElements

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:48:
DISALLOW_COPY_AND_ASSIGN(Item);
On 2012/01/25 22:03:13, groby wrote:
> This macro only works in private: section

Done.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:85: size_t
inline_disposition_index() const { return inline_disposition_index_; }
On 2012/01/25 22:03:13, groby wrote:
> That is an odd thing to have on the model - shouldn't the picker provide that?

Maybe... I was thinking of the model as any necessary state to display the
picker. The currently displayed inline disposition service should be included in
that, right?

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:95:
WebIntentPickerModelObserver* observer_;
On 2012/01/25 22:03:13, groby wrote:
> The observer should probably be a WeakPtr<> - that saves you from having to
> remove it on destruction of observer. (To get weak ptrs of the PickerModel,
have
> it inherit from SupportsWeakPtr<>)

I don't see any examples of this.

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model_observer.h (right):

http://codereview.chromium.org/9148032/diff/29001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model_observer.h:16: // Called to
notify that |model| has changed.
On 2012/01/25 22:03:13, groby wrote:
> nit: Called to notify that -> Callback when

Done.

[groby-ooo-7-16, 2012-01-27 00:05:40]

lgtm

[binji, 2012-01-27 18:25:40]

+erg: gtk OWNER
+sail: cocoa OWNER
+sky: views OWNER
+joi: tab_contents OWNER

[Elliot Glaysher, 2012-01-27 18:30:17]

can I see mocks or screenshots?

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/gtk/web_i...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:306:
gtk_container_foreach(GTK_CONTAINER(container), RemoveAllHelper, NULL);
gtk_util::RemoveAllChildren(). declared in chrome/browser/ui/gtk/gtk_util.h.

[sky, 2012-01-27 18:35:34]

What's the lifetime of the model?

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/views/web...
File chrome/browser/ui/views/web_intent_picker_view.cc (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/views/web...
chrome/browser/ui/views/web_intent_picker_view.cc:166: size_t index) {
nit: spacing

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/views/web...
chrome/browser/ui/views/web_intent_picker_view.cc:169:
static_cast<views::TextButton*>(buttons_[index]);
Can we make buttons_ std::vector<views::TextButton*> since that's what it
contains and avoids this cast?

[sail, 2012-01-27 18:43:58]

cocoa/* LGTM!

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:17: virtual
void OnServiceChosen(size_t index, Disposition disposition) {};
could you add OVERRIDE on all these virtual functions

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:122: const GURL& url =
item.url;
I don't think you need this temp variable

[binji, 2012-01-27 19:04:20]

erg: screenshots @ http://code.google.com/p/chromium/issues/detail?id=110417#c4

sky: the WebIntentPickerModel is owned by the WebIntentPickerController, which
is owned by the TabContentsWrapper. The model is never destroyed as long as the
controller is live, but the view is destroyed when it is closed.

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:17: virtual
void OnServiceChosen(size_t index, Disposition disposition) {};
On 2012/01/27 18:43:59, sail wrote:
> could you add OVERRIDE on all these virtual functions

Done.

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:122: const GURL& url =
item.url;
On 2012/01/27 18:43:59, sail wrote:
> I don't think you need this temp variable

Done.

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/gtk/web_i...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:306:
gtk_container_foreach(GTK_CONTAINER(container), RemoveAllHelper, NULL);
On 2012/01/27 18:30:18, Elliot Glaysher wrote:
> gtk_util::RemoveAllChildren(). declared in chrome/browser/ui/gtk/gtk_util.h.

Done.

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/views/web...
File chrome/browser/ui/views/web_intent_picker_view.cc (right):

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/views/web...
chrome/browser/ui/views/web_intent_picker_view.cc:166: size_t index) {
On 2012/01/27 18:35:34, sky wrote:
> nit: spacing

Done.

http://codereview.chromium.org/9148032/diff/38001/chrome/browser/ui/views/web...
chrome/browser/ui/views/web_intent_picker_view.cc:169:
static_cast<views::TextButton*>(buttons_[index]);
On 2012/01/27 18:35:34, sky wrote:
> Can we make buttons_ std::vector<views::TextButton*> since that's what it
> contains and avoids this cast?

Done.

[Elliot Glaysher, 2012-01-27 19:19:43]

http://codereview.chromium.org/9148032/diff/39025/chrome/browser/ui/gtk/web_i...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9148032/diff/39025/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:62: // |kServiceButtonScale|.
Comment is wrong

http://codereview.chromium.org/9148032/diff/39025/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:247: contents_ =
gtk_hbox_new(FALSE, ui::kContentAreaSpacing);
Looking at your screenshot, you have your close button hanging out rightwards to
the intent button, when their right sides should align.

I believe this widget structure will fix the floating close button, though I
might be wrong:
* toplevel vbox (instead of hbox as it is now)
*** hbox for header
***** start packed label "Complete action using:"
***** end packed close button
*** your button vbox (which is fine)
*** your webstore message label.

[binji, 2012-01-27 19:39:40]

http://codereview.chromium.org/9148032/diff/39025/chrome/browser/ui/gtk/web_i...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9148032/diff/39025/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:62: // |kServiceButtonScale|.
On 2012/01/27 19:19:44, Elliot Glaysher wrote:
> Comment is wrong

Done.

http://codereview.chromium.org/9148032/diff/39025/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:247: contents_ =
gtk_hbox_new(FALSE, ui::kContentAreaSpacing);
yes, I think that is better. screenshot at crbug.com/110417#c5

[Elliot Glaysher, 2012-01-27 20:12:14]

gtk lgtm

[sky, 2012-01-27 20:18:33]

LGTM

[Jói, 2012-01-30 09:31:24]

tab_contents LGTM

Sorry for the delay, was out of town over the weekend.  For something that
changes something where I'm OWNER so trivially, I'm OK with a TBR.

Cheers,
Jói

[commit-bot: I haz the power, 2012-01-30 19:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/9148032/44001

[commit-bot: I haz the power, 2012-01-30 20:37:48]

Try job failure for 9148032-44001 (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-01-30 23:55:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/9148032/51002

[commit-bot: I haz the power, 2012-01-31 01:24:41]

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

[commit-bot: I haz the power, 2012-02-02 00:19:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/9148032/57002

[commit-bot: I haz the power, 2012-02-02 02:40:53]

Change committed as 120149