Introduce FastShowPickler which will be used for app list cold start.

Introduce FastShowPickler which will be used for app list cold start.

This class will be responsible for serializing a copy of AppListModel to
populate a dummy AppListView that is shown immediately on startup if
--show-app-list is used.

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

[koz (OOO until 15th September), 2013-08-28 08:19:54]

benwells: for general review
xiyuan: do you mind checking my use of SkBitmap, gfx::ImageSkia and friends? :-)

[xiyuan, 2013-08-28 17:00:39]

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:41: if (!it->ReadBytes(&pixels,
size))
Can we ReadBytes into SkBitmap's pixel buffer to save a copy?

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:85:
pickle->WriteInt(it->sk_bitmap().getConfig());
I might be a paranoid so not feel comfortable use the enum value directly. Maybe
we should do an explicit translation here to make the persisted data not depend
on the enum declaration in SkBitmap.

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:86: int size =
static_cast<int>(it->sk_bitmap().getSize());
getSafeSize() might be better for the usage here.

[koz (OOO until 15th September), 2013-08-29 03:19:01]

Thanks, Xiyuan!

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:41: if (!it->ReadBytes(&pixels,
size))
On 2013/08/28 17:00:39, xiyuan wrote:
> Can we ReadBytes into SkBitmap's pixel buffer to save a copy?

ReadBytes() has the effect of pointing |pixels| into the pickle's internal
buffer (as per the comment on base/pickle.h:PickleIterator::ReadBytes()), so
this isn't a copy. The memcpy() later has the effect of copying straight out of
the pickle into the SkBitmap.

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:85:
pickle->WriteInt(it->sk_bitmap().getConfig());
On 2013/08/28 17:00:39, xiyuan wrote:
> I might be a paranoid so not feel comfortable use the enum value directly.
Maybe
> we should do an explicit translation here to make the persisted data not
depend
> on the enum declaration in SkBitmap.

Done.

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:86: int size =
static_cast<int>(it->sk_bitmap().getSize());
On 2013/08/28 17:00:39, xiyuan wrote:
> getSafeSize() might be better for the usage here.

Done.

[benwells, 2013-08-29 04:49:28]

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:6: 
Nit: extra blank line.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:166: gfx::ImageSkia icon =
UnpickleImage(it);
Should you check icon.empty (or whatever the call is)? Error handling seems to
be covered for all other things that could go wrong.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:201: if (!result->WriteInt((int)
model->apps()->item_count()))
Nit: If it was me I'd swap the item_count and signed_in. item_count just before
the list of items feels better.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler.h (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.h:16: 
Nit: extra blank line.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.h:27: static scoped_ptr<Pickle>
PickleAppListModelForFastShow(
Nit: Maybe don't need 'ForFastShow' in the name as you'll call it via
FastShowPickler::PickleAppListModelForFastShow. Likewise for unpickle.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.h:37: static void CopyOver(
What is this going to be used for?

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler_unittest.cc (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler_unittest.cc:13: 
Nit: extra blank line.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler_unittest.cc:102:
TEST_F(AppListModelPicklerUnitTest, Images) {
nit: pickling of signed in isn't being tested.

https://codereview.chromium.org/23684003/diff/6001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:3121: # compile/link/test cycles during
development.
\o/

[xiyuan, 2013-08-29 16:52:11]

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/23684003/diff/1/chrome/browser/ui/app_list/fa...
chrome/browser/ui/app_list/fast_show_pickler.cc:41: if (!it->ReadBytes(&pixels,
size))
On 2013/08/29 03:19:01, koz wrote:
> On 2013/08/28 17:00:39, xiyuan wrote:
> > Can we ReadBytes into SkBitmap's pixel buffer to save a copy?
> 
> ReadBytes() has the effect of pointing |pixels| into the pickle's internal
> buffer (as per the comment on base/pickle.h:PickleIterator::ReadBytes()), so
> this isn't a copy. The memcpy() later has the effect of copying straight out
of
> the pickle into the SkBitmap.

You are right. Apparently, I have never used Pickle before. :p

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:83:
std::vector<gfx::ImageSkiaRep> reps(image.image_reps());
I just thought of potential problem. ExtensionAppItem uses extensions::IconImage
for its icon. IconImage is lazily loaded. It only loads the image when
GetRepresentation of its exposed ImageSkia is called (during paint or called
explicitly as the preload).

We probably need to force the icons to be loaded before the pickling code.
Otherwise, we probably only persist the icons of the visible page.

[koz (OOO until 15th September), 2013-08-30 00:44:50]

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler.cc (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:6: 
On 2013/08/29 04:49:28, benwells wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:83:
std::vector<gfx::ImageSkiaRep> reps(image.image_reps());
On 2013/08/29 16:52:11, xiyuan wrote:
> I just thought of potential problem. ExtensionAppItem uses
extensions::IconImage
> for its icon. IconImage is lazily loaded. It only loads the image when
> GetRepresentation of its exposed ImageSkia is called (during paint or called
> explicitly as the preload).
> 
> We probably need to force the icons to be loaded before the pickling code.
> Otherwise, we probably only persist the icons of the visible page.

I plan on handling that in a higher layer. There will be a service that listens
to changes in the AppListModel and schedules pickling to occur when necessary,
an icon loading or changing will prompt a re-pickle.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:166: gfx::ImageSkia icon =
UnpickleImage(it);
On 2013/08/29 04:49:28, benwells wrote:
> Should you check icon.empty (or whatever the call is)? Error handling seems to
> be covered for all other things that could go wrong.

Ah, good point. gfx::ImageSkia() is a valid value so I can't use it as a
sentinel anyway. I've changed this function to use an out param.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler.cc:201: if (!result->WriteInt((int)
model->apps()->item_count()))
On 2013/08/29 04:49:28, benwells wrote:
> Nit: If it was me I'd swap the item_count and signed_in. item_count just
before
> the list of items feels better.

Done.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/fast_show_pickler_unittest.cc (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler_unittest.cc:13: 
On 2013/08/29 04:49:28, benwells wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/23684003/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/fast_show_pickler_unittest.cc:102:
TEST_F(AppListModelPicklerUnitTest, Images) {
On 2013/08/29 04:49:28, benwells wrote:
> nit: pickling of signed in isn't being tested.

Yes, true. And indeed, wasn't implemented :-| Nice spot!

https://codereview.chromium.org/23684003/diff/6001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/23684003/diff/6001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:3121: # compile/link/test cycles during
development.
On 2013/08/29 04:49:28, benwells wrote:
> \o/

From 6 minutes to 6 seconds!

[tapted, 2013-08-30 01:13:34]

https://codereview.chromium.org/23684003/diff/16001/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/23684003/diff/16001/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:3132:
'browser/ui/app_list/chrome_app_list_unit_tests.cc',
Drive-by: should this go in a test subfolder. I.e.
chrome/browser/ui/app_list/test/ ?

And I think it's more common for there to be no underscore in unit_tests. i.e.
target/file = chrome_app_list_unittests  (to match e.g. ui_unittests,
base_unittests, etc.).

Poking around... it may even be convention to have the source file always called
'run_all_unittests.cc', with the target/path deciding what it's actually for..
But these all look the following, so maybe work different (I haven't really
played that much with gtest..)


#include "base/test/test_suite.h"

int main(int argc, char** argv) {
  return base::TestSuite(argc, argv).Run();
}

[koz (OOO until 15th September), 2013-08-30 05:54:23]

https://codereview.chromium.org/23684003/diff/16001/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/23684003/diff/16001/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:3132:
'browser/ui/app_list/chrome_app_list_unit_tests.cc',
On 2013/08/30 01:13:35, tapted wrote:
> Drive-by: should this go in a test subfolder. I.e.
> chrome/browser/ui/app_list/test/ ?
> 
> And I think it's more common for there to be no underscore in unit_tests. i.e.
> target/file = chrome_app_list_unittests  (to match e.g. ui_unittests,
> base_unittests, etc.).
> 
> Poking around... it may even be convention to have the source file always
called
> 'run_all_unittests.cc', with the target/path deciding what it's actually for..
> But these all look the following, so maybe work different (I haven't really
> played that much with gtest..)
> 
> 
> #include "base/test/test_suite.h"
> 
> int main(int argc, char** argv) {
>   return base::TestSuite(argc, argv).Run();
> }

Ooh, thanks for the suggestion Trent. It seems that such a file isn't even
needed if we depend on base/base.gyp:run_all_unittests which will link in a
main()/test runner for you.

[benwells, 2013-08-30 07:53:55]

lgtm

[commit-bot: I haz the power, 2013-09-13 20:19:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/20001

[commit-bot: I haz the power, 2013-09-13 20:37:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-18 01:48:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/28001

[commit-bot: I haz the power, 2013-09-18 02:10:36]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, net_unittests, sql_unittests,
ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-18 07:55:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/26001

[commit-bot: I haz the power, 2013-09-18 08:27:01]

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

[commit-bot: I haz the power, 2013-09-19 01:06:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/94001

[commit-bot: I haz the power, 2013-09-19 01:31:28]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, net_unittests, sql_unittests,
ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-20 09:22:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/121001

[commit-bot: I haz the power, 2013-09-20 11:27:22]

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

[commit-bot: I haz the power, 2013-09-20 20:06:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/121001

[commit-bot: I haz the power, 2013-09-20 22:25:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-23 00:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/138001

[commit-bot: I haz the power, 2013-09-23 01:00:16]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-23 01:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/156001

[commit-bot: I haz the power, 2013-09-23 04:29:25]

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

[commit-bot: I haz the power, 2013-09-23 04:34:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/23684003/156001

[commit-bot: I haz the power, 2013-09-23 09:36:25]

Change committed as 224699