Refactor DoesIntersectRect into BrowserNonClientFrameView.

Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check tab strip, toolbar, and window controls to make sure still work correctly.

Committed: https://crrev.com/55e6cc560a8e152d26f1b69e0929c53681eeacd7
Cr-Commit-Position: refs/heads/master@{#413514}

[Bret, 2016-08-19 00:19:55]

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

[commit-bot: I haz the power, 2016-08-19 00:20:35]

Dry run: 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, 2016-08-19 01:10:02]

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

[commit-bot: I haz the power, 2016-08-19 01:10:03]

Dry run: This issue passed the CQ dry run.

[Bret, 2016-08-19 18:06:45]

Description was changed from

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should be different.

TEST=Check all UI, but especially tab strip and window controls.
==========

to

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check all UI, but especially tab strip and window controls.
==========

[Bret, 2016-08-19 20:44:24]

Description was changed from

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check all UI, but especially tab strip and window controls.
==========

to

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check tab strip, toolbar, and window controls to make sure still work
correctly.
==========

[Bret, 2016-08-19 20:44:24]

bsep@chromium.org changed reviewers:
+ msw@chromium.org, sky@chromium.org

[msw, 2016-08-19 23:56:40]

lgtm with nits; but please wait for Scott's review.

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

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:213: const
gfx::Rect& rect) const {
nit: indentation. I see other formatting issues; please run 'git cl format'

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:216: // |rect| is
outside OpaqueBrowserFrameView's bounds.
nit: s/OpaqueBrowserFrameView/BrowserNonClientFrameView/

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:220: // If the
rect is outside the bounds of the client area, claim it.
Neither Ash or Mus did this previously; I suspect the difference is that the
desktop opaque frame impl supports non-client targeting for clicks on the window
left/right/bottom edges, while ash/chromeos doesn't have any pixels for those
edges. I guess it seems okay to support this in all subclasses, even if they
don't have a use for it; if anything it's a small amount of wasted computation.
Still, Scott might catch something I'm missing here, or suggest putting this in
an #if !defined(USE_ASH) or similar.

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:239: if
(rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) {
nit: consider using the other impl here:
  return (rect_in_tabstrip_coords.y() <= tabstrip->height()) &&
          (!tabstrip->HitTestRect(rect_in_tabstrip_coords) ||
           tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords));

[sky, 2016-08-20 20:17:10]

LGTM

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

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:214:
CHECK_EQ(target, this);
Normally we use DCHECK for these sort of checks.

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:220: // If the
rect is outside the bounds of the client area, claim it.
On 2016/08/19 23:56:40, msw wrote:
> Neither Ash or Mus did this previously; I suspect the difference is that the
> desktop opaque frame impl supports non-client targeting for clicks on the
window
> left/right/bottom edges, while ash/chromeos doesn't have any pixels for those
> edges. I guess it seems okay to support this in all subclasses, even if they
> don't have a use for it; if anything it's a small amount of wasted
computation.
> Still, Scott might catch something I'm missing here, or suggest putting this
in
> an #if !defined(USE_ASH) or similar.

For ash this doesn't matter as I believe the client area fills the entire bounds
(so this is always true), and for mus it should be ok.

[Bret, 2016-08-22 18:29:42]

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

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:213: const
gfx::Rect& rect) const {
On 2016/08/19 23:56:40, msw wrote:
> nit: indentation. I see other formatting issues; please run 'git cl format'

Done.

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:214:
CHECK_EQ(target, this);
On 2016/08/20 20:17:10, sky wrote:
> Normally we use DCHECK for these sort of checks.

Done.

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:216: // |rect| is
outside OpaqueBrowserFrameView's bounds.
On 2016/08/19 23:56:40, msw wrote:
> nit: s/OpaqueBrowserFrameView/BrowserNonClientFrameView/

Done.

https://codereview.chromium.org/2262563002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:239: if
(rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) {
On 2016/08/19 23:56:40, msw wrote:
> nit: consider using the other impl here:
>   return (rect_in_tabstrip_coords.y() <= tabstrip->height()) &&
>           (!tabstrip->HitTestRect(rect_in_tabstrip_coords) ||
>            tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords));

I like this one better, it's a lot more readable imo.

[Bret, 2016-08-22 18:29:54]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-08-22 18:29:55]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2262563002/#ps20001
(title: "fix comments")

[commit-bot: I haz the power, 2016-08-22 18:33:33]

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, 2016-08-22 19:45:44]

Description was changed from

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check tab strip, toolbar, and window controls to make sure still work
correctly.
==========

to

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check tab strip, toolbar, and window controls to make sure still work
correctly.
==========

[commit-bot: I haz the power, 2016-08-22 19:45:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-22 19:48:18]

Description was changed from

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check tab strip, toolbar, and window controls to make sure still work
correctly.
==========

to

==========
Refactor DoesIntersectRect into BrowserNonClientFrameView.

As far as I can tell the DoesIntersectRect implementations of
OpaqueBrowserFrameView, BrowserNonClientFrameViewMus, and
BrowserNonClientFrameViewAsh are all behaviorly identical but entirely
duplicated. So I moved them all into their common base class. Nothing
should look or act differently after this change.

TEST=Check tab strip, toolbar, and window controls to make sure still work
correctly.

Committed: https://crrev.com/55e6cc560a8e152d26f1b69e0929c53681eeacd7
Cr-Commit-Position: refs/heads/master@{#413514}
==========

[commit-bot: I haz the power, 2016-08-22 19:48:20]

Patchset 2 (id:??) landed as
https://crrev.com/55e6cc560a8e152d26f1b69e0929c53681eeacd7
Cr-Commit-Position: refs/heads/master@{#413514}

[varkha, 2016-08-23 16:21:46]

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

[varkha, 2016-08-23 16:21:46]

This change seems to be breaking window controls on Chrome OS for V1 frames
(More Tools -> Add to shelf and check "Open as window"). Minimize / restore /
close buttons do not work at ToT and start working again if I locally revert
this patch.

[Bret, 2016-08-23 21:40:05]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2273863002/ by bsep@chromium.org.

The reason for reverting is: It looks like I broke V1 apps on Chrome OS (see
crbug.com/640230) which means it's probably broken for Mus as well (though it's
hard to tell) and that negates the benefit of this refactor. I'm very sad..

[sky, 2016-08-23 23:54:45]

Did you investigate what broke in your patch? I wouldn't think it that
hard to repro with a chromeos build.

On Tue, Aug 23, 2016 at 2:40 PM,  <bsep@chromium.org> wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2273863002/ by bsep@chromium.org.
>
> The reason for reverting is: It looks like I broke V1 apps on Chrome OS (see
> crbug.com/640230) which means it's probably broken for Mus as well (though
> it's
> hard to tell) and that negates the benefit of this refactor. I'm very sad..
>
> https://codereview.chromium.org/2262563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bret, 2016-08-23 23:57:32]

On 2016/08/23 23:54:45, sky wrote:
> Did you investigate what broke in your patch? I wouldn't think it that
> hard to repro with a chromeos build.

I don't have a way to build ChromeOS right now, so I reverted to take the time
pressure off while I set that up and finish the other thing I'm working on.

[sky, 2016-08-24 02:07:09]

Reverting right away is the right thing, was just curious if you had a
good handle on what might have went wrong. A chromeos build should
just be a matter of adding:

target_os = "chromeos"

to your args.gn

  -Scott

On Tue, Aug 23, 2016 at 4:57 PM,  <bsep@chromium.org> wrote:
> On 2016/08/23 23:54:45, sky wrote:
>> Did you investigate what broke in your patch? I wouldn't think it that
>> hard to repro with a chromeos build.
>
> I don't have a way to build ChromeOS right now, so I reverted to take the
> time
> pressure off while I set that up and finish the other thing I'm working on.
>
> https://codereview.chromium.org/2262563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.