Make activities have a thick border when in overview mode part 1

Make activities have a thick border when in overview mode part 1

BUG=401559
TEST=Manual, see bug
TBR=sadrul, oshima (TBR for DEPS additions)
NOTRY=true

Committed: https://crrev.com/bc021eeb04dc604001e761ecb6451955e694a50a
Cr-Commit-Position: refs/heads/master@{#293620}

[pkotwicz, 2014-08-30 00:56:37]

pkotwicz@chromium.org changed reviewers:
+ sadrul@chromium.org

[pkotwicz, 2014-08-30 00:56:38]

Sadrul, can you please take a look?

This CL is a subset of https://codereview.chromium.org/479533003/ I split it up
for the sake of expediency

https://codereview.chromium.org/518673007/diff/1/athena/activity/activity_fra...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/1/athena/activity/activity_fra...
athena/activity/activity_frame_view.cc:63: AddChildView(icon_);
I still add |icon_| as a child right away but only set it as visible when in
overview mode

https://codereview.chromium.org/518673007/diff/1/athena/activity/activity_fra...
athena/activity/activity_frame_view.cc:116: Layout();
Yes, this is necessary. |title_| no longer takes up the entire width of the
non-client frame view so it needs to be repositioned each time the text changes.

https://codereview.chromium.org/518673007/diff/1/athena/activity/activity_fra...
athena/activity/activity_frame_view.cc:162: gfx::Rect border_bounds =
GetLocalBounds();
In the old code |title_| had a bottom border. This matches that. From
looking at the mocks, I think that the header is supposed to have a bottom
border when not in overview mode

[sadrul, 2014-09-02 15:38:18]

Can you address the comments from the earlier CL?

[pkotwicz, 2014-09-02 20:37:26]

I believe I addressed the subset of comments
https://codereview.chromium.org/479533003/ which are relevant to this CL in
message #2 of this CL. I just went through the comments on the other CL and did
not find any relevant comments that I did not address

[sadrul, 2014-09-03 16:10:18]

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:63: AddChildView(icon_);
Add |icon_| only when entering the overview mode,

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:94: HTCAPTION : client_hit_test;
This still doesn't look right. We should return HTCAPTION only for the window
title (and maybe for the icon, but we don't use hit-test in overview mode)

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:104: void
ActivityFrameView::UpdateWindowIcon() {
Update icon_ here?
(https://codereview.chromium.org/479533003/diff/340001/athena/activity/activit...)

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:155: preferred_title_size.height());
Consider using a BoxLayout instead. (in the box layout, set flex = 1 for title_,
and 0 for icon_). When going into overview mode, left-align title_, and reset to
center-align when going out of overview mode. Would that work?

https://codereview.chromium.org/518673007/diff/40001/athena/resource_manager/...
File athena/resource_manager/resource_manager_unittest.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/resource_manager/...
athena/resource_manager/resource_manager_unittest.cc:55: virtual void
PrepareContentsForOverview() OVERRIDE { }
{}

https://codereview.chromium.org/518673007/diff/40001/athena/test/sample_activ...
File athena/test/sample_activity.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/test/sample_activ...
athena/test/sample_activity.cc:88: 
Same order as in the header.

[pkotwicz, 2014-09-03 19:40:59]

Sadrul, can you please take another look?

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:94: HTCAPTION : client_hit_test;
You're right, I did not take into account the left, right and bottom borders in
overview mode.

I am going to keep returning HCAPTION for the window icon. This is similar to
what we do with BrowserNonClientFrameViewAsh::window_icon_

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:104: void
ActivityFrameView::UpdateWindowIcon() {
Yes, when we have a real icon. Currently, |icon_| is a blue bitmap.

Aside: It may be possible for the activity's representative color to change
independant of either the title or the icon changing so we will have to deal
with that.

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:155: preferred_title_size.height());
I think that |icon_| and |title_| would need to be children of a container view
which covers the header portion (as opposed to the entire NonClientFrameView)
for using BoxLayout to work. I doubt that using BoxLayout will decrease line
count / complexity in this case.

Thanks for suggesting BoxLayout though.

https://codereview.chromium.org/518673007/diff/40001/athena/resource_manager/...
File athena/resource_manager/resource_manager_unittest.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/resource_manager/...
athena/resource_manager/resource_manager_unittest.cc:55: virtual void
PrepareContentsForOverview() OVERRIDE { }
On 2014/09/03 16:10:18, sadrul wrote:
> {}

Done.

https://codereview.chromium.org/518673007/diff/40001/athena/test/sample_activ...
File athena/test/sample_activity.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/test/sample_activ...
athena/test/sample_activity.cc:88: 
Fixed order of methods in sample_activity.h (and in a couple of other places
too)

[sadrul, 2014-09-03 20:05:07]

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:63: AddChildView(icon_);
On 2014/09/03 16:10:18, sadrul wrote:
> Add |icon_| only when entering the overview mode,

ping?

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:104: void
ActivityFrameView::UpdateWindowIcon() {
On 2014/09/03 19:40:59, pkotwicz wrote:
> Yes, when we have a real icon. Currently, |icon_| is a blue bitmap.
> 
> Aside: It may be possible for the activity's representative color to change
> independant of either the title or the icon changing so we will have to deal
> with that.

Ah, I see. It would be a good idea to plumb the icon from FaviconStatus through
the AcitivityViewModel (so we would get the icon when running from chrome?). But
we can do that in a separate CL.

[pkotwicz, 2014-09-03 20:15:45]

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:63: AddChildView(icon_);
Sorry for missing your comment

I believe that your initial concern was "Instead of always having |icon_| with
zero opacity, it'd be better to add it
only when in overview mode."
I believe I have addressed this concern by toggling |icon_|'s visibility based
on overview mode. I think that it is ok to always keep |icon_| in the view
hierarchy.

[pkotwicz, 2014-09-03 20:15:46]

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:63: AddChildView(icon_);
Sorry for missing your comment

I believe that your initial concern was "Instead of always having |icon_| with
zero opacity, it'd be better to add it
only when in overview mode."
I believe I have addressed this concern by toggling |icon_|'s visibility based
on overview mode. I think that it is ok to always keep |icon_| in the view
hierarchy.

[sadrul, 2014-09-03 20:28:43]

lgtm

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/40001/athena/activity/activity...
athena/activity/activity_frame_view.cc:63: AddChildView(icon_);
On 2014/09/03 20:15:45, pkotwicz wrote:
> Sorry for missing your comment
> 
> I believe that your initial concern was "Instead of always having |icon_| with
> zero opacity, it'd be better to add it
> only when in overview mode."
> I believe I have addressed this concern by toggling |icon_|'s visibility based
> on overview mode. I think that it is ok to always keep |icon_| in the view
> hierarchy.

Sounds good.

[pkotwicz, 2014-09-03 20:30:45]

pkotwicz@chromium.org changed reviewers:
+ oshima@chromium.org

[pkotwicz, 2014-09-03 20:30:46]

oshima for OWNERS

[oshima, 2014-09-03 22:36:12]

lgtm with nits and questions

https://codereview.chromium.org/518673007/diff/120001/athena/activity/activit...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/120001/athena/activity/activit...
athena/activity/activity_frame_view.cc:96: GetHTComponentForFrame(point, 0,
NonClientBorderThickness(), 0, 0, false);
just q: How are we going to use HTBOTTOM/LEFT etc in overview mode?

https://codereview.chromium.org/518673007/diff/120001/athena/activity/activit...
athena/activity/activity_frame_view.cc:97: return (window_hit_test) == HTNOWHERE
? HTCAPTION : client_hit_test;
nuke ()

https://codereview.chromium.org/518673007/diff/120001/athena/activity/activit...
athena/activity/activity_frame_view.cc:167: canvas->DrawRect(border_bounds,
SkColorSetA(SK_ColorGRAY, 0x7f));
can you document these operations?

https://codereview.chromium.org/518673007/diff/120001/athena/content/app_acti...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/518673007/diff/120001/athena/content/app_acti...
athena/content/app_activity.cc:144: web_view_->SetFastResize(true);
I wonder if this should be default on athena. When would be normal resize be
preferable on athena?

[pkotwicz, 2014-09-05 16:31:43]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2014-09-05 16:31:59]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-05 16:38:50]

https://codereview.chromium.org/518673007/diff/120001/athena/activity/activit...
File athena/activity/activity_frame_view.cc (right):

https://codereview.chromium.org/518673007/diff/120001/athena/activity/activit...
athena/activity/activity_frame_view.cc:96: GetHTComponentForFrame(point, 0,
NonClientBorderThickness(), 0, 0, false);
I do not think that we will use them. Consistency with other code is good though

https://codereview.chromium.org/518673007/diff/120001/athena/content/app_acti...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/518673007/diff/120001/athena/content/app_acti...
athena/content/app_activity.cc:144: web_view_->SetFastResize(true);
This should not be the default. Fast resize does not resize the window but clips
it instead. We want to actually resize the window when putting it into split
view.

[pkotwicz, 2014-09-05 16:38:54]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:40:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/518673007/160001

[commit-bot: I haz the power, 2014-09-05 16:50:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:50:43]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/52220)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/57060)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pkotwicz, 2014-09-05 17:48:20]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-05 17:50:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/518673007/180001

[commit-bot: I haz the power, 2014-09-05 18:03:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:03:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2014-09-05 20:19:29]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-05 20:46:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/518673007/180001

[pkotwicz, 2014-09-06 04:08:22]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-06 04:08:22]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-06 04:09:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/518673007/180001

[commit-bot: I haz the power, 2014-09-06 11:55:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-06 11:55:19]

Exceeded time limit waiting for builds to trigger.

[pkotwicz, 2014-09-06 18:27:37]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-06 18:28:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/518673007/200001

[pkotwicz, 2014-09-06 18:31:24]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-06 18:31:26]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-06 18:34:53]

Committed patchset #11 (id:200001) as d5cafc4f172d5cea897161b2ca5cad04b8dbdf80

[commit-bot: I haz the power, 2014-09-10 03:42:47]

Patchset 11 (id:??) landed as
https://crrev.com/bc021eeb04dc604001e761ecb6451955e694a50a
Cr-Commit-Position: refs/heads/master@{#293620}