Rebase BookmarkBubble on the new views bubble.

Rebase BookmarkBubble on the new views bubble.

Use a View instead of a Point for anchoring the bubble.
Remove the now unused IDS_BOOKMARK_BUBBLE_ADD_BOOKMARK.
Remove the unused BubbleDelegate base class from StarView.

BUG=97248, 98312, 98323
TEST=Bookmark bubble has the same behavior/appearance.

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

[msw, 2011-11-08 23:24:04]

Woohoo! The bookmark bubble is ready! PTAL; thanks!

[Ben Goodger (Google), 2011-11-08 23:58:10]

http://codereview.chromium.org/8368016/diff/27001/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

http://codereview.chromium.org/8368016/diff/27001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:96:
bookmark_bubble_->title_tf_->RequestFocus();
Can the title_tf_ be the InitiallyFocusedView instead?

http://codereview.chromium.org/8368016/diff/27001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:97:
bookmark_bubble_->title_tf_->SelectAll();
I wonder if we should have a property on TextField that is "select all on
focus".

The omnibox would use it too.

[msw, 2011-11-09 01:47:05]

Done, PTAL; thanks!
TODO(in another CL): Omnibox use of select_all_on_focus.

http://codereview.chromium.org/8368016/diff/27001/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

http://codereview.chromium.org/8368016/diff/27001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:96:
bookmark_bubble_->title_tf_->RequestFocus();
On 2011/11/08 23:58:10, Ben Goodger (Google) wrote:
> Can the title_tf_ be the InitiallyFocusedView instead?

Done.

http://codereview.chromium.org/8368016/diff/27001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:97:
bookmark_bubble_->title_tf_->SelectAll();
On 2011/11/08 23:58:10, Ben Goodger (Google) wrote:
> I wonder if we should have a property on TextField that is "select all on
> focus".
> 
> The omnibox would use it too.

Done.

[Ben Goodger (Google), 2011-11-09 17:11:08]

General comment...

seeing all this positioning logic showing up.. makes me wonder if the show
bubble routines shouldn't take an anchor view rather than a point. Then the
bubble would figure out where to position itself relative to the anchor view.
WDYT?

http://codereview.chromium.org/8368016/diff/28014/views/controls/textfield/te...
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/8368016/diff/28014/views/controls/textfield/te...
views/controls/textfield/textfield.cc:392: SelectAll();
unit test?

[msw, 2011-11-10 06:12:03]

Done. PTAL; thanks!

Anchoring on a view and allowing bubbles to override GetAnchorPoint looks good;
thanks for the suggestion!

I'd like to cleanup/consolidate the anchor and bounds code further, but some
polish might have to wait until I make more progress converting bubbles.

http://codereview.chromium.org/8368016/diff/28014/views/controls/textfield/te...
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/8368016/diff/28014/views/controls/textfield/te...
views/controls/textfield/textfield.cc:392: SelectAll();
On 2011/11/09 17:11:08, Ben Goodger (Google) wrote:
> unit test?

Done.

[jennyz, 2011-11-10 18:08:33]

http://codereview.chromium.org/8368016/diff/34002/views/controls/textfield/te...
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/8368016/diff/34002/views/controls/textfield/te...
views/controls/textfield/textfield.cc:396: ClearSelection();
I still concern this may affect the
NativeTextfieldViews::OnMousePressed()/OnMouseRelease() behavior, since the code
after text_field_ might think this is a single click after all text is selected,
and it will clear the selection and just flash the mouse cursor.

[jennyz, 2011-11-10 18:27:06]

http://codereview.chromium.org/8368016/diff/34002/views/controls/textfield/te...
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/8368016/diff/34002/views/controls/textfield/te...
views/controls/textfield/textfield.cc:396: ClearSelection();
Sorry I missed some detail in previous comment.
I still concern this may affect the
NativeTextfieldViews::OnMousePressed()/OnMouseRelease() behavior, since the code
in NativeTextfieldViews::OnMousePress()
after text_field_->RequestFocus() might think this is a single click after all
text is selected,
and it will clear the selection and just flash the mouse cursor. 
On 2011/11/10 18:08:33, jennyz wrote:
 I still concern this may affect the
 NativeTextfieldViews::OnMousePressed()/OnMouseRelease() behavior, since the
code
 after text_field_->RequestFocus() in NativeTextfieldViews::OnMousePressed might
think this is a single click after all text is selected,
> and it will clear the selection and just flash the mouse cursor.

[msw, 2011-11-10 20:35:25]

I'm backing off on the textfield change. Currently the bookmark bubble only
select the entire title textfield contents when the bubble is first shown; not
on every textfield focus. I'll help Jenny with omnibox/textfield changes
seperately.

[msw, 2011-11-11 00:41:23]

Ben, PTAL as you have time; thanks!

[Ben Goodger (Google), 2011-11-11 16:54:08]

lgtm

fair nuff

[commit-bot: I haz the power, 2011-11-11 19:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/8368016/39002

[commit-bot: I haz the power, 2011-11-11 20:43:18]

Change committed as 109688