Show avatar icon on V2 app's frame

ash/frame/custom_frame_view_ash.cc

Index: ash/frame/custom_frame_view_ash.cc
diff --git a/ash/frame/custom_frame_view_ash.cc b/ash/frame/custom_frame_view_ash.cc
index 964020b8a48c9cbe0b3492e30bd5e3b5ef243ed6..31e6ac7368b755499923b6fa766d8409f3439787 100644
--- a/ash/frame/custom_frame_view_ash.cc
+++ b/ash/frame/custom_frame_view_ash.cc
@@ -9,7 +9,10 @@
 #include "ash/frame/caption_buttons/frame_maximize_button.h"
 #include "ash/frame/caption_buttons/frame_maximize_button_observer.h"
 #include "ash/frame/frame_border_hit_test_controller.h"
+#include "ash/frame/frame_util.h"
 #include "ash/frame/header_painter.h"
+#include "ash/session_state_delegate.h"
+#include "ash/shell.h"
 #include "ash/wm/immersive_fullscreen_controller.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_state_delegate.h"
@@ -22,9 +25,11 @@
 #include "ui/aura/window_observer.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_list.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/rect_conversions.h"
 #include "ui/gfx/size.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/native_widget_aura.h"
 #include "ui/views/widget/widget.h"
@@ -156,6 +161,11 @@ class CustomFrameViewAsh::HeaderView
   // Returns the view's minimum width.
   int GetMinimumWidth() const;
 
+  // Updates the avatar icon based on the current state
+  // of the SessionStateDelegate::ShouldShowAvatar and
+  // SessionStateDelegate::GetUserImage.

[pkotwicz, 2014/03/18 04:41:15]

How about: "Updates the avatar icon based on the current state of
SessionStateDelegate::ShouldShowAvatar() and of
SessionStateDelegate::GetUserImage()"

[Aside] I am unsure if it is worth mentioning the function names because it is
very easy for the comment to go stale. SessionStateDelegate::GetUserImage() is
called via GetAvatarImageForContext()

[oshima, 2014/03/18 14:57:03]

Do want to remove it then? (I'm fine with it)
The comment without them doesn't make much sense either.

[pkotwicz, 2014/03/18 15:29:44]

I can't think of a good comment so I am fine with no comment

+  void UpdateAvatarIcon();
+
   // views::View overrides:
   virtual void Layout() OVERRIDE;
   virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE;
@@ -164,6 +174,10 @@ class CustomFrameViewAsh::HeaderView
     return header_painter_.get();
   }
 
+  views::View* avatar_icon() const {
+    return avatar_icon_;
+  }
+
  private:
   // ImmersiveFullscreenController::Delegate overrides:
   virtual void OnImmersiveRevealStarted() OVERRIDE;
@@ -178,6 +192,8 @@ class CustomFrameViewAsh::HeaderView
   // The widget that the caption buttons act on.
   views::Widget* frame_;
 
+  views::ImageView* avatar_icon_;

[pkotwicz, 2014/03/18 04:41:15]

[Optional]: This might be clearer if you made |avatar_icon_| into a scoped_ptr
and used views::View::set_owned_by_client()

Nit: Move |avatar_icon_| below |header_painter_|. That way all of the child
views are together

[oshima, 2014/03/18 14:57:03]

Done.

+
   // Helper for painting the header.
   scoped_ptr<HeaderPainter> header_painter_;
 
@@ -200,6 +216,7 @@ class CustomFrameViewAsh::HeaderView
 
 CustomFrameViewAsh::HeaderView::HeaderView(views::Widget* frame)
     : frame_(frame),
+      avatar_icon_(NULL),
       header_painter_(new ash::HeaderPainter),
       caption_button_container_(NULL),
       maximize_bubble_(NULL),
@@ -255,6 +272,32 @@ int CustomFrameViewAsh::HeaderView::GetMinimumWidth() const {
   return header_painter_->GetMinimumHeaderWidth();
 }
 
+void CustomFrameViewAsh::HeaderView::UpdateAvatarIcon() {
+  SessionStateDelegate* delegate =
+      Shell::GetInstance()->session_state_delegate();
+  aura::Window* window = frame_->GetNativeView();
+  bool show = delegate->ShouldShowAvatar(window);
+  if (!show) {
+    if (!avatar_icon_)
+      return;
+    delete avatar_icon_;
+    avatar_icon_ = NULL;
+  } else {
+    gfx::ImageSkia image = GetAvatarImageForContext(
+        delegate->GetBrowserContextForWindow(window)).AsImageSkia();
+    DCHECK(!image.isNull());
+    if (!avatar_icon_) {
+      avatar_icon_ = new views::ImageView();
+      AddChildView(avatar_icon_);
+    }
+    avatar_icon_->SetImage(image);
+    avatar_icon_->SetBounds(0, 0, image.width(), image.height());

[pkotwicz, 2014/03/18 04:41:15]

Setting the bounds of |avatar_icon_| is unnecessary?

[oshima, 2014/03/18 14:57:03]

This was necessary in old patch set, but not any more. Removed.

+    avatar_icon_->SchedulePaint();

[pkotwicz, 2014/03/18 04:41:15]

Is calling SchedulePaint() necessary?
  SchedulePaint() is called as part of ImageView::SetImage() and
View::SetBoundsRect()

[oshima, 2014/03/18 14:57:03]

Thanks, I forgot to remove this after troubleshooting the layout issue. Removed.

+  }
+  header_painter_->UpdateWindowIcon(avatar_icon_, avatar_icon_->size());
+  Layout();
+}
+
 void CustomFrameViewAsh::HeaderView::Layout() {
   header_painter_->LayoutHeader();
   header_painter_->set_header_height(GetPreferredHeight());
@@ -405,6 +448,7 @@ CustomFrameViewAsh::CustomFrameViewAsh(views::Widget* frame)
         new CustomFrameViewAshWindowStateDelegate(
             window_state, this)));
   }

[pkotwicz, 2014/03/18 04:41:15]

Nit: Could UpdateAvatarIcon() be done as part of the HeaderView constructor?

[oshima, 2014/03/18 14:57:03]

Done.

+  header_view_->UpdateAvatarIcon();
 }
 
 CustomFrameViewAsh::~CustomFrameViewAsh() {
@@ -504,10 +548,20 @@ bool CustomFrameViewAsh::HitTestRect(const gfx::Rect& rect) const {
   return false;
 }
 
+void CustomFrameViewAsh::VisibilityChanged(views::View* starting_from,
+                                           bool is_visible) {
+  if (is_visible)
+    header_view_->UpdateAvatarIcon();
+}
+
 views::View* CustomFrameViewAsh::GetHeaderView() {
   return header_view_;
 }
 
+const views::View* CustomFrameViewAsh::GetAvatarIconViewForTest() const {
+  return header_view_->avatar_icon();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // CustomFrameViewAsh, private: