Rebase PageInfoBubble on the new views bubble.

Rebase PageInfoBubble on the new views bubble.

Move ShowPageInfoBubble to browser_dialogs.h, nix page_info_window.h.
Add BubbleDelegateView::SizeToContents(), slight refactoring.
Add non-const location_icon_view(), make animation impls protected.

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

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

[msw, 2011-11-11 23:07:49]

PTAL; thanks!

[msw, 2011-11-14 23:58:31]

Ping! Ben, should I send these bubble conversion reviews to someone else? We'll
likely have a lot in the next two weeks to hit our deadline.

[msw, 2011-11-15 00:00:51]

Finnur, perhaps you can help review here?

[Ben Goodger (Google), 2011-11-15 00:08:44]

Sorry, when you add me as a reviewer with someone else to a CR, I assume
you just want OWNERS after the other person does the prelim :-)

-Ben

On Mon, Nov 14, 2011 at 4:00 PM, <msw@chromium.org> wrote:

> Finnur, perhaps you can help review here?
>
>
http://codereview.chromium.**org/8508048/<http://codereview.chromium.org/8508...
>

[msw, 2011-11-15 00:11:50]

Ah, gotcha. Alice, Finnur: can you take look first?

[alicet1, 2011-11-15 02:02:13]

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:1140:
browser::ShowPageInfoBubble(GetLocationBarView()->location_icon_view(),
would it be better that you expose a non-const version of the accessor in
LocationBarView, as in the case of 

  virtual const OmniboxView* location_entry() const OVERRIDE;
  virtual OmniboxView* location_entry() OVERRIDE;

I'm a bit weary about adding the const to the anchor view we stored instead, if
we ended up to const cast away, say, for anchor_view()->GetPreferredSize() or
some such.

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (left):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:311: string16
PageInfoBubbleView::GetAccessibleName() {
where did this go?

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:313: }
The bubble will be closed on deactivation. add a comment here bubble will be
closed?

[Finnur, 2011-11-15 10:05:01]

General comment: Looks like you need to sync to latest and try  the change
again.

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (left):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:319: bubble_->Close();
I remember this problem. Do you now guarantee that the bubble closes on all
platforms (specifically chrome-os) when a new tab is opened?

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:293: // bubble.
What's the status on this comment? Is there still a concern?

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:321: }
Out of curiosity... is there a specific reason for not passing AnimationEnded
down the chain in all cases? (same with AnimationProgressed)

http://codereview.chromium.org/8508048/diff/11001/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8508048/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:95: virtual void AnimationProgressed(const
ui::Animation* animation);
Why not OVERRIDE?

[msw, 2011-11-15 20:16:57]

Alice, Finnur: Comments addressed; merged to ToT. PTAL; thanks!

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:1140:
browser::ShowPageInfoBubble(GetLocationBarView()->location_icon_view(),
On 2011/11/15 02:02:13, alicet1 wrote:
> would it be better that you expose a non-const version of the accessor in
> LocationBarView, as in the case of 
> 
>   virtual const OmniboxView* location_entry() const OVERRIDE;
>   virtual OmniboxView* location_entry() OVERRIDE;
> 
> I'm a bit weary about adding the const to the anchor view we stored instead,
if
> we ended up to const cast away, say, for anchor_view()->GetPreferredSize() or
> some such. 

Done.

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (left):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:311: string16
PageInfoBubbleView::GetAccessibleName() {
On 2011/11/15 02:02:13, alicet1 wrote:
> where did this go?

GetAccessibleName was only ever used on the Windows version of the old bubbles
to call SetWindowText, which "Changes the text of the specified window's title
bar (if it has one)." Bubbles do not have a title bar, so I'm removing this code
as I did for the BookmarkBubble.

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:319: bubble_->Close();
On 2011/11/15 10:05:01, Finnur wrote:
> I remember this problem. Do you now guarantee that the bubble closes on all
> platforms (specifically chrome-os) when a new tab is opened?

This bubble does indeed close on deactivate on my local CrOS builds, as Alice
mentioned. We can re-introduce this workaround if there is a regression.

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:293: // bubble.
On 2011/11/15 10:05:01, Finnur wrote:
> What's the status on this comment? Is there still a concern?

Just tested; the jank is still there; leaving this for another day...

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:313: }
On 2011/11/15 02:02:13, alicet1 wrote:
> The bubble will be closed on deactivation. add a comment here bubble will be
> closed?

Done.

http://codereview.chromium.org/8508048/diff/11001/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:321: }
On 2011/11/15 10:05:01, Finnur wrote:
> Out of curiosity... is there a specific reason for not passing AnimationEnded
> down the chain in all cases? (same with AnimationProgressed)

BubbleDelegateView was DCHECKing that it only gets called for its
fade_animation_. I've changed that to simply no-op on other animations, and I'm
passing this one through now.

http://codereview.chromium.org/8508048/diff/11001/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8508048/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:95: virtual void AnimationProgressed(const
ui::Animation* animation);
On 2011/11/15 10:05:01, Finnur wrote:
> Why not OVERRIDE?

Done.

[alicet1, 2011-11-15 21:52:23]

lgtm

[msw, 2011-11-15 22:15:59]

Ben: PTAL; thanks!

[msw, 2011-11-16 22:08:10]

Ping! Ben, can you review/rubber stamp?

[Ben Goodger (Google), 2011-11-16 22:26:38]

lgtm

http://codereview.chromium.org/8508048/diff/24014/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/8508048/diff/24014/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:135: anchor_view() ?
anchor_view()->GetWidget()->GetNativeWindow() : NULL;
nit: 4-space indent

[msw, 2011-11-16 22:37:52]

Thanks! Landing.

http://codereview.chromium.org/8508048/diff/24014/chrome/browser/ui/views/pag...
File chrome/browser/ui/views/page_info_bubble_view.cc (right):

http://codereview.chromium.org/8508048/diff/24014/chrome/browser/ui/views/pag...
chrome/browser/ui/views/page_info_bubble_view.cc:135: anchor_view() ?
anchor_view()->GetWidget()->GetNativeWindow() : NULL;
On 2011/11/16 22:26:38, Ben Goodger (Google) wrote:
> nit: 4-space indent

Done.

[commit-bot: I haz the power, 2011-11-16 22:38:10]

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

[commit-bot: I haz the power, 2011-11-17 00:48:35]

Change committed as 110403