V2

sFirst cut at an experiment with what a simplified View/Widget API would look like.

Of note:

- Widget is a cross-platform class encapsulating widget-specific state
- NativeWidget interface wraps different implementations of a native widget. Starting with HWND.
- NativeWidget implementation should eventually be swappable at runtime.
- Simpler Event construction directly from a NativeEvent (e.g. MSG struct)
- Simpler View API with fewer, more clearly delineated overrides.

Notes:

- Window* are just empty files for now while I get View/Widget to work.

BUG=none
TEST=see unittests, in development


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

[sky, 2011-02-01 18:56:22]

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/context_men...
File ui/views/events/context_menu_controller.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/context_men...
ui/views/events/context_menu_controller.h:5: #ifndef
UI_VIEWS_CONTEXT_MENU_CONTROLLER_H_
VIEWS_EVENTS

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/drag_contro...
File ui/views/events/drag_controller.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/drag_contro...
ui/views/events/drag_controller.h:5: #ifndef UI_VIEWS_DRAG_CONTROLLER_H_
VIEWS_EVENTS

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h
File ui/views/events/event.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:5: #ifndef UI_VIEWS_EVENT_H_
VIEWS_EVENTS

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:55: bool IsShiftDown() const { return (flags_ &
EF_SHIFT_DOWN) != 0; }
I think all this should be named unix_hacker_style.

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:88: LocatedEvent(const LocatedEvent& other, View*
source, View* target);
This is worth documenting.

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:104: MouseEvent(NativeEvent native_event);
explicit

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:137: KeyEvent(NativeEvent native_event);
explicit

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:153: MouseWheelEvent(NativeEvent native_event);
explicit

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:163: /*
Did you mean to leave this commented out?

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event_win.cc
File ui/views/events/event_win.cc (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event_win.c...
ui/views/events/event_win.cc:170: key_code_ =
KeyboardCodeForWindowsKeyCode(native_event.wParam);
Move all to member initializer.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode42
ui/views/view.cc:42: return true;
TODO?

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode85
ui/views/view.cc:85: }
You sure you don't want the:

if (bounds == bounds_) {
  if (needs_layout_) ...

stuff from the old code? If you don't include that, we'll need to change code
relying on it.

Also, what about notifying the RootView of the change? That is needed for
tracking bounds changes in the root (for native widget clipping).

I also tend to think this should handle invoking Layout so that subclasses don't
have to worry about invoking Layout themselves.

At a minimum you should add TODOs for these.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode136
ui/views/view.cc:136: }
This is missing some trickier bits. Specifically SchedulePaint and checking
needs_layout_. If you don't want to add now, add the TODO.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode141
ui/views/view.cc:141: Invalidate();
Because Invalidate does nothing if !visible_, you need to either invoke this
before and after you change visible_ or invalidate the parent.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode224
ui/views/view.cc:224: // TODO(beng): Notify other objects like tooltip, layout
manager, etc.
and child bounds notification and focus initialization.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode244
ui/views/view.cc:244: RemoveChildView(*it);
This needs to honor delete_children.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode279
ui/views/view.cc:279: return GetContentsBounds().Contains(point);
I think the border should be considered part of the view, so this shouldn't use
GetContentBounds.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode431
ui/views/view.cc:431: void View::Paint(gfx::Canvas* canvas) {
Because the canvas is not in the child coordinates I think this shouldn't be
virtual. Otherwise it's all to easy for a subclass to forget the translate and
paint in the wrong place. In fact maybe this should be private?

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode440
ui/views/view.cc:440: ScopedCanvasState canvas_state(canvas);
Don't you need a different name than on line 436?

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode504
ui/views/view.cc:504: void View::StartShellDrag(const MouseEvent& event,
How come you named this with 'shell'? Why not just StartDrag?

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode513
ui/views/view.cc:513: if (!enabled_)
This crashes if 'this' is deleted in handling OnMousePressed.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode540
ui/views/view.cc:540: return !!context_menu_controller_ ||
drag_info->possible_drag;
This crashes if 'this' is deleted in handling OnMouseDragged.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode545
ui/views/view.cc:545: if (context_menu_controller_ &&
event.IsOnlyRightMouseButton()) {
This crashes if 'this' is deleted in handling OnMouseReleased.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.h
File ui/views/view.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.h#newcode25
ui/views/view.h:25: class ViewLayer;
Nuke ViewLayer.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.h#newcode240
ui/views/view.h:240: virtual void OnPaint(gfx::Canvas* canvas);
I think it's worth documenting these so that folks know which one to override
and why.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.h#newcode248
ui/views/view.h:248: ContextMenuController* context_menu_controller) {
indent this two more spaces and bring in line 249 two spaces.

http://codereview.chromium.org/6286013/diff/59002/ui/views/widget/native_widg...
File ui/views/widget/native_widget_views.cc (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/widget/native_widg...
ui/views/widget/native_widget_views.cc:6: namespace internal {
Do you want this file?

http://codereview.chromium.org/6286013/diff/59002/ui/views/widget/native_widg...
File ui/views/widget/native_widget_views.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/widget/native_widg...
ui/views/widget/native_widget_views.h:10: namespace internal {
Do you really want this file?

[Ben Goodger (Google), 2011-02-01 20:04:57]

Updated CL.

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h
File ui/views/events/event.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/events/event.h#new...
ui/views/events/event.h:163: /*
On 2011/02/01 18:56:22, sky wrote:
> Did you mean to leave this commented out?

Yes I have not tested it yet.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode224
ui/views/view.cc:224: // TODO(beng): Notify other objects like tooltip, layout
manager, etc.
On 2011/02/01 18:56:22, sky wrote:
> and child bounds notification and focus initialization.

I actually want to try and see if I can remove the focus initialization and have
the focus manager just infer that information somehow.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode504
ui/views/view.cc:504: void View::StartShellDrag(const MouseEvent& event,
On 2011/02/01 18:56:22, sky wrote:
> How come you named this with 'shell'? Why not just StartDrag?

These methods are for initiating and dealing with shell D&D vs. view drag
events.

http://codereview.chromium.org/6286013/diff/59002/ui/views/view.cc#newcode513
ui/views/view.cc:513: if (!enabled_)
On 2011/02/01 18:56:22, sky wrote:
> This crashes if 'this' is deleted in handling OnMousePressed.

I added a TODO... I do have to wonder if this is a case we want to forbid. I
can't think of any code doing this for a reasonable reason?

http://codereview.chromium.org/6286013/diff/59002/ui/views/widget/native_widg...
File ui/views/widget/native_widget_views.h (right):

http://codereview.chromium.org/6286013/diff/59002/ui/views/widget/native_widg...
ui/views/widget/native_widget_views.h:10: namespace internal {
On 2011/02/01 18:56:22, sky wrote:
> Do you really want this file?

Yes... a stub... working on the other stuff right now.

[sky, 2011-02-01 20:08:12]

LGTM