Show avatar menu from NTP4

Show avatar menu from NTP4

If the user clicks on the NTP login status we now show the avatar menu.

BUG=93179
TEST=Tested on Views. Mac build pending.


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

[sail, 2011-10-02 18:04:03]

jhawkins@chromium.org: webui
rsesek@chromium.org: cocoa
erg@chromium.org: gtk
pkasting@chromium.org:views

[Robert Sesek, 2011-10-03 14:55:57]

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_window.h
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_windo...
chrome/browser/ui/browser_window.h:356: // Shows the avatar bubble at the given
position inside |tab_contents|.
In what coordinate system is the point?

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_windo...
chrome/browser/ui/browser_window.h:357: virtual void
ShowAvatarBubble(TabContents* tab_contents, int x, int y) = 0;
Use a gfx::Point?

[arv (Not doing code reviews), 2011-10-03 17:06:09]

drive by

http://codereview.chromium.org/8110003/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.js:154:
chrome.send('loginContainerClicked', [rect.right, rect.bottom]);
Don't you want to send the entire rect here? RTL and lack of screen space seems
to require the whole rect.

[jennb, 2011-10-03 17:08:06]

drive by:

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:492: NOTIMPLEMENTED();
Could you change this to a comment saying not applicable to Panels because
Panels don't have "tabs", thus anything related to NTP would not apply to
panels? Save us a patch to change it later. Thanks.

[sail, 2011-10-03 17:32:48]

On 2011/10/03 17:08:06, jennb wrote:
> drive by:
> 
>
http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel.cc
> File chrome/browser/ui/panels/panel.cc (right):
> 
>
http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel....
> chrome/browser/ui/panels/panel.cc:492: NOTIMPLEMENTED();
> Could you change this to a comment saying not applicable to Panels because
> Panels don't have "tabs", thus anything related to NTP would not apply to
> panels? Save us a patch to change it later. Thanks.

Actually, this should be implemented by any UI that shows the new tab page. Will
the panel UI ever show the new tab page?

[jennb, 2011-10-03 17:40:25]

Panels will never show a new tab page. Just like pop-ups don't show the new
tab page.
Maybe you should even assert that function is never called and put in a
NOTREACHED()?

On Mon, Oct 3, 2011 at 10:32 AM, <sail@chromium.org> wrote:

> On 2011/10/03 17:08:06, jennb wrote:
>
>> drive by:
>>
>
>
> http://codereview.chromium.**org/8110003/diff/1/chrome/**
>
browser/ui/panels/panel.cc<http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel.cc>
>
>> File chrome/browser/ui/panels/**panel.cc (right):
>>
>
>
> http://codereview.chromium.**org/8110003/diff/1/chrome/**
>
browser/ui/panels/panel.cc#**newcode492<http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel.cc#newcode492>
>
>> chrome/browser/ui/panels/**panel.cc:492: NOTIMPLEMENTED();
>> Could you change this to a comment saying not applicable to Panels because
>> Panels don't have "tabs", thus anything related to NTP would not apply to
>> panels? Save us a patch to change it later. Thanks.
>>
>
> Actually, this should be implemented by any UI that shows the new tab page.
> Will
> the panel UI ever show the new tab page?
>
>
http://codereview.chromium.**org/8110003/<http://codereview.chromium.org/8110...
>

[Elliot Glaysher, 2011-10-03 18:25:01]

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/gtk/browser_w...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/gtk/browser_w...
chrome/browser/ui/gtk/browser_window_gtk.cc:2402: void
BrowserWindowGtk::ShowAvatarBubble(TabContents* tab_contents,
Move to right under CreateFindBar().

[sail, 2011-10-03 18:46:03]

Addressed review comments. Please take another look.

http://codereview.chromium.org/8110003/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.js:154:
chrome.send('loginContainerClicked', [rect.right, rect.bottom]);
On 2011/10/03 17:06:09, arv wrote:
> Don't you want to send the entire rect here? RTL and lack of screen space
seems
> to require the whole rect.

Done.
I've also filed http://crbug.com/98884 about the bubble not being positioned
correct. Fixing that will require changing the bubble view API which I'll do in
a separate review.

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_window.h
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_windo...
chrome/browser/ui/browser_window.h:356: // Shows the avatar bubble at the given
position inside |tab_contents|.
On 2011/10/03 14:55:57, rsesek wrote:
> In what coordinate system is the point?

Should be in tab_content's coordinate system. Added this to comments.

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_windo...
chrome/browser/ui/browser_window.h:357: virtual void
ShowAvatarBubble(TabContents* tab_contents, int x, int y) = 0;
On 2011/10/03 14:55:57, rsesek wrote:
> Use a gfx::Point?

Changed to gfx::Rect.

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/gtk/browser_w...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/gtk/browser_w...
chrome/browser/ui/gtk/browser_window_gtk.cc:2402: void
BrowserWindowGtk::ShowAvatarBubble(TabContents* tab_contents,
On 2011/10/03 18:25:01, Elliot Glaysher wrote:
> Move to right under CreateFindBar().

Done.

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:492: NOTIMPLEMENTED();
On 2011/10/03 17:08:07, jennb wrote:
> Could you change this to a comment saying not applicable to Panels because
> Panels don't have "tabs", thus anything related to NTP would not apply to
> panels? Save us a patch to change it later. Thanks. 

Done.

[Elliot Glaysher, 2011-10-03 19:26:46]

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_window.h
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_windo...
chrome/browser/ui/browser_window.h:357: virtual void
ShowAvatarBubble(TabContents* tab_contents, int x, int y) = 0;
On 2011/10/03 18:46:03, sail wrote:
> On 2011/10/03 14:55:57, rsesek wrote:
> > Use a gfx::Point?
> 
> Changed to gfx::Rect.

Why a Rect? The bubble size will depend on platform specific stuff.

[sail, 2011-10-03 19:28:35]

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_window.h
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/8110003/diff/1/chrome/browser/ui/browser_windo...
chrome/browser/ui/browser_window.h:357: virtual void
ShowAvatarBubble(TabContents* tab_contents, int x, int y) = 0;
On 2011/10/03 19:26:46, Elliot Glaysher wrote:
> On 2011/10/03 18:46:03, sail wrote:
> > On 2011/10/03 14:55:57, rsesek wrote:
> > > Use a gfx::Point?
> > 
> > Changed to gfx::Rect.
> 
> Why a Rect? The bubble size will depend on platform specific stuff.

The bubble position depends on the width and height of the object it's
positioning itself over.

[Robert Sesek, 2011-10-03 22:18:27]

c/b/u/cocoa LGTM

[Elliot Glaysher, 2011-10-03 22:20:37]

gtk lgtm then

On Mon, Oct 3, 2011 at 3:18 PM,  <rsesek@chromium.org> wrote:
> c/b/u/cocoa LGTM
>
> http://codereview.chromium.org/8110003/
>

[Peter Kasting, 2011-10-03 23:44:24]

views rubber-stamp LGTM

[sail, 2011-10-05 14:17:46]

jhawkins: need quick rubber stamp for DOMUI changes.

[James Hawkins, 2011-10-05 16:37:32]

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:84: CHECK(args->GetDouble(0,
&x));
Why are these CHECKs?

[sail, 2011-10-05 16:40:54]

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:84: CHECK(args->GetDouble(0,
&x));
On 2011/10/05 16:37:32, James Hawkins wrote:
> Why are these CHECKs?

I just copy pasted these from else where. Should I just remove the CHECK()s all
together?

[James Hawkins, 2011-10-05 16:43:53]

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:84: CHECK(args->GetDouble(0,
&x));
On 2011/10/05 16:40:54, sail wrote:
> On 2011/10/05 16:37:32, James Hawkins wrote:
> > Why are these CHECKs?
> 
> I just copy pasted these from else where. Should I just remove the CHECK()s
all
> together?

Yes. CHECK should only be used to debug problems in the wild.  If you want to
assert (debug) that the calls to GetDouble succeed:

if (!args->GetDouble(...) || !args->GetDouble(...) ||...)
  NOTREACHED();

[sail, 2011-10-05 16:58:31]

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8110003/diff/7003/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:84: CHECK(args->GetDouble(0,
&x));
On 2011/10/05 16:43:53, James Hawkins wrote:
> On 2011/10/05 16:40:54, sail wrote:
> > On 2011/10/05 16:37:32, James Hawkins wrote:
> > > Why are these CHECKs?
> > 
> > I just copy pasted these from else where. Should I just remove the CHECK()s
> all
> > together?
> 
> Yes. CHECK should only be used to debug problems in the wild.  If you want to
> assert (debug) that the calls to GetDouble succeed:
> 
> if (!args->GetDouble(...) || !args->GetDouble(...) ||...)
>   NOTREACHED();

Fixed by using DCHECK(success) instead.

[James Hawkins, 2011-10-05 17:56:09]

lgtm

[commit-bot: I haz the power, 2011-10-05 18:00:14]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8110003/17001

[commit-bot: I haz the power, 2011-10-05 18:00:17]

Can't apply patch for file chrome/browser/resources/ntp4/new_tab.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/ntp4/new_tab.js
Hunk #1 FAILED at 150.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/resources/ntp4/new_tab.js.rej

[commit-bot: I haz the power, 2011-10-06 05:33:55]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8110003/18001

[commit-bot: I haz the power, 2011-10-06 06:03:40]

Try job failure for 8110003-18001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-06 06:10:36]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8110003/20015

[commit-bot: I haz the power, 2011-10-06 07:42:23]

Try job failure for 8110003-20015 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-10-06 13:43:36]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8110003/24001

[commit-bot: I haz the power, 2011-10-06 15:17:36]

Change committed as 104300