Move resources used by app_list into app_list/resources

Move resources used by app_list into app_list/resources

TBR=thakis
BUG=503224
Committed: https://crrev.com/ce222744c3a849938b3b34bbfadfb0b5c72340e4
Cr-Commit-Position: refs/heads/master@{#344246}

[bshe, 2015-07-30 20:41:23]

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

[commit-bot: I haz the power, 2015-07-30 20:41:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257223005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257223005/80001

[commit-bot: I haz the power, 2015-07-30 21:07:24]

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

[commit-bot: I haz the power, 2015-07-30 21:07:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bshe, 2015-07-30 21:17:01]

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

[commit-bot: I haz the power, 2015-07-30 21:17:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257223005/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257223005/90001

[commit-bot: I haz the power, 2015-07-30 21:52:10]

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

[commit-bot: I haz the power, 2015-07-30 21:52:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bshe, 2015-07-31 17:05:10]

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

[commit-bot: I haz the power, 2015-07-31 17:05:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257223005/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257223005/130001

[commit-bot: I haz the power, 2015-07-31 18:28:26]

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

[commit-bot: I haz the power, 2015-07-31 18:28:27]

Dry run: This issue passed the CQ dry run.

[bshe, 2015-07-31 18:29:42]

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

[bshe, 2015-07-31 18:29:43]

Hey Sadrul.

Do you mind to take a first look at this? It passed the cq dry run. If this is
fine with you, I will add owners.

THanks

[tfarina, 2015-07-31 22:47:02]

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

[tfarina, 2015-07-31 22:47:02]

Could you measure the time before and after your patch? Is it the same or
slower?

[bshe, 2015-08-04 20:59:42]

On 2015/07/31 22:47:02, tfarina wrote:
> Could you measure the time before and after your patch? Is it the same or
> slower?

Sorry I am new to this. Do you mind to share more information about what and
how should I measure the time? thanks

[Matt Giuca, 2015-08-05 00:06:29]

On 2015/08/04 20:59:42, bshe wrote:
> Sorry I am new to this. Do you mind to share more information about what and
> how should I measure the time? thanks

Normally we don't do perf testing before landing a CL (we have bots and sheriffs
to pick up perf regressions after CLs land). tfarina: Do you have a specific
reason to think this will make things slower?

(As far as I can tell, this CL doesn't change any code at all, it just moves
resources around, so I'm not sure what you're suspicious about.)

[tfarina, 2015-08-05 00:38:48]

On Tue, Aug 4, 2015 at 9:06 PM, <mgiuca@chromium.org> wrote:

> On 2015/08/04 20:59:42, bshe wrote:
>
>> Sorry I am new to this. Do you mind to share more information about what
>> and
>> how should I measure the time? thanks
>>
>
> Normally we don't do perf testing before landing a CL (we have bots and
> sheriffs
> to pick up perf regressions after CLs land). tfarina: Do you have a
> specific
> reason to think this will make things slower?
>
This is creating a new grd file, and making us run grit again (which used
to be slow, hence we used to have fewer grd files). Could you measure that
and see if it is not making a difference? See my comment in the bug.

Joi, Nico or Brett can give you better details though!


> https://codereview.chromium.org/1257223005/
>



-- 
Thiago Farina

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

[sadrul, 2015-08-11 19:28:15]

Looks good in general. Please get a review from an app-list owner too

https://codereview.chromium.org/1257223005/diff/130001/ui/app_list/app_list.grd
File ui/app_list/app_list.grd (right):

https://codereview.chromium.org/1257223005/diff/130001/ui/app_list/app_list.g...
ui/app_list/app_list.grd:1: <?xml version="1.0" encoding="UTF-8"?>
Where do we need this file?

[bshe, 2015-08-12 14:13:34]

bshe@chromium.org changed reviewers:
+ thakis@chromium.org, xiyuan@chromium.org

[bshe, 2015-08-12 14:13:35]

Thanks Sadrul!

Adding app_list owner and chrome owner:
+xiyuan for ui/app_list/
+thakis for chrome/BUILD.gn

https://codereview.chromium.org/1257223005/diff/130001/ui/app_list/app_list.grd
File ui/app_list/app_list.grd (right):

https://codereview.chromium.org/1257223005/diff/130001/ui/app_list/app_list.g...
ui/app_list/app_list.grd:1: <?xml version="1.0" encoding="UTF-8"?>
On 2015/08/11 19:28:15, sadrul wrote:
> Where do we need this file?

huh. Sorry. added by mistake. Removed.

[xiyuan, 2015-08-12 16:14:48]

xiyuan@chromium.org changed reviewers:
+ mgiuca@chromium.org

[xiyuan, 2015-08-12 16:14:48]

+mgiuca@

LGTM but I am no longer actively working on app list so please wait for Matt.

[bshe, 2015-08-13 18:21:24]

On 2015/08/12 16:14:48, xiyuan wrote:
> +mgiuca@
> 
> LGTM but I am no longer actively working on app list so please wait for Matt.

friendly ping?

[Matt Giuca, 2015-08-14 10:46:38]

On 2015/08/13 18:21:24, bshe wrote:
> On 2015/08/12 16:14:48, xiyuan wrote:
> > +mgiuca@
> > 
> > LGTM but I am no longer actively working on app list so please wait for
Matt.
> 
> friendly ping?

Sorry, I've been at an offsite.

Lgtm, but I'd like an explanation of the two files where I've written a comment.

[Matt Giuca, 2015-08-14 10:47:32]

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/BUILD.gn
File ui/app_list/BUILD.gn (right):

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/BUILD.gn#n...
ui/app_list/BUILD.gn:307: "//ui/app_list/resources",
See comment in gyp file.

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/app_list.g...
ui/app_list/app_list.gyp:260:
'resources/app_list_resources.gyp:app_list_resources',
Why is this required now when it wasn't before?

[bshe, 2015-08-17 18:20:38]

thanks for review.

thakis@ do you mind to take a look at the simple change in
chrome/BUILD.gn file? somehow this needs OWNER's approval but it counterpart gyp
file doesn't.

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/BUILD.gn
File ui/app_list/BUILD.gn (right):

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/BUILD.gn#n...
ui/app_list/BUILD.gn:307: "//ui/app_list/resources",
On 2015/08/14 10:47:32, Matt Giuca wrote:
> See comment in gyp file.

Done.

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/app_list.g...
ui/app_list/app_list.gyp:260:
'resources/app_list_resources.gyp:app_list_resources',
On 2015/08/14 10:47:32, Matt Giuca wrote:
> Why is this required now when it wasn't before?

You are right. It is not needed. Removed.

[bshe, 2015-08-19 15:04:00]

On 2015/08/17 18:20:38, bshe wrote:
> thanks for review.
> 
> thakis@ do you mind to take a look at the simple change in
> chrome/BUILD.gn file? somehow this needs OWNER's approval but it counterpart
gyp
> file doesn't.
> 
> https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/BUILD.gn
> File ui/app_list/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/BUILD.gn#n...
> ui/app_list/BUILD.gn:307: "//ui/app_list/resources",
> On 2015/08/14 10:47:32, Matt Giuca wrote:
> > See comment in gyp file.
> 
> Done.
> 
>
https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/app_list.gyp
> File ui/app_list/app_list.gyp (right):
> 
>
https://codereview.chromium.org/1257223005/diff/150001/ui/app_list/app_list.g...
> ui/app_list/app_list.gyp:260:
> 'resources/app_list_resources.gyp:app_list_resources',
> On 2015/08/14 10:47:32, Matt Giuca wrote:
> > Why is this required now when it wasn't before?
> 
> You are right. It is not needed. Removed.

TBR thakis since it is really trivial .gn change and the corresponding gyp
change is approved.

[bshe, 2015-08-19 15:04:16]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-08-19 15:04:16]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org,
xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1257223005/#ps170001
(title: "review")

[commit-bot: I haz the power, 2015-08-19 15:06:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257223005/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257223005/170001

[commit-bot: I haz the power, 2015-08-19 16:20:43]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-08-19 16:21:27]

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

[Nico, 2015-08-19 17:10:29]

lgtm, sorry about not seeing this