Introduce a mock ui_unittests Framework for loading resources.

Introduce a mock ui_unittests Framework for loading resources.

This allows ui_unittests to stop depending on the chrome framework.

On Mac, this creates (e.g.)
- out/ui_unittests Framework.framework/
  +-- Resources -> Versions/A/Resources
  \-- Versions
      \-- A
          \-- Resources
              +-- Info.plist
              +-- am.lproj
              |   \-- locale.pak
              +-- ...
              +-- chrome_100_percent.pak -> ui_test.pak
              +-- ...
              +-- en.lproj
              |   \-- locale.pak
              +-- ...

On other platforms, out/ui_test.pak is loaded directly and
out/ui_unittests_strings/ is set as the locale folder (for tests that
load en-US.pak from there). ui_unittests currently depends on
out/locales/ which is only created when Chrome is built.

Note that ui_unittests does not currently succeed in a clobber build
(crbug.com/347851), so that missing dependency is fixed by this change
as well.

BUG=331669, 35878, 347851
TEST=ui_unittests should build and run after clobbering the build folder

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

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

[tfarina, 2014-02-07 01:06:26]

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
File ui/base/test/run_all_unittests.cc (right):

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
ui/base/test/run_all_unittests.cc:59: path = path.AppendASCII("ui_unittests
Framework.framework");
Can we get this chunk landed first (and separate) from the chunk below?

So we can see exactly what is (or not) necessary for mac?

[tfarina, 2014-02-07 03:57:24]

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
File ui/base/test/run_all_unittests.cc (right):

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
ui/base/test/run_all_unittests.cc:59: path = path.AppendASCII("ui_unittests
Framework.framework");
For reference, content_shell has a clean way to do this (imo):

https://chromium.googlesource.com/chromium/chromium/+/trunk/content/shell/app...

[tapted, 2014-02-28 08:10:55]

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
File ui/base/test/run_all_unittests.cc (right):

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
ui/base/test/run_all_unittests.cc:59: path = path.AppendASCII("ui_unittests
Framework.framework");
On 2014/02/07 03:57:24, tfarina wrote:
> For reference, content_shell has a clean way to do this (imo):
> 
>
https://chromium.googlesource.com/chromium/chromium/+/trunk/content/shell/app...

Since we know it's a test binary running we can simplify here. Otherwise these
lines should have the same effect as OverrideFrameworkBundlePath in that link

https://codereview.chromium.org/152543005/diff/60001/ui/base/test/run_all_uni...
ui/base/test/run_all_unittests.cc:59: path = path.AppendASCII("ui_unittests
Framework.framework");
On 2014/02/07 01:06:27, tfarina wrote:
> Can we get this chunk landed first (and separate) from the chunk below?
> 
> So we can see exactly what is (or not) necessary for mac?

I've added another symlink in the Framework bundle so we can avoid the chunk
below on Mac. Haven't found a nice way to avoid it on other platforms, and there
were some other things missed that come up only when doing clobber builds.

[tapted, 2014-02-28 08:12:06]

Hi Ben,
 PTAL. This gets rid of one of the nastier gyp file-dependency loops in
aura/views, and was suggested at crrev.com/131843004/#msg12 .

I've tested clobber builds on mac, win, ios and linux to ensure the dependencies
are correct. Note currently (without this change) ui_unittests does not actually
have enough dependencies (packed_resources isn't enough) and tests will fail in
a fresh build (http://crbug.com/347851). So this fixes that too.

https://codereview.chromium.org/152543005/diff/330001/ui/base/strings/ui_stri...
File ui/base/strings/ui_strings.gyp (left):

https://codereview.chromium.org/152543005/diff/330001/ui/base/strings/ui_stri...
ui/base/strings/ui_strings.gyp:41: ['os_posix == 1 and OS != "mac"', {
note that this now builds the target on windows, so that it can pull a locale to
do (e.g.) time format tests

[tfarina, 2014-02-28 14:11:16]

Trent is the locales fix necessary for the Mac changes?

I'd like to get it in a separate change if we can avoid mixing it with other
changes. So it is clear which changes we made to make ui_unittests run without
packed_resources.

I can them in this change, but would be good in a separate CL though.

You will need a Mac expert to review your Mac changes. May be Nico?

[tapted, 2014-02-28 21:45:37]

On 2014/02/28 14:11:16, tfarina wrote:
> Trent is the locales fix necessary for the Mac changes?
>
> I'd like to get it in a separate change if we can avoid mixing it with other
> changes. So it is clear which changes we made to make ui_unittests run without
> packed_resources.
>
> I can them in this change, but would be good in a separate CL though.

The tests on other platforms fail without the DIR_LOCALES override. It's only a
curiosity of the builders that they build all of Chrome before running tests, so
the bots don't notice. Since I'm removing the dependency, I need to ensure all
platforms remain intact. The best way to do this is to ensure all the tests
pass, without compiling all of Chrome.

[tfarina, 2014-02-28 22:44:58]

I'm not comfortable in approving this change alone.

+Tony for a more high/expert level review.

Thanks!

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
File ui/base/test/run_all_unittests.cc (right):

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
ui/base/test/run_all_unittests.cc:68:
ui::ResourceBundle::InitSharedInstanceWithLocale("en-US", NULL);
this still depends on chrome_100_percent.pak internally and thus in
packed_resources. Isn't this an issue anymore? how removing packed_resources
from ui_unittests.gyp but still calling this works on those platforms?

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
ui/base/test/run_all_unittests.cc:75:
exe_path.Append(FILE_PATH_LITERAL("ui_test.pak")));
could you change to AppendASCII?

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
ui/base/test/run_all_unittests.cc:81:
exe_path.Append(FILE_PATH_LITERAL("ui_unittests_strings")));
could you change to AppendASCII?

[tony, 2014-03-01 00:42:27]

Seems reasonable to me.  ui/base/l10n LGTM

https://codereview.chromium.org/152543005/diff/360001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/360001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:60: 'target_name': 'ui_test_dll',
Why is this called ui_test_dll?  Can it be something like ui_unittest_resources?
 Alternately, do we need a separate target for this?  Seems like you could
inline this in ui_unittests.

[tapted, 2014-03-03 12:40:40]

+thakis - could you please have a look at the Mac changes here (ui/base/test/*
and ui/*.gyp)

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
File ui/base/test/run_all_unittests.cc (right):

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
ui/base/test/run_all_unittests.cc:68:
ui::ResourceBundle::InitSharedInstanceWithLocale("en-US", NULL);
On 2014/02/28 22:44:58, tfarina wrote:
> this still depends on chrome_100_percent.pak internally and thus in
> packed_resources. Isn't this an issue anymore? how removing packed_resources
> from ui_unittests.gyp but still calling this works on those platforms?

This line now only gets executed on iOS and Android.

iOS already has extra steps in the gyp files, and doesn't depend on
chrome_100_percent.pak in tests. See comments around
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ui_unittests.gy...

I verified this by clobbering the build folders, and running the tests on the
iOS simulator after just building `ui_unittests.app`. That is, The following
doesn't result in chrome_foo_resources.pak being built (or in it being necessary
at runtime).

 xcodebuild -project ui/ui_unittests.xcodeproj -target "All" -configuration
Debug -sdk iphonesimulator build
 xcodebuild/ninja-iossim/Debug/iossim  -d iphone -s 7.0
xcodebuild/Debug-iphonesimulator/ui_unittests.app -- --gtest_print_time


On android, the test harness has additional requirements that will ensure the
runtime dependency is met (ui_unittests just makes
out/Debug/lib/libui_unittests.so on Android). That is, the same idea that made
the old `run_ui_unittests` target work applies for android since the runner has
the required dependencies.

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
ui/base/test/run_all_unittests.cc:75:
exe_path.Append(FILE_PATH_LITERAL("ui_test.pak")));
On 2014/02/28 22:44:58, tfarina wrote:
> could you change to AppendASCII?

Done.

https://codereview.chromium.org/152543005/diff/360001/ui/base/test/run_all_un...
ui/base/test/run_all_unittests.cc:81:
exe_path.Append(FILE_PATH_LITERAL("ui_unittests_strings")));
On 2014/02/28 22:44:58, tfarina wrote:
> could you change to AppendASCII?

Done.

https://codereview.chromium.org/152543005/diff/360001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/360001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:60: 'target_name': 'ui_test_dll',
On 2014/03/01 00:42:28, tony wrote:
> Why is this called ui_test_dll? Can it be something like
ui_unittest_resources?
>  Alternately, do we need a separate target for this?  Seems like you could
> inline this in ui_unittests.

The `ui_test_dll` name was because it mirrors the `chrome_dll` target in
chrome_dll.gypi, but I agree it feels out of place here. 

On mac, the target gets promoted into a shared_library type to create the
Framework bundle, so I don't think that part can be inlined. However, the
condition can be moved which does simplify things a bit.

Done x2.

[tony, 2014-03-03 17:49:36]

gyp(i) files look much better. Thanks!

[Nico, 2014-03-04 21:14:27]

lgtm

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
File ui/ui_unittests_bundle.gypi (right):

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
ui/ui_unittests_bundle.gypi:25: '<!@pymod_do_main(repack_locales -o -p <(OS) -g
<(grit_out_dir) -s <(SHARED_INTERMEDIATE_DIR) -x <(SHARED_INTERMEDIATE_DIR)
<(locales))',
How much does this add to gyp execution time?

[tapted, 2014-03-05 00:18:19]

On 2014/03/04 21:14:27, Nico wrote:
> lgtm
> 
>
https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
> File ui/ui_unittests_bundle.gypi (right):
> 
>
https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
> ui/ui_unittests_bundle.gypi:25: '<!@pymod_do_main(repack_locales -o -p <(OS)
-g
> <(grit_out_dir) -s <(SHARED_INTERMEDIATE_DIR) -x <(SHARED_INTERMEDIATE_DIR)
> <(locales))',
> How much does this add to gyp execution time?

It adds "a bit" - maybe about a second, but it doesn't seem to be on the
critical path with parallel gyp. With 5 samples on each on spinning rust I get:

master
real: 32.60 +/- 0.93
user: 66.35 +/- 0.17
sys: 7.27 +/- 0.02

branch
real: 32.87 +/- 0.21
user: 67.83 +/- 0.88
sys: 7.70 +/- 0.12

units: mean seconds with a 90% confidence interval for the true mean

[tapted, 2014-03-05 00:23:47]

ben: friendly ping. Since this came up in crrev.com/131843004 - does it lgty?

[tfarina, 2014-03-05 18:39:31]

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:351: 'includes': [ 'ui_unittests_bundle.gypi' ],
Trent, instead of going with a gyp include file, please, could you fold it
directly here? I know it is kind of big, but I think it will be better to keep
everything in this file together.

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
File ui/ui_unittests_bundle.gypi (right):

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
ui/ui_unittests_bundle.gypi:28: 'base/test/framework-Info.plist',
What this line does? I'm not sure what mac_bundle_resources mean. Just trying to
understand.

[tapted, 2014-03-05 23:03:26]

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:351: 'includes': [ 'ui_unittests_bundle.gypi' ],
On 2014/03/05 18:39:31, tfarina wrote:
> Trent, instead of going with a gyp include file, please, could you fold it
> directly here? I know it is kind of big, but I think it will be better to keep
> everything in this file together.

I think that would have a negative impact. 10 levels of indent is a bit nuts.
Decomposition is generally good. As is consistency (in this case with the same
pattern at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_dll....
). And since the goal of ui_unittests_bundle.gypi is to mock out the same thing
provided by chrome_dll_bundle.gypi, consistency is important.

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
File ui/ui_unittests_bundle.gypi (right):

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
ui/ui_unittests_bundle.gypi:25: '<!@pymod_do_main(repack_locales -o -p <(OS) -g
<(grit_out_dir) -s <(SHARED_INTERMEDIATE_DIR) -x <(SHARED_INTERMEDIATE_DIR)
<(locales))',
On 2014/03/04 21:14:28, Nico wrote:
> How much does this add to gyp execution time?

(Oops - this comment was detached. Attaching.)

It adds "a bit" - maybe about a second, but it doesn't seem to be on the
critical path with parallel gyp. With 5 samples on each on spinning rust I get:

master
real: 32.60 +/- 0.93
user: 66.35 +/- 0.17
sys: 7.27 +/- 0.02

branch
real: 32.87 +/- 0.21
user: 67.83 +/- 0.88
sys: 7.70 +/- 0.12

units: mean seconds with a 90% confidence interval for the true mean

https://codereview.chromium.org/152543005/diff/400001/ui/ui_unittests_bundle....
ui/ui_unittests_bundle.gypi:28: 'base/test/framework-Info.plist',
On 2014/03/05 18:39:31, tfarina wrote:
> What this line does? I'm not sure what mac_bundle_resources mean. Just trying
to
> understand.

See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_dll_...

My understanding of why it appears multiple times there is that it's required as
a dependency to satisfy the INFOPLIST_FILE key in xcode_settings above, but
shouldn't actually be bundled in with the pak files.

[Ben Goodger (Google), 2014-03-06 03:17:13]

lgtm if macxperts are happy.

[tapted, 2014-03-06 22:23:06]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-03-06 22:31:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/152543005/400001

[commit-bot: I haz the power, 2014-03-07 03:41:45]

Change committed as 255512

[tapted, 2014-03-11 11:11:02]

This got through the CQ but I had to revert it soon after it landed because it
broke the official mac build. The official build tries to strip symbols, but
there's nothing to strip in the mock framework. The fix is pretty
straightforward.

I also missed a dependency that managed to escape all my clobberin'.

Another round of checks suggests things are all good, but I'll wait until
morning. Then if the CQ is kind, it might land again while I'm awake in case of
fallout.

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:76: 'resources/ui_resources.gyp:ui_test_pak',
This just slipped through, but of course it's needed here! The GTK bot (of all
things) picked it up.

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests_bundle....
File ui/ui_unittests_bundle.gypi (right):

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests_bundle....
ui/ui_unittests_bundle.gypi:13: 'mac_strip': 0,
fix for official builds

[Ben Goodger (Google), 2014-03-11 18:08:40]

slgtm

[tfarina, 2014-03-11 18:22:02]

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:350: 'resources/ui_resources.gyp:ui_test_pak',
if you include this at line 76 regardless of the platform. Then we don't need to
include it here, right?

[tapted, 2014-03-11 22:33:26]

The CQ bit was checked by tapted@chromium.org

[tapted, 2014-03-11 22:35:36]

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests.gyp
File ui/ui_unittests.gyp (right):

https://codereview.chromium.org/152543005/diff/460001/ui/ui_unittests.gyp#new...
ui/ui_unittests.gyp:350: 'resources/ui_resources.gyp:ui_test_pak',
On 2014/03/11 18:22:03, tfarina wrote:
> if you include this at line 76 regardless of the platform. Then we don't need
to
> include it here, right?

The mock framework needs the dependency for its postbuild steps, so it can't
rely on the one attached to ui_unittests. I thought about guarding the other
dep, but I think it would just make things more confusing for no real gain.

[commit-bot: I haz the power, 2014-03-11 22:47:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/152543005/460001

[commit-bot: I haz the power, 2014-03-12 04:33:58]

Change committed as 256419