views: Inherit from views::WidgetDelegateView in more places.

views: Inherit from views::WidgetDelegateView in more places.

That way the clients don't need to reimplement the GetWidget() function everytime,
to do the same thing that the WidgetDelegateView implementation already does.

R=pkasting@chromium.org

[tfarina, 2012-07-18 02:56:23]

Does this looks right?

[Peter Kasting, 2012-07-18 03:09:02]

Nice!  LGTM

[tfarina, 2012-07-18 03:27:06]

On Wed, Jul 18, 2012 at 12:09 AM,  <pkasting@chromium.org> wrote:
> Nice!  LGTM
>
Do'h, hit send to early. :-(

There are tons of errors about  "error: reference to 'GetWidget' is
ambiguous." and other ambiguities as well due to the fact the
WidgetDelegateView inherits from View.

This is probably because at some of these classes we already inherit
from views::View (then inheriting from WidgetDelegateView causes us to
double inherit from it, this is either explicit or implicit through
some other class that already inherits from View), and they use
GetWidget()->Foo(), so looks like the compiler can't decide which one
to call, the version from WidgetDelegateView, or the one from View
itself.

Do you think it would make more sense to WidgetDelegate provide a
default implementation for GetWidget (instead of it being a pure
virtual function) that returns View::GetWidget?

And then we could probably kill WidgetDelegateView later on?

-- 
Thiago

[Peter Kasting, 2012-07-18 03:35:33]

On 2012/07/18 03:27:06, tfarina wrote:
> On Wed, Jul 18, 2012 at 12:09 AM,  <mailto:pkasting@chromium.org> wrote:
> > Nice!  LGTM
> >
> Do'h, hit send to early. :-(
> 
> There are tons of errors about  "error: reference to 'GetWidget' is
> ambiguous." and other ambiguities as well due to the fact the
> WidgetDelegateView inherits from View.

Ahhhhh.... multiple inheritance rears its ugly head.  I suppose in theory we
could solve this via virtual inheritance but I'm pretty sure that would be
stretching the style guide too far :)

> Do you think it would make more sense to WidgetDelegate provide a
> default implementation for GetWidget (instead of it being a pure
> virtual function) that returns View::GetWidget?

But how would it do this without WidgetDelegate inheriting from View?  At which
point you have the same problem.

I guess one way we could do it is to do something like:

class WidgetDelegateView : public WidgetDelegate {
  // Subclasses should pass |this| here.
  WidgetDelegate(View* this_) : this_(this_) {}

  View* this_;
};

Widget* WidgetDelegateView::GetWidget() {
  return this_->View::GetWidget();
}

(at least I think this is valid C++).

You might want to ask sky or ask on chromium-dev if there are better answers.

[tfarina, 2012-07-18 04:46:25]

On Wed, Jul 18, 2012 at 12:35 AM,  <pkasting@chromium.org> wrote:
> On 2012/07/18 03:27:06, tfarina wrote:
>
>> On Wed, Jul 18, 2012 at 12:09 AM,  <mailto:pkasting@chromium.org> wrote:
>> > Nice!  LGTM
>> >
>> Do'h, hit send to early. :-(
>
>
>> There are tons of errors about  "error: reference to 'GetWidget' is
>> ambiguous." and other ambiguities as well due to the fact the
>> WidgetDelegateView inherits from View.
>
>
> Ahhhhh.... multiple inheritance rears its ugly head.
:) heh, just exposed it.

>
>> Do you think it would make more sense to WidgetDelegate provide a
>> default implementation for GetWidget (instead of it being a pure
>> virtual function) that returns View::GetWidget?
>
>
> But how would it do this without WidgetDelegate inheriting from View?
Just confused, because this has the same syntax of static member functions. :/

> At
> which
> point you have the same problem.
>
> I guess one way we could do it is to do something like:
>
> class WidgetDelegateView : public WidgetDelegate {
>   // Subclasses should pass |this| here.
>   WidgetDelegate(View* this_) : this_(this_) {}
>
>   View* this_;
> };
>
> Widget* WidgetDelegateView::GetWidget() {
>   return this_->View::GetWidget();
> }
>
This has another problem, just tried it here and it give us this:

../../ui/views/widget/widget_delegate.h:168:19: note: member found by
ambiguous name lookup
  virtual Widget* GetWidget() OVERRIDE;
                  ^
../../ui/views/bubble/bubble_delegate.cc:280:3: error: member
'GetWidget' found in multiple base classes of different types
  GetWidget()->Close();
  ^

Because we inherit from View and WidgetDelegateView, both still have
an declaration/definition of GetWidget(). So the ambiguity is still
there. Ouch, :-(

Ben did a dark trick here ;)

> (at least I think this is valid C++).
>
> You might want to ask sky or ask on chromium-dev if there are better
> answers.
>

-- 
Thiago

[Ben Goodger (Google), 2012-07-23 21:17:54]

etc etc.

you can't use WDV in a place where the class already subclasses View by some
other method.

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/ash/pa...
File chrome/browser/ui/views/ash/panel_view_aura.h (right):

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/ash/pa...
chrome/browser/ui/views/ash/panel_view_aura.h:40: public
views::WidgetDelegateView,
This is wrong. If you do this, you'll end up subclassing View via both
NativeViewHost and WDV.

WDV is intended to mean "A View which is a WidgetDelegate" not "Something that
implements GetWidget()".

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/constr...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/constr...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:75: public
views::WidgetDelegateView {
again, wrong.

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.h:86: public
views::WidgetDelegateView,
wrong

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/web_di...
File chrome/browser/ui/views/web_dialog_view.h (right):

http://codereview.chromium.org/10795005/diff/1/chrome/browser/ui/views/web_di...
chrome/browser/ui/views/web_dialog_view.h:43: public views::WidgetDelegateView {
wrong