Allow Uber Tray popup pointing to the related tray item view.

Allow Uber Tray popup pointing to the related tray item view.


BUG=124636, 124637
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137440

[sadrul, 2012-05-14 21:30:34]

Because there is also crbug 124637, I was hoping we could use the same approach
to fix both of these bugs. The approach I had in mind:

 * SystemTray creates the bubble, and gives it an integer offset for the arrow
location. For mouse-clicks (i.e. 124637), the offset can be calculated easily.
For the other cases, SystemTray will compute the offset for the correct item in
the tray.
 * Instead of adding SystemTrayItem::GetTrayView, SystemTray will keep a
map<SystemTrayItem, View*>, and thus it can easily compute the arrow-offset
without having to go through the SystemTrayItem.
 * SystemTray sends the computed offset to SystemTrayBubble, possibly via
InitView (perhaps it makes sense to have STB::InitParams (like
views::Widget::InitParams) instead of just adding another parameter to
InitView). STB then sets the correct offset to the STBBorder it creates.

I think this would fix both of the bugs at the same time. What do you think?

[Jun Mukai, 2012-05-15 01:28:48]

On 2012/05/14 21:30:34, sadrul wrote:
> Because there is also crbug 124637, I was hoping we could use the same
approach
> to fix both of these bugs. The approach I had in mind:
> 
>  * SystemTray creates the bubble, and gives it an integer offset for the arrow
> location. For mouse-clicks (i.e. 124637), the offset can be calculated easily.
> For the other cases, SystemTray will compute the offset for the correct item
in
> the tray.
>  * Instead of adding SystemTrayItem::GetTrayView, SystemTray will keep a
> map<SystemTrayItem, View*>, and thus it can easily compute the arrow-offset
> without having to go through the SystemTrayItem.

Done.

>  * SystemTray sends the computed offset to SystemTrayBubble, possibly via
> InitView (perhaps it makes sense to have STB::InitParams (like
> views::Widget::InitParams) instead of just adding another parameter to
> InitView). STB then sets the correct offset to the STBBorder it creates.
> 

Done.

> I think this would fix both of the bugs at the same time. What do you think?

Yeah, I added the fix for 124637 too.

[sadrul, 2012-05-15 02:18:15]

This is looking very good. Thanks! Left a few comments:

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:258: void
SystemTray::ShowDefaultViewWithOffset(int arrow_offset) {
The order of the functions in the .cc file should match the order in the .h file

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:356: // want to guess the offset from the
position of its parent.
I guess this happens when the tray-view for an item is invisible (e.g. for
caps-lock when it's turned off)?

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:221: }
I am not sure I understand this. What does this do?

I was thinking you would only need to change how left_base_x is computed:

  left_base_x = IsRTL() ? kArrowWidth + arrow_offset :
                          owner_->width() - kArrowWidth - arrow_offset;

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:339: arrow_offset(-1) {
Initialize offset to kArrowPaddingFromRight

[Jun Mukai, 2012-05-15 02:43:05]

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:258: void
SystemTray::ShowDefaultViewWithOffset(int arrow_offset) {
On 2012/05/15 02:18:15, sadrul wrote:
> The order of the functions in the .cc file should match the order in the .h
file

Done.

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:356: // want to guess the offset from the
position of its parent.
On 2012/05/15 02:18:15, sadrul wrote:
> I guess this happens when the tray-view for an item is invisible (e.g. for
> caps-lock when it's turned off)?

No... It happens first time the user presses caps lock.  At that time, the view
bounds is not yet calculated, so the bounds is (0, 0), 0x0.
Added the comment more to describe this situation.

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:221: }
On 2012/05/15 02:18:15, sadrul wrote:
> I am not sure I understand this. What does this do?
> 
> I was thinking you would only need to change how left_base_x is computed:
> 
>   left_base_x = IsRTL() ? kArrowWidth + arrow_offset :
>                           owner_->width() - kArrowWidth - arrow_offset;

Done.

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:339: arrow_offset(-1) {
On 2012/05/15 02:18:15, sadrul wrote:
> Initialize offset to kArrowPaddingFromRight

Done.

[sadrul, 2012-05-15 02:58:16]

LGTM

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10381132/diff/1018/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:356: // want to guess the offset from the
position of its parent.
On 2012/05/15 02:43:05, Jun Mukai wrote:
> On 2012/05/15 02:18:15, sadrul wrote:
> > I guess this happens when the tray-view for an item is invisible (e.g. for
> > caps-lock when it's turned off)?
> 
> No... It happens first time the user presses caps lock.  At that time, the
view
> bounds is not yet calculated, so the bounds is (0, 0), 0x0.
> Added the comment more to describe this situation.

Ah, cool. Thanks for explaining.

[Jun Mukai, 2012-05-15 18:16:43]

rebase and I noticed a mistake on the calculation for arrow position --
arrow_offset_ should be specified for the position of the tip of the arrow but
left_base is used.  The new patchset contains this fix too.