Move more webdata unittests into components_unittests target.

Move web_data_service_unittest.cc from unit_tests target to components_unittests target.

Recent changes from Blundell made it possible.

BUG=140037, 181277
TEST=components_unittests
R=joi@chromium.org,thestig@chromium.org

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

[tfarina, 2013-11-22 00:58:26]

Lei -> chrome
Joi -> components

[Lei Zhang, 2013-11-22 01:44:27]

trybots look unhappy

[tfarina, 2013-11-22 08:42:04]

On 2013/11/22 01:44:27, Lei Zhang wrote:
> trybots look unhappy

Fixed. PTAL.

[Jói, 2013-11-22 08:46:13]

LGTM, thanks Thiago!!

[commit-bot: I haz the power, 2013-11-22 15:06:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/82353002/90001

[commit-bot: I haz the power, 2013-11-22 15:24:16]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Lei Zhang, 2013-11-22 19:37:04]

lgtm

https://codereview.chromium.org/82353002/diff/90001/components/dom_distiller/...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://codereview.chromium.org/82353002/diff/90001/components/dom_distiller/...
components/dom_distiller/core/distiller_unittest.cc:19: #include
"ui/base/resource/resource_bundle.h"
nit: remove

[commit-bot: I haz the power, 2013-11-22 20:29:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/82353002/240001

[commit-bot: I haz the power, 2013-11-22 20:46:33]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tfarina, 2013-11-22 21:30:30]

That is awful. Why the heck the owner of other directory needs to sign-off the
adiction of a line to a DEPS file. Sigh!

Tony can you review this to me?

Thanks!

[tony, 2013-11-22 21:43:49]

https://codereview.chromium.org/82353002/diff/240001/components/test/run_all_...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/82353002/diff/240001/components/test/run_all_...
components/test/run_all_unittests.cc:45:
ui::ResourceBundle::InitSharedInstanceWithLocale("en-US", NULL);
This line loads en-US.pak, right?  I don't see where the compontents_unittests
target builds this.  I bet it's built by something in src/chrome.

You probably need to make a test pak file like you did for ui_unittests.

[tfarina, 2013-11-23 23:12:16]

Tony, I made the changes you suggested.

Please, take another look.

Thanks!

[tfarina, 2013-11-23 23:20:29]

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
components/components_tests.gyp:30:
'<(SHARED_INTERMEDIATE_DIR)/ui/app_locale_settings/app_locale_settings_en-US.pak',
I had to add this line, otherwise ResourceBundle does not find
IDS_UI_FONT_FAMILY_CROS.

[tony, 2013-11-24 04:24:07]

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
components/components_tests.gyp:16: 'target_name':
'components_unittests_strings',
Nit: I would probably name the file something like components_test_pak and the
filename something like components_test.pak like you did with
ui_test_pak/ui_test.pak.

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
components/components_tests.gyp:30:
'<(SHARED_INTERMEDIATE_DIR)/ui/app_locale_settings/app_locale_settings_en-US.pak',
On 2013/11/23 23:20:30, tfarina wrote:
> I had to add this line, otherwise ResourceBundle does not find
> IDS_UI_FONT_FAMILY_CROS.

That's fine, but should this target depend on
ui/base/strings/ui_strings.gyp:ui_strings?

[tfarina, 2013-11-24 17:19:40]

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
components/components_tests.gyp:16: 'target_name':
'components_unittests_strings',
On 2013/11/24 04:24:07, tony wrote:
> Nit: I would probably name the file something like components_test_pak and the
> filename something like components_test.pak like you did with
> ui_test_pak/ui_test.pak.

Done.

https://codereview.chromium.org/82353002/diff/400001/components/components_te...
components/components_tests.gyp:30:
'<(SHARED_INTERMEDIATE_DIR)/ui/app_locale_settings/app_locale_settings_en-US.pak',
On 2013/11/24 04:24:07, tony wrote:
> On 2013/11/23 23:20:30, tfarina wrote:
> > I had to add this line, otherwise ResourceBundle does not find
> > IDS_UI_FONT_FAMILY_CROS.
> 
> That's fine, but should this target depend on
> ui/base/strings/ui_strings.gyp:ui_strings?
I added, thanks for catching this. I think it will be necessary, in order to
generate the app_locale_settings_en-US.pak.

[tfarina, 2013-11-24 17:47:44]

Ilya, do you know why mac is failing for my patch?

[tony, 2013-11-24 20:40:04]

On 2013/11/24 17:47:44, tfarina wrote:
> Ilya, do you know why mac is failing for my patch?

It looks like you need to add a link_settings section for mac that includes
$(SDKROOT)/System/Library/Frameworks/AddressBook.framework .  Look at the
browser target, which is how this worked for the unit_tests target.

[tfarina, 2013-11-24 21:33:01]

On 2013/11/24 20:40:04, tony wrote:
> On 2013/11/24 17:47:44, tfarina wrote:
> > Ilya, do you know why mac is failing for my patch?
> 
> It looks like you need to add a link_settings section for mac that includes
> $(SDKROOT)/System/Library/Frameworks/AddressBook.framework .  Look at the
> browser target, which is how this worked for the unit_tests target.

Thanks Tony! Adding that fixed the issue.

[tfarina, 2013-11-25 01:10:22]

Peter, now it is android that is complaining. I don't understand its output
failure. Can you help me?

[Jói, 2013-11-25 12:57:12]

> That is awful. Why the heck the owner of other directory needs to sign-off
> the
> adiction of a line to a DEPS file. Sigh!

It's to prevent higher layers from depending inappropriately on the
internals of lower layers (e.g. content) when there is a public
interface that higher layers are intended to consume (e.g. the Content
API). Without this check we were seeing inappropriate DEPS additions
being made.

Cheers,
Jói


On Fri, Nov 22, 2013 at 9:30 PM,  <tfarina@chromium.org> wrote:
> That is awful. Why the heck the owner of other directory needs to sign-off
> the
> adiction of a line to a DEPS file. Sigh!
>
> Tony can you review this to me?
>
> Thanks!
>
> https://codereview.chromium.org/82353002/

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

[tfarina, 2013-11-26 15:29:47]

-peter

+newt

Newton, could you help with the Android failure?

[Peter Beverloo, 2013-11-27 19:00:19]

https://codereview.chromium.org/82353002/diff/560006/components/test/run_all_...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/82353002/diff/560006/components/test/run_all_...
components/test/run_all_unittests.cc:60:
ui::ResourceBundle::InitSharedInstanceWithPakPath(pak_file);
ui::ResourceBundle requires screen information, which requires JNI to be
registered for Android.  You can look to UITestSuite to see how JNI for ui and
gfx is initialized there.

[tfarina, 2013-11-27 21:53:56]

Peter, I made the changes. PTAL.

[Peter Beverloo, 2013-11-27 21:56:39]

On 2013/11/27 21:53:56, tfarina wrote:
> Peter, I made the changes. PTAL.

lgtm

[tfarina, 2013-12-07 00:08:10]

Joi, I moved only web_data_service_unittest.cc, since it does not use anything
from ResourceBundle (pak stuff).

Pushing this to CQ!

[commit-bot: I haz the power, 2013-12-07 00:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/82353002/870001

[commit-bot: I haz the power, 2013-12-07 03:54:29]

Change committed as 239312

[Jói, 2013-12-09 10:47:40]

Still LGTM, thanks!