ui: Repack resources.

ui: Make views_unittests not depend on chrome's packed_resources target.

Depending on chrome's target is a layer violation, so we should break that dependency.

BUG=299841, 176960, 144345
TEST=views_unittests
R=tony@chromium.org

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

[tfarina, 2013-10-31 01:23:08]

Would you build it like this?

I haven't right a proper CL description yet. :/

Please, guide me towards the right direction.

https://codereview.chromium.org/45753004/diff/1/ui/aura/aura.gyp
File ui/aura/aura.gyp (left):

https://codereview.chromium.org/45753004/diff/1/ui/aura/aura.gyp#oldcode268
ui/aura/aura.gyp:268: '../../chrome/chrome_resources.gyp:packed_resources',
aura_unittests didn't fail for me locally. I expect things will be different on
the bots, specially on Windows and with XP.

[tony, 2013-10-31 19:48:34]

This seems like the right idea.  I would try fixing the dependencies for
ui/app_list/app_list.gyp and ui/views/views.gyp next.

The ui_unittest failures look like they might be related.

https://codereview.chromium.org/45753004/diff/1/content/test/content_test_sui...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/45753004/diff/1/content/test/content_test_sui...
content/test/content_test_suite.cc:45: ui_test_initializer_.reset(new
ui::test::UITestInitializer);
I'm a bit surprised that none of the content tests need resources in
content_resources.grd.  I would have expected needing a separate pak file for
content tests.

https://codereview.chromium.org/45753004/diff/1/ui/aura/aura.gyp
File ui/aura/aura.gyp (left):

https://codereview.chromium.org/45753004/diff/1/ui/aura/aura.gyp#oldcode268
ui/aura/aura.gyp:268: '../../chrome/chrome_resources.gyp:packed_resources',
On 2013/10/31 01:23:08, tfarina wrote:
> aura_unittests didn't fail for me locally. I expect things will be different
on
> the bots, specially on Windows and with XP.

You tested on a clean build right?  It looks like you shouldn't need it.

https://codereview.chromium.org/45753004/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/45753004/diff/1/ui/ui.gyp#newcode608
ui/ui.gyp:608: '<(PRODUCT_DIR)/ui.pak',
I would put 'test' in the filename to make it clear that it's not needed for
chrome or content_shell.  Maybe ui_test.pak or something.

[tfarina, 2013-10-31 23:30:53]

Hey Tony, I'll play on the safe side.

I introduced ui_test_pak target in ui_resources.gypi in patch set 2. It is a
copy of content_shell_pak, stripped the unnecessary things.

If I were to change ui/views/run_all_unittests.cc,
ui::ResourceBundle::InitSharedInstanceWithLocale(), that call will have to
change to InitSharedInstanceWithPakPath()?

PTAL.

[tony, 2013-10-31 23:33:05]

On 2013/10/31 23:30:53, tfarina wrote:
> Hey Tony, I'll play on the safe side.
> 
> I introduced ui_test_pak target in ui_resources.gypi in patch set 2. It is a
> copy of content_shell_pak, stripped the unnecessary things.

I think this would be more useful if something used it.  Maybe just convert a
single test target to use this?

> If I were to change ui/views/run_all_unittests.cc,
> ui::ResourceBundle::InitSharedInstanceWithLocale(), that call will have to
> change to InitSharedInstanceWithPakPath()?

Yes, I think so.

[tfarina, 2013-10-31 23:43:13]

On 2013/10/31 23:33:05, tony wrote:
> On 2013/10/31 23:30:53, tfarina wrote:
> > Hey Tony, I'll play on the safe side.
> > 
> > I introduced ui_test_pak target in ui_resources.gypi in patch set 2. It is a
> > copy of content_shell_pak, stripped the unnecessary things.
> 
> I think this would be more useful if something used it.  Maybe just convert a
> single test target to use this?
> 
Yup, I intended to do this. See patch set 3.

> > If I were to change ui/views/run_all_unittests.cc,
> > ui::ResourceBundle::InitSharedInstanceWithLocale(), that call will have to
> > change to InitSharedInstanceWithPakPath()?
> 
> Yes, I think so.
Note that I'm not doing any of the contortions that
content/shell/app/shell_main_delegate.cc does for mac and android.

[tfarina, 2013-11-01 00:22:42]

See? It is not that simple, I think InitSharedInstanceWithLocale() +
packed_resources does more "necessary" things. Or it might be something else. :(

It's very obscure/tricky :/

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

[tfarina, 2013-11-01 00:23:58]

oh, wait, I really don't understand how the local change to views_unittests is
affecting ui/gfx/render_text_unittest.cc tests! Do you?

[tfarina, 2013-11-01 00:54:57]

InitSharedInstanceWithLocale() does a lot more things than
InitSharedInstanceWithPakPath().

And LoadCommonResources() is tied to chrome_100_percent.pak. This is model is
just broken! It makes the modules outside of chrome/ actually depend on it.

[tfarina, 2013-11-01 13:20:25]

may be the failures I saw from gfx unittests were not related, as I'm seeing in
another patch:

http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/1835...

sending another try to linux_chromeos to see if it helps.

[tfarina, 2013-11-01 15:26:07]

No, the same thing.

Tony, do have any idea/suggestions now?

Thanks!

[tony, 2013-11-01 16:54:58]

Loading chrome*.pak in ui is
https://code.google.com/p/chromium/issues/detail?id=176960 .

We should maybe fix that first?  You might not be able to use
InitSharedInstanceWithPakPath() in views_unittests, but maybe
InitSharedInstanceWithPakFile would work (with true passed).

If you really want to shave this yak, I would try to clean up that there are 4
ResourceBundle::InitSharedInstance* functions.  For example, you could probably
merge InitSharedInstanceWithLocale and InitSharedInstanceLocaleOnly by adding a
bool.  You might be able to replace all calls to InitSharedInstanceWithPakPath
with calls to InitSharedInstanceWithPakFile.

Finally, we probably want a single method that takes a vector of PlatformFiles,
a pref_locale, and a delegate.  I think that would cover all the possible uses
and move chrome*.pak out of ui.

[tony, 2013-11-01 16:55:43]

This might be too big of a task.  I would try using
InitSharedInstanceWithPakFile() instead of InitSharedInstanceWithPakPath for the
test and see if that works.

[tfarina, 2013-11-01 22:12:12]

Tony,

For InitSharedInstanceWithPakFile(), how I will know the pak descriptor for the
ui_test_pak? I create one? I assign any number I want?

content_shell does this:

int pak_fd =                                                                    
 base::GlobalDescriptors::GetInstance()->MaybeGet(kShellPakDescriptor);

I think kShellPakDescriptor then it is 6.

[tony, 2013-11-01 22:43:59]

InitSharedInstanceWithPakFile takes a base::PlatformFile (which happens to be a
file descriptor on posix systems).  You can create one using
base::CreatePlatformFile in src/base/platform_file.h.

[tfarina, 2013-11-01 23:26:12]

On 2013/11/01 22:43:59, tony wrote:
> InitSharedInstanceWithPakFile takes a base::PlatformFile (which happens to be
a
> file descriptor on posix systems).  You can create one using
> base::CreatePlatformFile in src/base/platform_file.h.

Better now.

[30045:30045:1101/211946:780717028716:WARNING:resource_bundle.cc(361)] Unable to
load image with id 5787
[30045:30045:1101/211946:780717028925:FATAL:resource_bundle.cc(362)] Check
failed: false. 
 [0x7f074887d73e] base::debug::StackTrace::StackTrace()
 [0x7f07488dd142] logging::LogMessage::~LogMessage()
 [0x7f0747be38e3] ui::ResourceBundle::GetImageNamed()
 [0x7f0747be352b] ui::ResourceBundle::GetImageSkiaNamed()
 [0x7f074ac0ed9b] views::(anonymous namespace)::ImagePainter::ImagePainter()
 [0x7f074ac0e8be] views::Painter::CreateImageGridPainter()
 [0x7f074ab3ea73] views::LabelButtonBorder::LabelButtonBorder()

But not finding IDR_BUTTON_NORMAL? That is strange.

[tony, 2013-11-02 00:04:14]

On 2013/11/01 23:26:12, tfarina wrote:

> But not finding IDR_BUTTON_NORMAL? That is strange.

Hmm, weird.  I don't know why that didn't work.  I think you're going to have to
debug this.

[tfarina, 2013-11-02 00:43:29]

On 2013/11/02 00:04:14, tony wrote:
> On 2013/11/01 23:26:12, tfarina wrote:
> 
> > But not finding IDR_BUTTON_NORMAL? That is strange.
> 
> Hmm, weird.  I don't know why that didn't work.  I think you're going to have
to
> debug this.

Yep, I ran views_unittests through gdb to understand better what was happening.

I went back with WithPakPath() + a "hack" in ResouceBundle. I think the locale
stuff I added there will fix the issue with FontPropertySymbol test.

Please, see patch set 6.

[tfarina, 2013-11-02 00:51:42]

On 2013/11/02 00:43:29, tfarina wrote:
> I went back with WithPakPath() + a "hack" in ResouceBundle. I think the locale
> stuff I added there will fix the issue with FontPropertySymbol test.
> 
The failure in FontPropertySymbol is not related, it can't be, because it also
happens in 
https://codereview.chromium.org/48623008/, see linux_aura symbol.

Sigh! Let's see what the chromeos bot says.

> Please, see patch set 6.

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

It seems to be green! Yay!

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

the result is not attached, I think, because I used 'g try -c -b linux_chromeos
-r HEAD'.

I resent, now using:

g cl try -c -b linux_chromeos -r HEAD

[tfarina, 2013-11-02 20:02:55]

Tony, do you think this is good to go?

Do you have any other concerns/suggestions?

[tony, 2013-11-04 17:52:39]

https://codereview.chromium.org/45753004/diff/470001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/45753004/diff/470001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:176: scoped_ptr<DataPack> data_pack(new
DataPack(SCALE_FACTOR_100P));
This code that tries to load the locale resources with SCALE_FACTOR_100P should
be moved to the end of LoadTestResources.

[tfarina, 2013-11-05 01:04:44]

https://codereview.chromium.org/45753004/diff/470001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/45753004/diff/470001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:176: scoped_ptr<DataPack> data_pack(new
DataPack(SCALE_FACTOR_100P));
On 2013/11/04 17:52:40, tony wrote:
> This code that tries to load the locale resources with SCALE_FACTOR_100P
should
> be moved to the end of LoadTestResources.
Please, see if I did right in patch set 8.

[tony, 2013-11-05 07:28:28]

https://codereview.chromium.org/45753004/diff/540001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/45753004/diff/540001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:295:
locale_resources_data_.reset(data_pack.release());
What are you trying to do here?  Why are you loading the first path into
locale_resources_data_?  In the previous change, it looked like the difference
was initializing the locale path data pak with SCALE_FACTOR_100P.

https://codereview.chromium.org/45753004/diff/540001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:297: data_pack.reset(new
DataPack(ui::SCALE_FACTOR_NONE));
In general, I think the problem is that we weren't expecting to mix strings and
image resources in the same pak file.  The code assumes that your pak file has 1
or the other.

It looks like things work OK if you make a DataPack(SCALE_FACTOR_100P) and load
it into locale_resources_data_, but it's a bit weird.  I think it's fine for
this testing method, but I would maybe add a comment.

[tfarina, 2013-11-07 00:05:46]

https://codereview.chromium.org/45753004/diff/650001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/45753004/diff/650001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:296: data_pack.reset(new
DataPack(ui::SCALE_FACTOR_100P));
does this sounds more reasonable? is it ok if it passes the trybots?

[tony, 2013-11-07 18:13:37]

I noticed that the constructor for DataPack is not explicit.  If you could fix
that in a separate change, that would be great.

https://codereview.chromium.org/45753004/diff/650001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/45753004/diff/650001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:296: data_pack.reset(new
DataPack(ui::SCALE_FACTOR_100P));
On 2013/11/07 00:05:46, tfarina wrote:
> does this sounds more reasonable? is it ok if it passes the trybots?

I think this is OK.  Why does this work?  It doesn't look like scale factor
should matter for locale_resources_data_.

[tfarina, 2013-11-09 00:24:34]

Mind take a (another :)) look?

Thanks!

https://codereview.chromium.org/45753004/diff/650001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/45753004/diff/650001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:296: data_pack.reset(new
DataPack(ui::SCALE_FACTOR_100P));
On 2013/11/07 18:13:37, tony wrote:
> On 2013/11/07 00:05:46, tfarina wrote:
> > does this sounds more reasonable? is it ok if it passes the trybots?
> 
> I think this is OK.  Why does this work?  It doesn't look like scale factor
> should matter for locale_resources_data_.
Yeah, it does not seem to matter. The failing unit tests are getting disabled at
crbug.com/316955. The first clobber try I sent failed at those tests. I sent
another. I hope it will pass this time.

[tfarina, 2013-11-11 15:02:13]

Tony, ping??

[tony, 2013-11-11 17:22:36]

LGTM

https://codereview.chromium.org/45753004/diff/720001/ui/ui_resources.gypi
File ui/ui_resources.gypi (right):

https://codereview.chromium.org/45753004/diff/720001/ui/ui_resources.gypi#new...
ui/ui_resources.gypi:51: {
Nit: Weird indent.  I guess the whole file has this.  We can fix it in a
separate change.

[tfarina, 2013-11-11 18:03:12]

Scott, can I get your approval for the rest of ui/ (Ben might be busy)?

Tony has reviewed everything and I have his approval for ui/base/resource/.

Thanks,

[sky, 2013-11-11 21:07:56]

LGTM

[commit-bot: I haz the power, 2013-11-11 21:12:50]

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

[commit-bot: I haz the power, 2013-11-12 13:24:05]

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

[commit-bot: I haz the power, 2013-11-12 16:24:43]

Change committed as 234537