Fix resource-related issues in views_unittests

Fix resource-related issues in views_unittests

Make sure ui_test.pak file is available for views_unittests.

Also make views_unittests independent from chrome locale pak files by replacing l10n_util::GetApplicationLocale() with base::i18n::GetConfiguredLocale() for tests. The first function needs chrome locale pak files to exist or it returns empty locale string.

BUG=

TEST=Remove all output files. Build only views_unittests. Run views_unittests.

Committed: https://crrev.com/20307b827528c927cb508f8057838f92b3913899
Cr-Commit-Position: refs/heads/master@{#362370}

[Tomasz Moniuszko, 2015-11-19 15:40:19]

Description was changed from

==========
Make sure resources are available for views_unittests

Add necessary resources to views_unittests deps.

TEST=Build views_unittests target from clean sources. Run views_unittests.
     Tests crash if resources are not available.

BUG=
==========

to

==========
Make sure resources are available for views_unittests

Add necessary resources to views_unittests deps.

BUG=

TEST=Build views_unittests target from clean sources. Run views_unittests.
     Tests crash if resources are not available.
==========

[Tomasz Moniuszko, 2015-11-19 15:40:53]

tmoniuszko@opera.com changed reviewers:
+ sky@chromium.org

[sky, 2015-11-19 20:44:12]

https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn#newcode215
ui/views/BUILD.gn:215: "//chrome:packed_resources",
Why do we need chrome resources? I don't see these in the gyp side. Am I just
missing that?

[Tomasz Moniuszko, 2015-11-20 15:03:44]

On 2015/11/19 20:44:12, sky wrote:
> https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn
> File ui/views/BUILD.gn (right):
> 
> https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn#newcode215
> ui/views/BUILD.gn:215: "//chrome:packed_resources",
> Why do we need chrome resources? I don't see these in the gyp side. Am I just
> missing that?

There are 13 crashes otherwise:
HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture
HorizontalSliderTest.SliderListenerEventsForScrollGesture
HorizontalSliderTest.SliderListenerEventsForTapGesture
HorizontalSliderTest.SliderValueForScrollGesture
HorizontalSliderTest.SliderValueForTapGesture
HorizontalSliderTest.UpdateFromClickHorizontal
HorizontalSliderTest.UpdateFromClickRTLHorizontal
TextfieldTest.HitOutsideTextAreaInRTLTest
TextfieldTest.OverflowInRTLTest
TextfieldTest.TextCursorDisplayInRTLTest
VerticalSliderTest.UpdateFromClickVertical
ViewLayerTest.BoundInRTL
ViewLayerTest.ResizeParentInRTL

HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture crashes in
base::i18n::GetTextDirectionForLocaleInStartUp() because locale_name is "" and
locale_split vector is empty.

Empty locale is set by base::i18n::SetICUDefaultLocale(default_locale_) call in
SliderTest::TearDown(). But earlier, in SliderTest::SetUp(), call to
l10n_util::GetApplicationLocale() returns that empty locale.
l10n_util::GetApplicationLocale() internally calls
l10n_util::IsLocaleAvailable() which uses
ui::ResourceBundle::LocaleDataPakExists() to test if locale pak file is
available. If views_unittests target is being build from clean sources there are
no pak files in locale directory.

I haven't investigated other tests but I guess the root cause is similar there
as all crashes are gone when pak files are available.

And the same 13 tests crashed also in GYP build. I added the same dependency in
Patch Set #2.

This is probably not the prettiest way of solving this issue though. I guess
ui/views code shouldn't depend on chrome code. On the other hand dependency
exists anyway. It wasn't just expressed in GN/GYP files. Do you maybe have a
better idea on how to solve this issues?

[sky, 2015-11-20 16:29:46]

These tests aren't crashing on the waterfall now. How are they working?

  -Scott

On Fri, Nov 20, 2015 at 7:03 AM,  <tmoniuszko@opera.com> wrote:
> On 2015/11/19 20:44:12, sky wrote:
>>
>> https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn
>> File ui/views/BUILD.gn (right):
>
>
>>
>>
https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn#newcode215
>> ui/views/BUILD.gn:215: "//chrome:packed_resources",
>> Why do we need chrome resources? I don't see these in the gyp side. Am I
>> just
>> missing that?
>
>
> There are 13 crashes otherwise:
> HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture
> HorizontalSliderTest.SliderListenerEventsForScrollGesture
> HorizontalSliderTest.SliderListenerEventsForTapGesture
> HorizontalSliderTest.SliderValueForScrollGesture
> HorizontalSliderTest.SliderValueForTapGesture
> HorizontalSliderTest.UpdateFromClickHorizontal
> HorizontalSliderTest.UpdateFromClickRTLHorizontal
> TextfieldTest.HitOutsideTextAreaInRTLTest
> TextfieldTest.OverflowInRTLTest
> TextfieldTest.TextCursorDisplayInRTLTest
> VerticalSliderTest.UpdateFromClickVertical
> ViewLayerTest.BoundInRTL
> ViewLayerTest.ResizeParentInRTL
>
> HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture crashes
> in
> base::i18n::GetTextDirectionForLocaleInStartUp() because locale_name is ""
> and
> locale_split vector is empty.
>
> Empty locale is set by base::i18n::SetICUDefaultLocale(default_locale_) call
> in
> SliderTest::TearDown(). But earlier, in SliderTest::SetUp(), call to
> l10n_util::GetApplicationLocale() returns that empty locale.
> l10n_util::GetApplicationLocale() internally calls
> l10n_util::IsLocaleAvailable() which uses
> ui::ResourceBundle::LocaleDataPakExists() to test if locale pak file is
> available. If views_unittests target is being build from clean sources there
> are
> no pak files in locale directory.
>
> I haven't investigated other tests but I guess the root cause is similar
> there
> as all crashes are gone when pak files are available.
>
> And the same 13 tests crashed also in GYP build. I added the same dependency
> in
> Patch Set #2.
>
> This is probably not the prettiest way of solving this issue though. I guess
> ui/views code shouldn't depend on chrome code. On the other hand dependency
> exists anyway. It wasn't just expressed in GN/GYP files. Do you maybe have a
> better idea on how to solve this issues?
>
> https://codereview.chromium.org/1464503002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tomasz Moniuszko, 2015-11-23 09:47:53]

On 2015/11/20 16:29:46, sky wrote:
> These tests aren't crashing on the waterfall now. How are they working?
> 
>   -Scott
> 

I guess waterfall builds all the targets first and then runs the tests.
ui_test.pak and locale pak files are generated by other targets and they are
available when views_unittests are run. This issue is reproducible on local
machine with following steps:

1. Make sure you start with clean checkout (or delete your out directory: as a
minimal test case deleting ui_tests.pak and locale pak files is enough).
2. Build only views_unittests target.
3. Run views_unittests.

Current result:
ui_test.pak and locale pak files are not generated while building
views_unittests and tests crash in runtime.

[sky, 2015-11-23 16:08:50]

I get the ui_test.pak dep, but not the chrome one. What resources are
being used from //chrome:packed_resources?

On Mon, Nov 23, 2015 at 1:47 AM,  <tmoniuszko@opera.com> wrote:
> On 2015/11/20 16:29:46, sky wrote:
>>
>> These tests aren't crashing on the waterfall now. How are they working?
>
>
>>    -Scott
>
>
>
> I guess waterfall builds all the targets first and then runs the tests.
> ui_test.pak and locale pak files are generated by other targets and they are
> available when views_unittests are run. This issue is reproducible on local
> machine with following steps:
>
> 1. Make sure you start with clean checkout (or delete your out directory: as
> a
> minimal test case deleting ui_tests.pak and locale pak files is enough).
> 2. Build only views_unittests target.
> 3. Run views_unittests.
>
> Current result:
> ui_test.pak and locale pak files are not generated while building
> views_unittests and tests crash in runtime.
>
> https://codereview.chromium.org/1464503002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2015-11-23 16:28:33]

On Mon, Nov 23, 2015 at 2:08 PM, Scott Violet <sky@chromium.org> wrote:

> I get the ui_test.pak dep, but not the chrome one. What resources are
> being used from //chrome:packed_resources?
>
> It shouldn't! I guess I have fixed that dependency long time ago!

Tomasz, could explain better what are you doing? Did you clobber your
output directory? What command lines are you running to reproduce this
issue?

-- 
Thiago Farina

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tomasz Moniuszko, 2015-11-24 11:43:52]

On 2015/11/23 16:28:33, tfarina wrote:
> Tomasz, could explain better what are you doing? Did you clobber your
> output directory? What command lines are you running to reproduce this
> issue?

The commands are (in chromium/src directory, Windows, Git Bash):
git pull
rm -rf out/Release
gclient sync
ninja -C out/Release views_unittests
out/Release/views_unittests.exe

I'm getting 13 tests crashed:
    HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture
    HorizontalSliderTest.SliderListenerEventsForScrollGesture
    HorizontalSliderTest.SliderListenerEventsForTapGesture
    HorizontalSliderTest.SliderValueForScrollGesture
    HorizontalSliderTest.SliderValueForTapGesture
    HorizontalSliderTest.UpdateFromClickHorizontal
    HorizontalSliderTest.UpdateFromClickRTLHorizontal
    TextfieldTest.HitOutsideTextAreaInRTLTest
    TextfieldTest.OverflowInRTLTest
    TextfieldTest.TextCursorDisplayInRTLTest
    VerticalSliderTest.UpdateFromClickVertical
    ViewLayerTest.BoundInRTL
    ViewLayerTest.ResizeParentInRTL

Now, build resources and retry tests:
ninja -C out/Release packed_resources
out/Release/views_unittests.exe

No tests crash now because pak files in out/Release/locale are available.

I agree that dependency on //chrome:packed_resources may be not the right thing.
I will try to avoid this dependency and fix crashes in tests source code. At
least for some tests it should be possible.

[sky, 2015-11-24 16:34:24]

On Fri, Nov 20, 2015 at 7:03 AM,  <tmoniuszko@opera.com> wrote:
> On 2015/11/19 20:44:12, sky wrote:
>>
>> https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn
>> File ui/views/BUILD.gn (right):
>
>
>>
>>
https://codereview.chromium.org/1464503002/diff/1/ui/views/BUILD.gn#newcode215
>> ui/views/BUILD.gn:215: "//chrome:packed_resources",
>> Why do we need chrome resources? I don't see these in the gyp side. Am I
>> just
>> missing that?
>
>
> There are 13 crashes otherwise:
> HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture
> HorizontalSliderTest.SliderListenerEventsForScrollGesture
> HorizontalSliderTest.SliderListenerEventsForTapGesture
> HorizontalSliderTest.SliderValueForScrollGesture
> HorizontalSliderTest.SliderValueForTapGesture
> HorizontalSliderTest.UpdateFromClickHorizontal
> HorizontalSliderTest.UpdateFromClickRTLHorizontal
> TextfieldTest.HitOutsideTextAreaInRTLTest
> TextfieldTest.OverflowInRTLTest
> TextfieldTest.TextCursorDisplayInRTLTest
> VerticalSliderTest.UpdateFromClickVertical
> ViewLayerTest.BoundInRTL
> ViewLayerTest.ResizeParentInRTL
>
> HorizontalSliderTest.SliderListenerEventsForMultiFingerScrollGesture crashes
> in
> base::i18n::GetTextDirectionForLocaleInStartUp() because locale_name is ""
> and
> locale_split vector is empty.
>
> Empty locale is set by base::i18n::SetICUDefaultLocale(default_locale_) call
> in
> SliderTest::TearDown(). But earlier, in SliderTest::SetUp(), call to
> l10n_util::GetApplicationLocale() returns that empty locale.
> l10n_util::GetApplicationLocale() internally calls
> l10n_util::IsLocaleAvailable() which uses
> ui::ResourceBundle::LocaleDataPakExists() to test if locale pak file is
> available. If views_unittests target is being build from clean sources there
> are
> no pak files in locale directory.

Is there a way to detect this case from the test and do nothing?

>
> I haven't investigated other tests but I guess the root cause is similar
> there
> as all crashes are gone when pak files are available.
>
> And the same 13 tests crashed also in GYP build. I added the same dependency
> in
> Patch Set #2.
>
> This is probably not the prettiest way of solving this issue though. I guess
> ui/views code shouldn't depend on chrome code. On the other hand dependency
> exists anyway. It wasn't just expressed in GN/GYP files. Do you maybe have a
> better idea on how to solve this issues?
>
> https://codereview.chromium.org/1464503002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tomasz Moniuszko, 2015-11-27 11:11:33]

Description was changed from

==========
Make sure resources are available for views_unittests

Add necessary resources to views_unittests deps.

BUG=

TEST=Build views_unittests target from clean sources. Run views_unittests.
     Tests crash if resources are not available.
==========

to

==========
Fix resource-related issues in views_unittests

Make sure ui_test.pak file is available for views_unittests.

Also make views_unittests independent from chrome locale pak files by replacing
l10n_util::GetApplicationLocale() with base::i18n::GetConfiguredLocale() for
tests. The first function needs chrome locale pak files to exist or it returns
empty locale string.

BUG=

TEST=Remove all output files. Build only views_unittests. Run views_unittests.
==========

[Tomasz Moniuszko, 2015-11-27 11:32:41]

On 2015/11/24 16:34:24, sky wrote:
> Is there a way to detect this case from the test and do nothing?

I replaced l10n_util::GetApplicationLocale("") function with
base::i18n::GetConfiguredLocale() in Patch Set 3. The first one needs chrome
locale pak files to work properly. The latter one uses ICU default locale. All
these failing tests remember the locale before the test and restore it after the
test. Locale is being restored by setting ICU default locale. Now locale is
being remembered by getting ICU default locale which I think is even better
solution than previous one.

I also modified this issue's title and description to reflect current change
set.

[sky, 2015-11-30 17:12:45]

LGTM

[Tomasz Moniuszko, 2015-12-01 07:56:47]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2015-12-01 07:57:07]

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

[commit-bot: I haz the power, 2015-12-01 10:45:35]

Description was changed from

==========
Fix resource-related issues in views_unittests

Make sure ui_test.pak file is available for views_unittests.

Also make views_unittests independent from chrome locale pak files by replacing
l10n_util::GetApplicationLocale() with base::i18n::GetConfiguredLocale() for
tests. The first function needs chrome locale pak files to exist or it returns
empty locale string.

BUG=

TEST=Remove all output files. Build only views_unittests. Run views_unittests.
==========

to

==========
Fix resource-related issues in views_unittests

Make sure ui_test.pak file is available for views_unittests.

Also make views_unittests independent from chrome locale pak files by replacing
l10n_util::GetApplicationLocale() with base::i18n::GetConfiguredLocale() for
tests. The first function needs chrome locale pak files to exist or it returns
empty locale string.

BUG=

TEST=Remove all output files. Build only views_unittests. Run views_unittests.
==========

[commit-bot: I haz the power, 2015-12-01 10:45:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-01 10:46:34]

Description was changed from

==========
Fix resource-related issues in views_unittests

Make sure ui_test.pak file is available for views_unittests.

Also make views_unittests independent from chrome locale pak files by replacing
l10n_util::GetApplicationLocale() with base::i18n::GetConfiguredLocale() for
tests. The first function needs chrome locale pak files to exist or it returns
empty locale string.

BUG=

TEST=Remove all output files. Build only views_unittests. Run views_unittests.
==========

to

==========
Fix resource-related issues in views_unittests

Make sure ui_test.pak file is available for views_unittests.

Also make views_unittests independent from chrome locale pak files by replacing
l10n_util::GetApplicationLocale() with base::i18n::GetConfiguredLocale() for
tests. The first function needs chrome locale pak files to exist or it returns
empty locale string.

BUG=

TEST=Remove all output files. Build only views_unittests. Run views_unittests.

Committed: https://crrev.com/20307b827528c927cb508f8057838f92b3913899
Cr-Commit-Position: refs/heads/master@{#362370}
==========

[commit-bot: I haz the power, 2015-12-01 10:46:34]

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

[tfarina, 2015-12-01 12:10:17]

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

[tfarina, 2015-12-01 12:10:18]

Sorry, I missed that!

Thanks for figuring out a better way to do this and making this change!

The changes LGTM 2!