Use nine image painter to draw tray background

Use nine image painter to draw tray background

BUG=432500
Committed: https://crrev.com/dd85e3fed5ffa725dfb16b19bd8bc1835b2dd9e3
Cr-Commit-Position: refs/heads/master@{#308484}

[oshima, 2014-12-13 02:15:23]

Patchset #1 (id:1) has been deleted

[oshima, 2014-12-13 02:17:07]

oshima@chromium.org changed reviewers:
+ stevenjb@chromium.org

[oshima, 2014-12-13 02:17:36]

Can you review ash/ ?

Changes in ui/ will be reviewed in https://codereview.chromium.org/806433002/

[stevenjb, 2014-12-15 17:19:10]

ash lgtm w/nit

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:120: const int
kImages[kNumOrientations][kNumStates][9] = {
nit: 9 should be a const defined with the macro, otheriwse one has to look at
the macro definition to understand what is going on here.

[sadrul, 2014-12-15 19:45:56]

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

[sadrul, 2014-12-15 19:45:56]

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:120: const int
kImages[kNumOrientations][kNumStates][9] = {
On 2014/12/15 17:19:10, stevenjb wrote:
> nit: 9 should be a const defined with the macro, otheriwse one has to look at
> the macro definition to understand what is going on here.

Someone using a NineImagePainter should be aware of what it does, and where this
9 comes from, no?

[sadrul, 2014-12-15 19:47:13]

sadrul@chromium.org changed reviewers:
- sadrul@chromium.org

[oshima, 2014-12-15 19:53:10]

On 2014/12/15 19:45:56, sadrul wrote:
>
https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
> File ash/system/tray/tray_background_view.cc (right):
> 
>
https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
> ash/system/tray/tray_background_view.cc:120: const int
> kImages[kNumOrientations][kNumStates][9] = {
> On 2014/12/15 17:19:10, stevenjb wrote:
> > nit: 9 should be a const defined with the macro, otheriwse one has to look
at
> > the macro definition to understand what is going on here.
> 
> Someone using a NineImagePainter should be aware of what it does, and where
this
> 9 comes from, no?

Most use cases do not have to care about this because compiler can fill the
size.
(https://code.google.com/p/chromium/codesearch#search/&q=IMAGE_GRID%5C(&type=c...
This does not work for this case because I'm using array of arrays.

I don't have strong opinion though. It shouldn't be hard to find out, and also
hard to cause real problem (you'll get compilation error).

[stevenjb, 2014-12-15 20:03:24]

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:120: const int
kImages[kNumOrientations][kNumStates][9] = {
On 2014/12/15 19:45:56, sadrul wrote:
> On 2014/12/15 17:19:10, stevenjb wrote:
> > nit: 9 should be a const defined with the macro, otheriwse one has to look
at
> > the macro definition to understand what is going on here.
> 
> Someone using a NineImagePainter should be aware of what it does, and where
this
> 9 comes from, no?

Just saying that, even with the diff right here in the CL it took me an extra
moment to puzzle out where the 9 came from. Besides, we're supposed to avoid
"magic numbers" like this anyway :P

[sadrul, 2014-12-15 20:55:41]

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:120: const int
kImages[kNumOrientations][kNumStates][9] = {
On 2014/12/15 20:03:23, stevenjb wrote:
> On 2014/12/15 19:45:56, sadrul wrote:
> > On 2014/12/15 17:19:10, stevenjb wrote:
> > > nit: 9 should be a const defined with the macro, otheriwse one has to look
> at
> > > the macro definition to understand what is going on here.
> > 
> > Someone using a NineImagePainter should be aware of what it does, and where
> this
> > 9 comes from, no?
> 
> Just saying that, even with the diff right here in the CL it took me an extra
> moment to puzzle out where the 9 came from. Besides, we're supposed to avoid
> "magic numbers" like this anyway :P

Consider the alternative: we would have a constant kNineImageGridSize = 9
(because kImageGridNum is too generic), and that wouldn't be nice.

If we were to make it easier to read, we should define a const int locally here:

  const int kGridSizeForPainter = 9;
  const int kImages[...][kGridSizeForPainter] = { ... };

[stevenjb, 2014-12-15 21:17:00]

I personally would be happy with kNineImageGridSize, but whatever you think
is clear is fine by me. Already lgtm'd.


On Mon Dec 15 2014 at 12:55:41 PM <sadrul@chromium.org> wrote:

>
> https://codereview.chromium.org/798163003/diff/20001/ash/
> system/tray/tray_background_view.cc
> File ash/system/tray/tray_background_view.cc (right):
>
> https://codereview.chromium.org/798163003/diff/20001/ash/
> system/tray/tray_background_view.cc#newcode120
> ash/system/tray/tray_background_view.cc:120
>
<https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...>:
> const int
> kImages[kNumOrientations][kNumStates][9] = {
> On 2014/12/15 20:03:23, stevenjb wrote:
> > On 2014/12/15 19:45:56, sadrul wrote:
> > > On 2014/12/15 17:19:10, stevenjb wrote:
> > > > nit: 9 should be a const defined with the macro, otheriwse one has
> to look
> > at
> > > > the macro definition to understand what is going on here.
> > >
> > > Someone using a NineImagePainter should be aware of what it does,
> and where
> > this
> > > 9 comes from, no?
>
> > Just saying that, even with the diff right here in the CL it took me
> an extra
> > moment to puzzle out where the 9 came from. Besides, we're supposed to
> avoid
> > "magic numbers" like this anyway :P
>
> Consider the alternative: we would have a constant kNineImageGridSize =
> 9 (because kImageGridNum is too generic), and that wouldn't be nice.
>
> If we were to make it easier to read, we should define a const int
> locally here:
>
>    const int kGridSizeForPainter = 9;
>    const int kImages[...][kGridSizeForPainter] = { ... };
>
> https://codereview.chromium.org/798163003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-12-15 23:50:35]

Patchset #2 (id:40001) has been deleted

[oshima, 2014-12-15 23:51:07]

On 2014/12/15 20:55:41, sadrul wrote:
>
https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
> File ash/system/tray/tray_background_view.cc (right):
> 
>
https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
> ash/system/tray/tray_background_view.cc:120: const int
> kImages[kNumOrientations][kNumStates][9] = {
> On 2014/12/15 20:03:23, stevenjb wrote:
> > On 2014/12/15 19:45:56, sadrul wrote:
> > > On 2014/12/15 17:19:10, stevenjb wrote:
> > > > nit: 9 should be a const defined with the macro, otheriwse one has to
look
> > at
> > > > the macro definition to understand what is going on here.
> > > 
> > > Someone using a NineImagePainter should be aware of what it does, and
where
> > this
> > > 9 comes from, no?
> > 
> > Just saying that, even with the diff right here in the CL it took me an
extra
> > moment to puzzle out where the 9 came from. Besides, we're supposed to avoid
> > "magic numbers" like this anyway :P
> 
> Consider the alternative: we would have a constant kNineImageGridSize = 9
> (because kImageGridNum is too generic), and that wouldn't be nice.
> 
> If we were to make it easier to read, we should define a const int locally
here:
> 
>   const int kGridSizeForPainter = 9;
>   const int kImages[...][kGridSizeForPainter] = { ... };

done

[oshima, 2014-12-15 23:51:09]

On 2014/12/15 20:55:41, sadrul wrote:
>
https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
> File ash/system/tray/tray_background_view.cc (right):
> 
>
https://codereview.chromium.org/798163003/diff/20001/ash/system/tray/tray_bac...
> ash/system/tray/tray_background_view.cc:120: const int
> kImages[kNumOrientations][kNumStates][9] = {
> On 2014/12/15 20:03:23, stevenjb wrote:
> > On 2014/12/15 19:45:56, sadrul wrote:
> > > On 2014/12/15 17:19:10, stevenjb wrote:
> > > > nit: 9 should be a const defined with the macro, otheriwse one has to
look
> > at
> > > > the macro definition to understand what is going on here.
> > > 
> > > Someone using a NineImagePainter should be aware of what it does, and
where
> > this
> > > 9 comes from, no?
> > 
> > Just saying that, even with the diff right here in the CL it took me an
extra
> > moment to puzzle out where the 9 came from. Besides, we're supposed to avoid
> > "magic numbers" like this anyway :P
> 
> Consider the alternative: we would have a constant kNineImageGridSize = 9
> (because kImageGridNum is too generic), and that wouldn't be nice.
> 
> If we were to make it easier to read, we should define a const int locally
here:
> 
>   const int kGridSizeForPainter = 9;
>   const int kImages[...][kGridSizeForPainter] = { ... };

done

[oshima, 2014-12-15 23:51:12]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-12-15 23:52:09]

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

[commit-bot: I haz the power, 2014-12-16 00:58:03]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2014-12-16 00:58:58]

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

[oshima, 2014-12-16 18:33:33]

Patchset #3 (id:80001) has been deleted