[MediaGalleries] Add context menu for forgetting galleries. [views]

[MediaGalleries] Add context menu for forgetting galleries. [views]

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231703

[Greg Billock, 2013-10-23 21:31:56]

Ready for a look

[vandebo (ex-Chrome), 2013-10-24 01:06:38]

The controller looks fine modulo delete/forget, but I have no idea about the
views code.

https://codereview.chromium.org/37553002/diff/40001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/37553002/diff/40001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:15169: +    <message
name="IDS_MEDIA_GALLERIES_DIALOG_FORGET" desc="Text label of the Forget menu
item (which deletes selected media gallery for all apps).">
Lets call this delete instead of forget.  The other thing we might want to put
in the menu is to erase our notion of the gallery, i.e. forget about it until it
is attached again.... On the other hand it is consistent with preferences, which
doesn't support the other notion.

[Greg Billock, 2013-10-24 17:14:59]

+sky for views. Please look carefully at how this context menu works. I'm
following other usages in downloads and bookmarks, but there isn't one pattern,
so I'm not sure I've covered all bases.

https://codereview.chromium.org/37553002/diff/40001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/37553002/diff/40001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:15169: +    <message
name="IDS_MEDIA_GALLERIES_DIALOG_FORGET" desc="Text label of the Forget menu
item (which deletes selected media gallery for all apps).">
On 2013/10/24 01:06:38, vandebo wrote:
> Lets call this delete instead of forget.  The other thing we might want to put
> in the menu is to erase our notion of the gallery, i.e. forget about it until
it
> is attached again.... On the other hand it is consistent with preferences,
which
> doesn't support the other notion.

I think renaming is fine. I named it to echo the method name, but arguably we
should do the reverse....

[sky, 2013-10-24 19:23:22]

Your use of menus seems fine by me.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:74: :
view_(view), id_(kInvalidMediaGalleryPrefId) {}
nit: when you wrap, each param gets its own line.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:81: virtual
bool IsCommandIdChecked(int command_id) const OVERRIDE {
nit: prefix with something like:
// ui::SimpleMenuModel::Delegate overrides:

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:92: int
command_id, ui::Accelerator* accelerator) OVERRIDE {
nit: again, when you wrap one param per line.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:103: };
DISALLOW...

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:423: if
(source->GetClassName() == views::Checkbox::kViewClassName) {
The class name is best thought of as an internal implementation detail, mostly
used for debugging. OTOH the id is indented for your usage. Use the id instead.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:432: if
(!checkbox_container_view)
How can this ever be true?

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:435:
checkbox_container_view->child_at(0));
This seems rather fragile. Could you instead add members for the views you care
about?

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:441:
ShowContextMenu(point, source_type, iter->first);
return after this?

[Greg Billock, 2013-10-28 22:37:49]

OK. I've rejiggered this a bit -- realized when doing the mac version that I
should have the ui::MenuModel in the controller. Moved that, but no real
substantive change in how the menu itself works.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:74: :
view_(view), id_(kInvalidMediaGalleryPrefId) {}
On 2013/10/24 19:23:22, sky wrote:
> nit: when you wrap, each param gets its own line.

OK. Was trying too hard to conserve space in this interface since I just need a
small sliver of it.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:81: virtual
bool IsCommandIdChecked(int command_id) const OVERRIDE {
On 2013/10/24 19:23:22, sky wrote:
> nit: prefix with something like:
> // ui::SimpleMenuModel::Delegate overrides:

Done.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:92: int
command_id, ui::Accelerator* accelerator) OVERRIDE {
On 2013/10/24 19:23:22, sky wrote:
> nit: again, when you wrap one param per line.

Done.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:103: };
On 2013/10/24 19:23:22, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:423: if
(source->GetClassName() == views::Checkbox::kViewClassName) {
On 2013/10/24 19:23:22, sky wrote:
> The class name is best thought of as an internal implementation detail, mostly
> used for debugging. OTOH the id is indented for your usage. Use the id
instead.

OK. Found a much better plan for this.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:432: if
(!checkbox_container_view)
On 2013/10/24 19:23:22, sky wrote:
> How can this ever be true?

line 299. (At least, I'm trying to set the context menu on the whole line/row --
it seems to get triggered in practice, which is what I want. Should I not want
that?)

Found a much-improved way to say this in views-anese now.

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:435:
checkbox_container_view->child_at(0));
On 2013/10/24 19:23:22, sky wrote:
> This seems rather fragile. Could you instead add members for the views you
care
> about?

no longer used

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:441:
ShowContextMenu(point, source_type, iter->first);
On 2013/10/24 19:23:22, sky wrote:
> return after this?

Done.

[sky, 2013-10-29 01:19:50]

LGTM

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:432: if
(!checkbox_container_view)
On 2013/10/28 22:37:49, Greg Billock wrote:
> On 2013/10/24 19:23:22, sky wrote:
> > How can this ever be true?
> 
> line 299. (At least, I'm trying to set the context menu on the whole line/row
--
> it seems to get triggered in practice, which is what I want. Should I not want
> that?)
> 
> Found a much-improved way to say this in views-anese now.

The part I was confused on is you assign checkbox_container_view_ on 428 (to
source) and 430 (to source->parent). I don't see how either of those could ever
be NULL, hence not understanding the if.

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:388: void
MediaGalleriesDialogViews::ShowContextMenu(const gfx::Point& point,
Position should match that of header. Maybe a bunch of the functions are out of
order here?

[vandebo (ex-Chrome), 2013-10-29 14:55:17]

LGTM

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_dialog_controller.cc
(right):

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_dialog_controller.cc:102:
menu_model->AddItem(1, GetDeleteMenuCommand());
Now that this is in the controller, you don't need GetDeleteMenuCommand()

[Greg Billock, 2013-10-29 17:04:05]

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_dialog_controller.cc
(right):

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_dialog_controller.cc:102:
menu_model->AddItem(1, GetDeleteMenuCommand());
On 2013/10/29 14:55:17, vandebo wrote:
> Now that this is in the controller, you don't need GetDeleteMenuCommand()

Done.

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/37553002/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:388: void
MediaGalleriesDialogViews::ShowContextMenu(const gfx::Point& point,
On 2013/10/29 01:19:50, sky wrote:
> Position should match that of header. Maybe a bunch of the functions are out
of
> order here?

I put this after ButtonPressed in both, but yes, I think InitChildViews is
out-of-order. Perhaps others too.

[Greg Billock, 2013-10-29 17:05:28]

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

https://codereview.chromium.org/37553002/diff/60001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:432: if
(!checkbox_container_view)
On 2013/10/29 01:19:50, sky wrote:
> On 2013/10/28 22:37:49, Greg Billock wrote:
> > On 2013/10/24 19:23:22, sky wrote:
> > > How can this ever be true?
> > 
> > line 299. (At least, I'm trying to set the context menu on the whole
line/row
> --
> > it seems to get triggered in practice, which is what I want. Should I not
want
> > that?)
> > 
> > Found a much-improved way to say this in views-anese now.
> 
> The part I was confused on is you assign checkbox_container_view_ on 428 (to
> source) and 430 (to source->parent). I don't see how either of those could
ever
> be NULL, hence not understanding the if.

ok. I found a much better solution for this with Contains(), so this nastiness
went away.

[commit-bot: I haz the power, 2013-10-29 17:06:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/37553002/340001

[commit-bot: I haz the power, 2013-10-30 01:35:56]

Change committed as 231703