Fixes BrowserViewHostedAppTest.Layout when using MD

Fixes BrowserViewHostedAppTest.Layout when using MD

Find bar is pushed further above the content then the test expeected.
Also changes the layout to use LayoutConstant(FIND_BAR_VERTICAL_OFFSET).

BUG=538629
Committed: https://crrev.com/0f769dc9fff8917d568c2d43288248eadb110586
Cr-Commit-Position: refs/heads/master@{#353758}

[varkha, 2015-10-09 17:54:06]

Patchset #1 (id:1) has been deleted

[varkha, 2015-10-09 17:55:34]

varkha@chromium.org changed reviewers:
+ tdanderson@chromium.org

[varkha, 2015-10-09 17:55:35]

tdanderson@, can you please take a look?
Do you think I should hardcode expectations in the test or is using
GetLayoutConstant(FIND_BAR_VERTICAL_OFFSET) OK in the test?

[varkha, 2015-10-09 17:55:41]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 17:57:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1395193002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1395193002/20001

[tdanderson, 2015-10-09 18:12:09]

Looks good, and I definitely prefer the use of the layout constant instead of
hard-coded values.

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_layout.cc:230: // With Material
Design the find bar is pushed a bit higher still.
nit: I would word this as

"Position the find bar above the bottom of the top container so that it occludes
the border between the content area and the top container. The find bar should
look connected to the top container when material design is not enabled."

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_unittest.cc (right):

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_unittest.cc:203: // bottom of the
header or be a few pixels higher still with Material Design.
nit: I don't think changing the comment here is necessary.

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_constants.h (right):

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_constants.h:79: FIND_BAR_VERTICAL_OFFSET,
nit: keep in alphabetical order (here and in the .cc)

[varkha, 2015-10-09 18:20:36]

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_layout.cc:230: // With Material
Design the find bar is pushed a bit higher still.
On 2015/10/09 18:12:09, tdanderson wrote:
> nit: I would word this as
> 
> "Position the find bar above the bottom of the top container so that it
occludes
> the border between the content area and the top container. The find bar should
> look connected to the top container when material design is not enabled."

Done.

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_unittest.cc (right):

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_unittest.cc:203: // bottom of the
header or be a few pixels higher still with Material Design.
On 2015/10/09 18:12:09, tdanderson wrote:
> nit: I don't think changing the comment here is necessary.

Done.

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_constants.h (right):

https://codereview.chromium.org/1395193002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_constants.h:79: FIND_BAR_VERTICAL_OFFSET,
On 2015/10/09 18:12:09, tdanderson wrote:
> nit: keep in alphabetical order (here and in the .cc)

Done.

[Evan Stade, 2015-10-09 18:39:12]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-10-09 18:39:12]

as the person responsible for this breakage, thanks and lgtm aside from one
comment

https://codereview.chromium.org/1395193002/diff/40001/ui/base/resource/materi...
File ui/base/resource/material_design/material_design_controller.cc (left):

https://codereview.chromium.org/1395193002/diff/40001/ui/base/resource/materi...
ui/base/resource/material_design/material_design_controller.cc:49:
SetMode(Mode::NON_MATERIAL);
why this change? seems unrelated. is this for debugging?

[varkha, 2015-10-09 18:52:52]

varkha@chromium.org changed reviewers:
+ pkasting@chromium.org

[varkha, 2015-10-09 18:52:53]

+pkasting for OWNERS in chrome/browser/ui/views/.

https://codereview.chromium.org/1395193002/diff/40001/ui/base/resource/materi...
File ui/base/resource/material_design/material_design_controller.cc (left):

https://codereview.chromium.org/1395193002/diff/40001/ui/base/resource/materi...
ui/base/resource/material_design/material_design_controller.cc:49:
SetMode(Mode::NON_MATERIAL);
On 2015/10/09 18:39:12, Evan Stade wrote:
> why this change? seems unrelated. is this for debugging?

Yes, this change (and the one in switches.cc) will be removed before I land
this.
Thanks!

[Peter Kasting, 2015-10-09 22:15:06]

LGTM

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_layout.cc:232:
GetLayoutConstant(FIND_BAR_VERTICAL_OFFSET);
It seems like this is more an "overlap" value than an "offset"?  Maybe
"FIND_BAR_TOOLBAR_OVERLAP"?  (I realize "toolbar" is a little overly-specific,
but it's roughly accurate, and "TOP_CONTAINER" is too vague in
layout_constants.h.)

Then maybe the comment can just be "Overlap the find bar atop |top_container_|."

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_unittest.cc (right):

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_unittest.cc:205:
GetLayoutConstant(FIND_BAR_VERTICAL_OFFSET),
I would probably avoid checking this exact value, and instead check that the
find bar y is less than the frame top inset.

[varkha, 2015-10-13 14:51:13]

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_layout.cc:232:
GetLayoutConstant(FIND_BAR_VERTICAL_OFFSET);
On 2015/10/09 22:15:06, Peter Kasting wrote:
> It seems like this is more an "overlap" value than an "offset"?  Maybe
> "FIND_BAR_TOOLBAR_OVERLAP"?  (I realize "toolbar" is a little overly-specific,
> but it's roughly accurate, and "TOP_CONTAINER" is too vague in
> layout_constants.h.)
> 
> Then maybe the comment can just be "Overlap the find bar atop
|top_container_|."

Done. I've left the part about connected look when MD is not enabled, that
seemed like an important detail for now that is not easily read from code.

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_unittest.cc (right):

https://codereview.chromium.org/1395193002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_unittest.cc:205:
GetLayoutConstant(FIND_BAR_VERTICAL_OFFSET),
On 2015/10/09 22:15:06, Peter Kasting wrote:
> I would probably avoid checking this exact value, and instead check that the
> find bar y is less than the frame top inset.

Done.

[varkha, 2015-10-13 14:51:20]

The CQ bit was checked by varkha@chromium.org

[varkha, 2015-10-13 14:51:21]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1395193002/#ps80001
(title: "Fixes BrowserViewHostedAppTest.Layout when using MD (post-LGTM
comments)")

[commit-bot: I haz the power, 2015-10-13 14:55:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1395193002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1395193002/80001

[commit-bot: I haz the power, 2015-10-13 15:45:30]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-13 15:46:19]

Patchset 4 (id:??) landed as
https://crrev.com/0f769dc9fff8917d568c2d43288248eadb110586
Cr-Commit-Position: refs/heads/master@{#353758}