Add initial home card impl.

Add initial home card impl.

BUG=362288
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272681

[oshima, 2014-05-22 16:16:07]

ping?

[chromium-reviews, 2014-05-22 16:51:15]

Sorry. Got pulled into other things. Looking at this this morning.

On Thu May 22 2014 at 9:16:10 AM, <oshima@chromium.org> wrote:

> ping?
>
> https://codereview.chromium.org/287253003/
>

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

[Albert Bodenhamer, 2014-05-22 19:22:01]

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:20: class HomeCardLayoutManager : public
aura::LayoutManager {
Will we want the home card's layout manager to be an independent class at some
point? If it's likely to stay a locally defined class like this, leave it here,
otherwise let's go ahead and move it.

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:51: card_bounds.Inset(50, screen_bounds.height() -
50, 50, 0);
I know this is expected to change but can we pull magic numbers into constants?

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder.cc
File athena/main/placeholder.cc (left):

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder...
athena/main/placeholder.cc:26: label->SetText(base::ASCIIToUTF16("AppWindow"));
We don't have asset management/localization yet. Add a TODO here to make the
hardcoded string go away and associate with bug 375533. Here and elsewhere.

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder.cc
File athena/main/placeholder.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder...
athena/main/placeholder.cc:27:
label->set_background(views::Background::CreateSolidBackground(0xFFDDDDDD));
Create a constant for this color.

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:50: // TODO(oshima): Delete root window after
athena is cleaned up.
File a bug to track the TODO so it doesn't get lost.

[Mr4D (OOO till 08-26), 2014-05-22 20:53:28]

A few nits

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
File athena/home/home_card_delegate_view.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
athena/home/home_card_delegate_view.cc:14: const SkColor kHomeTopColor =
0xDDFFFFFF;
Could you please move these constants out into the namespace and comment what
they do? Make it easier to change and modify them later.

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:18: HomeCard* instance = NULL;
maybe gInstance? Or better make it a static member of HomeCardImpl?

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:69:
ScreenManager::Get()->CreateDefaultContainer("MainConttainer")) {
tt -> t

[oshima, 2014-05-22 21:23:05]

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:18: HomeCard* instance = NULL;
On 2014/05/22 20:53:28, Mr4D wrote:
> maybe gInstance? Or better make it a static member of HomeCardImpl?

g_ is for global variable, while this is a file scoped variable.

I usually avoid using static private member because it only exposes the
information that a user of the class don't have to know.

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:20: class HomeCardLayoutManager : public
aura::LayoutManager {
On 2014/05/22 19:22:02, Albert Bodenhamer wrote:
> Will we want the home card's layout manager to be an independent class at some
> point? If it's likely to stay a locally defined class like this, leave it
here,
> otherwise let's go ahead and move it.

This class most likely stay small.

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:51: card_bounds.Inset(50, screen_bounds.height() -
50, 50, 0);
On 2014/05/22 19:22:02, Albert Bodenhamer wrote:
> I know this is expected to change but can we pull magic numbers into
constants?

Done.

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder.cc
File athena/main/placeholder.cc (left):

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder...
athena/main/placeholder.cc:26: label->SetText(base::ASCIIToUTF16("AppWindow"));
On 2014/05/22 19:22:02, Albert Bodenhamer wrote:
> We don't have asset management/localization yet. Add a TODO here to make the
> hardcoded string go away and associate with bug 375533. Here and elsewhere.

This code will go a away, so I don't think TODO is useful here.

When we need a string for real UI, the author should simply create grd file and
that'll address this issue.

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder.cc
File athena/main/placeholder.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/main/placeholder...
athena/main/placeholder.cc:27:
label->set_background(views::Background::CreateSolidBackground(0xFFDDDDDD));
On 2014/05/22 19:22:02, Albert Bodenhamer wrote:
> Create a constant for this color.

Done.

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:50: // TODO(oshima): Delete root window after
athena is cleaned up.
On 2014/05/22 19:22:02, Albert Bodenhamer wrote:
> File a bug to track the TODO so it doesn't get lost.

The comment wasn't correct. If fixed the root cause and removed the comment.

https://codereview.chromium.org/287253003/diff/200001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:69:
ScreenManager::Get()->CreateDefaultContainer("MainConttainer")) {
On 2014/05/22 20:53:28, Mr4D wrote:
> tt -> t

Done.

[Mr4D (OOO till 08-26), 2014-05-22 22:19:14]

See my comments. lgtm

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
File athena/home/home_card_delegate_view.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
athena/home/home_card_delegate_view.cc:14: const SkColor kHomeTopColor =
0xDDFFFFFF;
Again - extracting them in one point feels like a good thing. In fact - since we
are starting now a new shell we might even accumulate all these constants in one
place / file. That way it would have all colors / width / sizes / UI constants
in one "metric" file. Something which I would have liked to see in ash as well.
But I leave this up to you.

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:18: HomeCard* instance = NULL;
"g_" Right. "static member": I was asked by other reviewers to do so. I only
rely here what I was asked for.

[chromium-reviews, 2014-05-22 22:24:59]

On Thu May 22 2014 at 3:19:15 PM, <skuhne@chromium.org> wrote:

> See my comments. lgtm
>
>
> https://codereview.chromium.org/287253003/diff/200001/
> athena/home/home_card_delegate_view.cc
> File athena/home/home_card_delegate_view.cc (right):
>
> https://codereview.chromium.org/287253003/diff/200001/
> athena/home/home_card_delegate_view.cc#newcode14
>
athena/home/home_card_delegate_view.cc:14<https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_delegate_view.cc#newcode14athena/home/home_card_delegate_view.cc:14>:
> const SkColor kHomeTopColor =
> 0xDDFFFFFF;
> Again - extracting them in one point feels like a good thing. In fact -
> since we are starting now a new shell we might even accumulate all these
> constants in one place / file. That way it would have all colors / width
> / sizes / UI constants in one "metric" file. Something which I would
> have liked to see in ash as well. But I leave this up to you.
>

+1

Perhaps we should go a step further and extract this sort of thing into a
config file in resources?


>
> https://codereview.chromium.org/287253003/diff/200001/
> athena/home/home_card_impl.cc
> File athena/home/home_card_impl.cc (right):
>
> https://codereview.chromium.org/287253003/diff/200001/
> athena/home/home_card_impl.cc#newcode18
>
athena/home/home_card_impl.cc:18<https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_impl.cc#newcode18athena/home/home_card_impl.cc:18>:
> HomeCard* instance = NULL;
> "g_" Right. "static member": I was asked by other reviewers to do so. I
> only rely here what I was asked for.
>
> https://codereview.chromium.org/287253003/
>

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

[oshima, 2014-05-23 14:54:37]

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
File athena/home/home_card_delegate_view.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
athena/home/home_card_delegate_view.cc:14: const SkColor kHomeTopColor =
0xDDFFFFFF;
On 2014/05/22 22:19:15, Mr4D wrote:
> Again - extracting them in one point feels like a good thing. In fact - since
we
> are starting now a new shell we might even accumulate all these constants in
one
> place / file. That way it would have all colors / width / sizes / UI constants
> in one "metric" file. Something which I would have liked to see in ash as
well.
> But I leave this up to you.

Well, this code is pure place holder, and the color and apparance are picked
randomly.
I do agree that we may want to put this is resource file (like android), and we
should discuss that. Similar suggestion came up in the past but, UX designers
preferred
using image file to control the color because they can just create an asset to
change the color.

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_i...
athena/home/home_card_impl.cc:18: HomeCard* instance = NULL;
On 2014/05/22 22:19:15, Mr4D wrote:
> "g_" Right. "static member": I was asked by other reviewers to do so. I only
> rely here what I was asked for.

You can redirect to me, and I can explain why private static member should be
avoided. (like other c++ features that banned)

[Mr4D (OOO till 08-26), 2014-05-23 15:29:25]

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
File athena/home/home_card_delegate_view.cc (right):

https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
athena/home/home_card_delegate_view.cc:14: const SkColor kHomeTopColor =
0xDDFFFFFF;
We will have colors and we will get more. Even if this is only a placeholder -
it is a precedent and others will follow. So it makes sense to start from the
beginning. But again - your choice.

[oshima, 2014-05-23 16:08:41]

On 2014/05/23 15:29:25, Mr4D wrote:
>
https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
> File athena/home/home_card_delegate_view.cc (right):
> 
>
https://codereview.chromium.org/287253003/diff/200001/athena/home/home_card_d...
> athena/home/home_card_delegate_view.cc:14: const SkColor kHomeTopColor =
> 0xDDFFFFFF;
> We will have colors and we will get more. Even if this is only a placeholder -
> it is a precedent and others will follow. So it makes sense to start from the
> beginning. But again - your choice.

I'm not fully convinced that putting them in one place (in C++) is good thing. I
do agree that it's good idea to put *default configurations*,
such as default font orsize, in one place because such data will be repeatedly
referenced. However, the configuration for each component are most likely
specific to them, and in that case, they should be put in the same place because
it's easier to find and read.

Having a resource file *may* be good thing (although i think it's still
debatable from chrome perspective) but that's outside of the scope of this CL.

[chromium-reviews, 2014-05-23 16:30:11]

Let's tackle putting them in a single place as a separate step. We should
definitely make them constants at least.

On Fri May 23 2014 at 9:08:43 AM, <oshima@chromium.org> wrote:

> On 2014/05/23 15:29:25, Mr4D wrote:
>
> https://codereview.chromium.org/287253003/diff/200001/
> athena/home/home_card_delegate_view.cc
> > File athena/home/home_card_delegate_view.cc (right):
>
>
> https://codereview.chromium.org/287253003/diff/200001/
> athena/home/home_card_delegate_view.cc#newcode14
> > athena/home/home_card_delegate_view.cc:14: const SkColor kHomeTopColor =
> > 0xDDFFFFFF;
> > We will have colors and we will get more. Even if this is only a
> > placeholder -
> > it is a precedent and others will follow. So it makes sense to start from
> > the
> > beginning. But again - your choice.
>
> I'm not fully convinced that putting them in one place (in C++) is good
> thing. I
> do agree that it's good idea to put *default configurations*,
> such as default font orsize, in one place because such data will be
> repeatedly
> referenced. However, the configuration for each component are most likely
> specific to them, and in that case, they should be put in the same place
> because
> it's easier to find and read.
>
> Having a resource file *may* be good thing (although i think it's still
> debatable from chrome perspective) but that's outside of the scope of this
> CL.
>
>
> https://codereview.chromium.org/287253003/
>

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

[oshima, 2014-05-23 20:39:18]

albert, ping?

[Albert Bodenhamer, 2014-05-23 20:48:27]

LGTM

Still a lot of magic numbers here, but I'm ok as long as we keep in mind we need
to clean them up.

[oshima, 2014-05-23 22:11:07]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-05-23 22:11:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/287253003/230001

[oshima, 2014-05-23 23:28:49]

ben, can you approve the DEPS change?

[Ben Goodger (Google), 2014-05-23 23:33:09]

lgtm

[commit-bot: I haz the power, 2014-05-24 01:58:28]

Change committed as 272681