Make ShadowBorder use ShadowValue.

Make ShadowBorder use ShadowValue.

This CL makes ShadowBorder depend on ShadowValue more rather than doing
independent calculations. It also adds a constructor that takes a
ShadowValue.

BUG=None

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

[calamity, 2015-02-05 06:37:29]

Patchset #1 (id:1) has been deleted

[calamity, 2015-02-05 06:38:00]

calamity@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-02-05 17:33:32]

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:43:
bounds.Inset(gfx::ShadowValue::GetMargin(shadows).Scale(-1));
While this looks right did you make sure the new logic isn't going to cause
problems for existing code?

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:50: return -gfx::ShadowValue::GetMargin(shadows);
Seems silly to do this all the time for something we know is never going to
change. How about caching the insets in the constructor?

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.h
File ui/views/shadow_border.h (right):

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.h...
ui/views/shadow_border.h:22: ShadowBorder(const gfx::ShadowValue& shadow);
I don't want another constructor that effectively duplicates the existing one.
I'm ok with this if you plan on going through and fixing all callers to the new
one. Is that your intention? Also, this should be explicit.

[calamity, 2015-02-06 02:15:20]

Patchset #2 (id:40001) has been deleted

[calamity, 2015-02-06 05:29:53]

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:43:
bounds.Inset(gfx::ShadowValue::GetMargin(shadows).Scale(-1));
On 2015/02/05 17:33:32, sky wrote:
> While this looks right did you make sure the new logic isn't going to cause
> problems for existing code?

The only callers are the message center and app list and an image diff shows the
before and after to be pixel perfectly the same.

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:50: return -gfx::ShadowValue::GetMargin(shadows);
On 2015/02/05 17:33:32, sky wrote:
> Seems silly to do this all the time for something we know is never going to
> change. How about caching the insets in the constructor?

Done.

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.h
File ui/views/shadow_border.h (right):

https://codereview.chromium.org/900163002/diff/20001/ui/views/shadow_border.h...
ui/views/shadow_border.h:22: ShadowBorder(const gfx::ShadowValue& shadow);
On 2015/02/05 17:33:32, sky wrote:
> I don't want another constructor that effectively duplicates the existing one.
> I'm ok with this if you plan on going through and fixing all callers to the
new
> one. Is that your intention? Also, this should be explicit.

Yeah, I'll just add it to this CL. Only a couple of callers.

[sky, 2015-02-06 15:43:28]

LGTM

https://codereview.chromium.org/900163002/diff/60001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/900163002/diff/60001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:22: }
nit: newline between the two '}' and add // namespace

[calamity, 2015-02-08 23:54:41]

New patchsets have been uploaded after l-g-t-m from sky@chromium.org

[calamity, 2015-02-08 23:55:01]

https://codereview.chromium.org/900163002/diff/60001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/900163002/diff/60001/ui/views/shadow_border.c...
ui/views/shadow_border.cc:22: }
On 2015/02/06 15:43:28, sky wrote:
> nit: newline between the two '}' and add // namespace

Done.

[calamity, 2015-02-08 23:55:08]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-02-08 23:55:38]

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

[commit-bot: I haz the power, 2015-02-09 00:38:35]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-02-09 00:39:51]

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