Make the edge where BackgroundGradientView draws a divider selectable.

Make the edge where BackgroundGradientView draws a divider selectable.

…and remove the duplicate divider drawing in DownloadShelfView.

BUG=589943
Committed: https://crrev.com/1e6e9e83fdaca6b8021fa4372c77c54463ef339f
Cr-Commit-Position: refs/heads/master@{#418641}

[Sidney San Martín, 2016-09-12 12:00:08]

Description was changed from

==========
BackgroundGradientView: choice of a top or bottom divider

This replaces the divider implementation in DownloadShelfView.

BUG=589943
==========

to

==========
BackgroundGradientView: choice of top or bottom divider

This replaces the divider implementation in DownloadShelfView.

BUG=589943
==========

[Sidney San Martín, 2016-09-12 12:00:08]

sdy@chromium.org changed reviewers:
+ thakis@chromium.org

[Sidney San Martín, 2016-09-12 12:01:42]

Description was changed from

==========
BackgroundGradientView: choice of top or bottom divider

This replaces the divider implementation in DownloadShelfView.

BUG=589943
==========

to

==========
BackgroundGradientView: choice of top or bottom divider

This replaces the duplicate divider drawing in DownloadShelfView.

BUG=589943
==========

[Nico, 2016-09-12 13:45:55]

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
File chrome/browser/ui/cocoa/background_gradient_view.h (right):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
chrome/browser/ui/cocoa/background_gradient_view.h:12: enum class
BackgroundGradientViewDivider {
why enum class? can we come up with something shorter than
`BackgroundGradientViewDivider::Bottom` too?

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
File chrome/browser/ui/cocoa/background_gradient_view.mm (left):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
chrome/browser/ui/cocoa/background_gradient_view.mm:39: showsDivider_ = YES;
This used to default to divider-on, now it defaults to divider-off, no?

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
File chrome/browser/ui/cocoa/background_gradient_view.mm (right):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
chrome/browser/ui/cocoa/background_gradient_view.mm:65: }
These 7 lines are shorter as

  if (divider_ == BackgroundGradientViewDivider::Top)
    borderRect.origin.y = NSHeight(self.bounds) - lineWidth;

But I'd probably leave the previous code and tweak it like so:

  NSDivideRect([self bounds], &borderRect, &contentRect, [self cr_lineWidth],
    divider_ == BackgroundGradientViewDivider::Top ? NSMaxYEdge : NSMinYEdge);

And if you don't make up your own enum but use NSMaxYEdge / NSMinYEdge instead,
then you don't even need the ternary.

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm (left):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm:84: }
Is this appearance-preserving?

[Sidney San Martín, 2016-09-14 17:46:59]

I think this addresses your comments so far, it feels much cleaner this way.

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
File chrome/browser/ui/cocoa/background_gradient_view.h (right):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
chrome/browser/ui/cocoa/background_gradient_view.h:12: enum class
BackgroundGradientViewDivider {
On 2016/09/12 13:45:55, Nico wrote:
> why enum class? can we come up with something shorter than
> `BackgroundGradientViewDivider::Bottom` too?

I'd rather shove one name into the global namespace than two (re. using an enum
class), and naming it after the class seemed consistent with Apple style. What
would you've done?

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
File chrome/browser/ui/cocoa/background_gradient_view.mm (left):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
chrome/browser/ui/cocoa/background_gradient_view.mm:39: showsDivider_ = YES;
On 2016/09/12 13:45:55, Nico wrote:
> This used to default to divider-on, now it defaults to divider-off, no?

Fixed, thanks.

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
File chrome/browser/ui/cocoa/background_gradient_view.mm (right):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/bac...
chrome/browser/ui/cocoa/background_gradient_view.mm:65: }
On 2016/09/12 13:45:55, Nico wrote:
> These 7 lines are shorter as
> 
>   if (divider_ == BackgroundGradientViewDivider::Top)
>     borderRect.origin.y = NSHeight(self.bounds) - lineWidth;
> 
> But I'd probably leave the previous code and tweak it like so:
> 
>   NSDivideRect([self bounds], &borderRect, &contentRect, [self cr_lineWidth],
>     divider_ == BackgroundGradientViewDivider::Top ? NSMaxYEdge : NSMinYEdge);
> 
> And if you don't make up your own enum but use NSMaxYEdge / NSMinYEdge
instead,
> then you don't even need the ternary.

I think I had an incomplete understanding of NSDivideRect. I like this idea much
better.

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm (left):

https://codereview.chromium.org/2336463002/diff/1/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm:84: }
On 2016/09/12 13:45:55, Nico wrote:
> Is this appearance-preserving?

It is now, as far as I can tell.

[Sidney San Martín, 2016-09-14 17:49:46]

Description was changed from

==========
BackgroundGradientView: choice of top or bottom divider

This replaces the duplicate divider drawing in DownloadShelfView.

BUG=589943
==========

to

==========
Make the edge where BackgroundGradientView draws a divider selectable.

…and remove the duplicate divider drawing in DownloadShelfView.

BUG=589943
==========

[Nico, 2016-09-14 18:06:37]

Sorry, one more question :-)

https://codereview.chromium.org/2336463002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm (right):

https://codereview.chromium.org/2336463002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm:45: [self
cr_lineWidth] * 2, NSMaxYEdge);
is the `* 2` here correct? from what I understand, the superclass now draws the
stroke, and we only draw the the highlight below that here now. the highlight
was one cr_lineWidth high before; now it looks like it's two cr_lineWidth high
as far as I can tell.

https://codereview.chromium.org/2336463002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm:58: }
this top highlight code looks pretty similar to the previous top stroke code --
i guess this will grow a IsModeMaterial() call eventually too.

(nothing to do here; just remarking to myself that i can see why the stroke code
was here before this cl, but i also see how having it only once in the
superclass makes sense as well)

[Sidney San Martín, 2016-09-14 18:21:16]

Patchset #4 (id:60001) has been deleted

[Sidney San Martín, 2016-09-14 18:23:50]

Patchset #4 (id:80001) has been deleted

[Sidney San Martín, 2016-09-14 18:34:37]

No problem! Thank you for looking at it closely enough to come up with these
kinds of comments.

https://codereview.chromium.org/2336463002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm (right):

https://codereview.chromium.org/2336463002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm:45: [self
cr_lineWidth] * 2, NSMaxYEdge);
On 2016/09/14 18:06:37, Nico wrote:
> is the `* 2` here correct? from what I understand, the superclass now draws
the
> stroke, and we only draw the the highlight below that here now. the highlight
> was one cr_lineWidth high before; now it looks like it's two cr_lineWidth high
> as far as I can tell.

Ugh, I read that parameter as affecting position, but NSDivideRect isn't
responsible for that (and the position gets shifted on the next line). Fixed.

https://codereview.chromium.org/2336463002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm:58: }
On 2016/09/14 18:06:37, Nico wrote:
> this top highlight code looks pretty similar to the previous top stroke code
--
> i guess this will grow a IsModeMaterial() call eventually too.
> 
> (nothing to do here; just remarking to myself that i can see why the stroke
code
> was here before this cl, but i also see how having it only once in the
> superclass makes sense as well)

I'm starting the MD version now :). Hopefully this whole block can be removed in
not too long.

[Nico, 2016-09-14 18:54:51]

lgtm

[Sidney San Martín, 2016-09-14 18:55:40]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2016-09-14 18:56:11]

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-09-14 19:37:56]

Description was changed from

==========
Make the edge where BackgroundGradientView draws a divider selectable.

…and remove the duplicate divider drawing in DownloadShelfView.

BUG=589943
==========

to

==========
Make the edge where BackgroundGradientView draws a divider selectable.

…and remove the duplicate divider drawing in DownloadShelfView.

BUG=589943
==========

[commit-bot: I haz the power, 2016-09-14 19:37:58]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-09-14 19:39:51]

Description was changed from

==========
Make the edge where BackgroundGradientView draws a divider selectable.

…and remove the duplicate divider drawing in DownloadShelfView.

BUG=589943
==========

to

==========
Make the edge where BackgroundGradientView draws a divider selectable.

…and remove the duplicate divider drawing in DownloadShelfView.

BUG=589943

Committed: https://crrev.com/1e6e9e83fdaca6b8021fa4372c77c54463ef339f
Cr-Commit-Position: refs/heads/master@{#418641}
==========

[commit-bot: I haz the power, 2016-09-14 19:39:52]

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