Fix opaque frame popup titlebar not being painted.

Fix opaque frame popup titlebar not being painted.

R=pkasting@chromium.org,thomasanderson@chromium.org
BUG=695473, 670959
Review-Url: https://codereview.chromium.org/2741603002
Cr-Commit-Position: refs/heads/master@{#455885}
Committed: https://chromium.googlesource.com/chromium/src/+/683596645abbc42f0c20768c1579a578f5b36553

[Bret, 2017-03-09 01:18:27]

https://codereview.chromium.org/2741603002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2741603002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:514: ?
layout_->NonClientTopHeight(false)
this "false" is "calculate for restored"... it looks like the titlebar is always
laid out as restored (see OpaqueBrowserFrameViewLayout::LayoutTitleBar), which
is strange but I'm not going to interrogate it for this patch.

[Tom Anderson, 2017-03-09 01:55:03]

lgtm

I tested this patch locally on Linux and Gtk3 headerbars still render properly

[Peter Kasting, 2017-03-09 05:27:06]

Is there any way to unittest this?  Maybe not.

LGTM assuming you consider and conclusively accept/reject the suggestion below.

https://codereview.chromium.org/2741603002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2741603002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:514: ?
layout_->NonClientTopHeight(false)
On 2017/03/09 01:18:26, Bret Sepulveda wrote:
> this "false" is "calculate for restored"... it looks like the titlebar is
always
> laid out as restored (see OpaqueBrowserFrameViewLayout::LayoutTitleBar), which
> is strange but I'm not going to interrogate it for this patch.

I think maybe this should be std::max(frame_image.height(),
layout_->NonClientTopHeight(...)).

My worry here is that if a broken theme provides an image that is too short for
the tab area, we should probably draw the normal background behind it, so we
don't have black/unpainted gaps.  I think that's what using max() here would
accomplish.  I'm not 100% sure.

[Tom (Use chromium acct), 2017-03-09 20:12:22]

Description was changed from

==========
Fix opaque frame popup titlebar not being painted.

R=pkasting@chromium.org,thomasanderson@chromium.org
BUG=695473
==========

to

==========
Fix opaque frame popup titlebar not being painted.

R=pkasting@chromium.org,thomasanderson@chromium.org
BUG=695473,670959
==========

[Bret, 2017-03-09 21:47:49]

There's no test suite for OpaqueBrowserFrameView. There is one for the layout
manager, but this isn't the layout manager even though it's a layout issue...
There's probably a refactoring opportunity here somewhere.

https://codereview.chromium.org/2741603002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2741603002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:514: ?
layout_->NonClientTopHeight(false)
On 2017/03/09 05:27:06, Peter Kasting wrote:
> On 2017/03/09 01:18:26, Bret Sepulveda wrote:
> > this "false" is "calculate for restored"... it looks like the titlebar is
> always
> > laid out as restored (see OpaqueBrowserFrameViewLayout::LayoutTitleBar),
which
> > is strange but I'm not going to interrogate it for this patch.
> 
> I think maybe this should be std::max(frame_image.height(),
> layout_->NonClientTopHeight(...)).
> 
> My worry here is that if a broken theme provides an image that is too short
for
> the tab area, we should probably draw the normal background behind it, so we
> don't have black/unpainted gaps.  I think that's what using max() here would
> accomplish.  I'm not 100% sure.

You're right, that's probably safer. Done.

[Bret, 2017-03-09 21:48:27]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-03-09 21:48:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
thomasanderson@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2741603002/#ps20001
(title: "use max")

[commit-bot: I haz the power, 2017-03-09 21:49:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 22:31:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489096107530560,
"parent_rev": "9f440279ce353aa0602999ac88e6d7236355324f", "commit_rev":
"683596645abbc42f0c20768c1579a578f5b36553"}

[commit-bot: I haz the power, 2017-03-09 22:32:01]

Description was changed from

==========
Fix opaque frame popup titlebar not being painted.

R=pkasting@chromium.org,thomasanderson@chromium.org
BUG=695473,670959
==========

to

==========
Fix opaque frame popup titlebar not being painted.

R=pkasting@chromium.org,thomasanderson@chromium.org
BUG=695473,670959

Review-Url: https://codereview.chromium.org/2741603002
Cr-Commit-Position: refs/heads/master@{#455885}
Committed:
https://chromium.googlesource.com/chromium/src/+/683596645abbc42f0c20768c1579...
==========

[commit-bot: I haz the power, 2017-03-09 22:32:01]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/683596645abbc42f0c20768c1579...