Unpack theme data from extensions off of UI thread.

Unpack theme data from extensions off of UI thread.

Even on a beefy workstation, resizing theme images takes a long time
(>1s). This causes a painful hiccup in the UI. This patch addresses that
by moving the operation to a helper thread. The actual application of
the theme won't be any faster but it won't block interaction.

As a future area of investigation it might be worth changing the "theme
installed" infobar to show earlier with a message like "installing
theme..." until this operation is completed, since otherwise it can feel
like nothing is happening.

BUG=316070
Review-Url: https://codereview.chromium.org/2799003002
Cr-Commit-Position: refs/heads/master@{#476505}
Committed: https://chromium.googlesource.com/chromium/src/+/49c3b86554c09f298bd100c02c812be4aa2f0e15

[Evan Stade, 2017-04-05 18:04:11]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 18:04:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 18:57:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 18:57:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-04-06 01:41:29]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:42:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 02:03:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 02:03:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-04-06 20:11:21]

estade@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[Evan Stade, 2017-04-06 20:11:22]

Some theme related tests are still failing*, but before I go through the effort
of fixing those, wdyt?

*shouldn't be hard; basically just have to deal with theme install being async
now as I've done with several tests already

[pkotwicz, 2017-04-07 14:36:15]

When BrowserThemePack::kThemePackVersion is updated we rebeuild the theme from
the extension in ThemeService::MigrateTheme() on startup. We have avoided doing
BrowserThemePack::BuildFromExtension() off of the UI thread in the past because
it makes "startup when BrowserThemePack::kThemePackVersion has been updated look
really crappy"

[pkotwicz, 2017-04-07 14:38:16]

Perhaps we can do ThemeService::BuildFromExtension() off the UI thread only when
it is run as a result of a user installing a new theme?

[Evan Stade, 2017-04-11 21:59:13]

On 2017/04/07 14:36:15, pkotwicz wrote:
> "startup when BrowserThemePack::kThemePackVersion has been updated look
> really crappy"

in testing that I found that it looks no different to my eyes either way. It's
not going to take any more time to migrate on another thread, the difference is
whether we block the UI in the meantime, and I think the preference should be
not to do so.

However there was a bug that we weren't notifying that the theme changed when
migrating the theme, which meant some UI elements didn't update. That's fixed by
adding another NotifyThemeChanged call in OnThemeBuiltFromExtension().

I also moved the theme install infobar instantiation so that it only appears
after the theme is done loading (which happened before because the UI thread was
blocked).

[Evan Stade, 2017-04-11 22:01:09]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 22:02:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 22:34:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 22:34:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pkotwicz, 2017-04-13 01:01:59]

Looks the same to me on Linux. Can you test on Windows and Mac? On Linux, we
don't seem to paint anything prior to the migration finishing in
ThemeService::OnThemeBuiltFromExtension()

[Evan Stade, 2017-04-13 02:24:14]

On 2017/04/13 01:01:59, pkotwicz wrote:
> Looks the same to me on Linux. Can you test on Windows and Mac? On Linux, we
> don't seem to paint anything prior to the migration finishing in
> ThemeService::OnThemeBuiltFromExtension()

not easily except by pestering someone who has a mac or windows machine handy.
Where did you get the quote about migration? i.e. who said that? Could they be
consulted?

[pkotwicz, 2017-04-13 14:18:59]

I think that erg@ said this. He was the person who wrote most of what is still
the current themes code.

[Evan Stade, 2017-04-13 15:40:43]

estade@chromium.org changed reviewers:
+ erg@chromium.org

[Evan Stade, 2017-04-13 15:40:43]

+erg

Elliot, any recollection of this code or opinion on the above discussion
(notably, whether migration should be done on the UI thread or a helper)?

[Elliot Glaysher, 2017-04-13 17:44:04]

On 2017/04/13 15:40:43, Evan Stade wrote:
> +erg
> 
> Elliot, any recollection of this code or opinion on the above discussion
> (notably, whether migration should be done on the UI thread or a helper)?

The problem with migration is that anything could have happened between theme
pack revisions. When we've had bugs where someone changes the theme pack code,
but doesn't change the version number, we've had missing icons, icons used as
the tab background and other really bad visual glitches.

I agree that this is all unfortunate.

[Evan Stade, 2017-04-13 21:45:15]

On 2017/04/13 17:44:04, Elliot Glaysher wrote:
> On 2017/04/13 15:40:43, Evan Stade wrote:
> > +erg
> > 
> > Elliot, any recollection of this code or opinion on the above discussion
> > (notably, whether migration should be done on the UI thread or a helper)?
> 
> The problem with migration is that anything could have happened between theme
> pack revisions. When we've had bugs where someone changes the theme pack code,
> but doesn't change the version number, we've had missing icons, icons used as
> the tab background and other really bad visual glitches.
> 
> I agree that this is all unfortunate.

I'm not sure I understand. Why does it follow that the migration should happen
on the UI thread? If you don't bump the theme pack number you won't migrate at
all.

[Elliot Glaysher, 2017-04-13 22:21:48]

On 2017/04/13 21:45:15, Evan Stade wrote:
> On 2017/04/13 17:44:04, Elliot Glaysher wrote:
> > On 2017/04/13 15:40:43, Evan Stade wrote:
> > > +erg
> > > 
> > > Elliot, any recollection of this code or opinion on the above discussion
> > > (notably, whether migration should be done on the UI thread or a helper)?
> > 
> > The problem with migration is that anything could have happened between
theme
> > pack revisions. When we've had bugs where someone changes the theme pack
code,
> > but doesn't change the version number, we've had missing icons, icons used
as
> > the tab background and other really bad visual glitches.
> > 
> > I agree that this is all unfortunate.
> 
> I'm not sure I understand. Why does it follow that the migration should happen
> on the UI thread?

It doesn't have to happen on the UI thread. It does need to block initial UI
display, which is what I assumed this patch did based off the description. If
the idea is to just block initial display of any chrome UI, but perform the
migration off the UI thread, then that's probably acceptable.

> If you don't bump the theme pack number you won't migrate at
> all.

Correct. But you now have a divergence between the data in the theme pack and
the code that's using the theme pack. Let's say that you have a ver45 pack with
ver45 code. You conceptually modify the code to ver46 without bumping the pack
constant at the top. Reading a ver45 file is probably going to be wrong and I
listed some symptoms we've seen in the past. And this is analogous to what would
happen if you're trying to reuse an old theme pack for display while waiting for
a new theme pack, right? I'd assume an corrupted interface at startup is worse
than a small delay.

[Evan Stade, 2017-04-13 22:25:42]

On 2017/04/13 22:21:48, Elliot Glaysher wrote:
> On 2017/04/13 21:45:15, Evan Stade wrote:
> > On 2017/04/13 17:44:04, Elliot Glaysher wrote:
> > > On 2017/04/13 15:40:43, Evan Stade wrote:
> > > > +erg
> > > > 
> > > > Elliot, any recollection of this code or opinion on the above discussion
> > > > (notably, whether migration should be done on the UI thread or a
helper)?
> > > 
> > > The problem with migration is that anything could have happened between
> theme
> > > pack revisions. When we've had bugs where someone changes the theme pack
> code,
> > > but doesn't change the version number, we've had missing icons, icons used
> as
> > > the tab background and other really bad visual glitches.
> > > 
> > > I agree that this is all unfortunate.
> > 
> > I'm not sure I understand. Why does it follow that the migration should
happen
> > on the UI thread?
> 
> It doesn't have to happen on the UI thread. It does need to block initial UI
> display, which is what I assumed this patch did based off the description. If
> the idea is to just block initial display of any chrome UI, but perform the
> migration off the UI thread, then that's probably acceptable.
> 
> > If you don't bump the theme pack number you won't migrate at
> > all.
> 
> Correct. But you now have a divergence between the data in the theme pack and
> the code that's using the theme pack. Let's say that you have a ver45 pack
with
> ver45 code. You conceptually modify the code to ver46 without bumping the pack
> constant at the top. Reading a ver45 file is probably going to be wrong and I
> listed some symptoms we've seen in the past. And this is analogous to what
would
> happen if you're trying to reuse an old theme pack for display while waiting
for
> a new theme pack, right? I'd assume an corrupted interface at startup is worse
> than a small delay.

I would think it ideal to not use any theme while the migration is occurring.
I'll look at how hard that is to do.

[Evan Stade, 2017-04-17 20:02:40]

On 2017/04/13 22:25:42, Evan Stade wrote:
> On 2017/04/13 22:21:48, Elliot Glaysher wrote:
> > On 2017/04/13 21:45:15, Evan Stade wrote:
> > > On 2017/04/13 17:44:04, Elliot Glaysher wrote:
> > > > On 2017/04/13 15:40:43, Evan Stade wrote:
> > > > > +erg
> > > > > 
> > > > > Elliot, any recollection of this code or opinion on the above
discussion
> > > > > (notably, whether migration should be done on the UI thread or a
> helper)?
> > > > 
> > > > The problem with migration is that anything could have happened between
> > theme
> > > > pack revisions. When we've had bugs where someone changes the theme pack
> > code,
> > > > but doesn't change the version number, we've had missing icons, icons
used
> > as
> > > > the tab background and other really bad visual glitches.
> > > > 
> > > > I agree that this is all unfortunate.
> > > 
> > > I'm not sure I understand. Why does it follow that the migration should
> happen
> > > on the UI thread?
> > 
> > It doesn't have to happen on the UI thread. It does need to block initial UI
> > display, which is what I assumed this patch did based off the description.
If
> > the idea is to just block initial display of any chrome UI, but perform the
> > migration off the UI thread, then that's probably acceptable.
> > 
> > > If you don't bump the theme pack number you won't migrate at
> > > all.
> > 
> > Correct. But you now have a divergence between the data in the theme pack
and
> > the code that's using the theme pack. Let's say that you have a ver45 pack
> with
> > ver45 code. You conceptually modify the code to ver46 without bumping the
pack
> > constant at the top. Reading a ver45 file is probably going to be wrong and
I
> > listed some symptoms we've seen in the past. And this is analogous to what
> would
> > happen if you're trying to reuse an old theme pack for display while waiting
> for
> > a new theme pack, right? I'd assume an corrupted interface at startup is
worse
> > than a small delay.
> 
> I would think it ideal to not use any theme while the migration is occurring.
> I'll look at how hard that is to do.

So it turns out that while we're waiting to migrate, we fall back to the default
theme, not an old/corrupted theme pack.

[Evan Stade, 2017-04-17 20:02:49]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 20:03:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 20:37:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 20:37:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-04-19 15:39:22]

On 2017/04/17 20:02:40, Evan Stade wrote:
> 
> So it turns out that while we're waiting to migrate, we fall back to the
default
> theme, not an old/corrupted theme pack.

ping

[Elliot Glaysher, 2017-04-19 17:24:29]

(pinging who?)

[Evan Stade, 2017-04-19 17:59:58]

On 2017/04/19 17:24:29, Elliot Glaysher wrote:
> (pinging who?)

Both. I think I responded to your concern re: migration, unless I missed
something. I think this patch is correct aside from some more tests that need
updating. WDYT?

[pkotwicz, 2017-04-19 18:50:05]

erg@: Has estade@ addressed your concern about migration?

[Elliot Glaysher, 2017-04-19 19:24:40]

lgtm as long as ux is fine with it. i still think having a flash of a different
theme is worse, but it's not really my call.

[Evan Stade, 2017-04-21 17:39:07]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 17:39:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 18:04:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 18:04:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-04-26 15:15:59]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 15:16:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 15:43:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 15:43:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-04-27 20:24:16]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 20:24:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 20:29:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 20:29:47]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2017-04-27 20:49:41]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 20:50:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 21:12:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 21:12:49]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Evan Stade, 2017-04-28 02:10:09]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 02:11:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 03:10:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 03:10:11]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Evan Stade, 2017-05-02 02:18:02]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 02:18:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 02:35:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 02:35:10]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Evan Stade, 2017-05-02 16:23:49]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 16:24:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 16:53:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 16:53:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-05-02 23:24:05]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 23:24:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 00:09:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:09:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Evan Stade, 2017-05-25 23:12:08]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 23:12:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 00:02:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 00:02:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-05-31 21:33:45]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 21:34:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-05-31 21:34:44]

On 2017/04/19 19:24:40, Elliot Glaysher wrote:
> lgtm as long as ux is fine with it. i still think having a flash of a
different
> theme is worse, but it's not really my call.

Sorry this took me a while to get back to. Wrangling all the tests proved very
time consuming.

I've kept the migration path on the UI thread. PTAL.

[Elliot Glaysher, 2017-05-31 22:13:25]

c/b/themes lgtm

(i'm not really sure about the extensions and sync code though; those parts are
really hazy to me.)

[Evan Stade, 2017-05-31 22:16:06]

estade@chromium.org changed reviewers:
+ pavely@chromium.org, rdevlin.cronin@chromium.org

[Evan Stade, 2017-05-31 22:16:07]

+pavely for sync
+rdevlin.cronin for extensions

PTAL

[commit-bot: I haz the power, 2017-05-31 22:24:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 22:24:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-06-01 00:28:09]

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_ui_browsertest.cc:73:
EXPECT_EQ(num_before + (theme_exists ? 0 : 1), num_after);
Adding a comment explaining the (theme_exists ? 0 : 1) would be helpful.

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_install_ui_default.cc (left):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_install_ui_default.cc:174:
ThemeInstalledInfoBarDelegate::Create(
Does moving this out of here and into ThemeService::OnThemeBuiltFromExtension()
result in the infobar being shown significantly later?  I think this method was
called prior to the theme initialization work, which meant there was some
instant UI feedback (though as the CL description notes, that feedback was a
little strange).  Is that correct?  If so, would it be worth keeping the bar
showing early?

[pkotwicz, 2017-06-01 03:21:09]

Just nits!

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:892: NotifyThemeChanged();
Can this be simplified as:

const std::string previous_theme_id = GetThemeID();
const bool previous_using_system_theme = UsingSystemTheme();

// Clear our image cache.
FreePlatformCaches();
SaveThemeID(extension->id());
NotifyThemeChanged();

if (previous_theme_id != extension_id)
  return;

I am unsure whether your current code works for themes which auto update
themselves using the mechanism in
https://developer.chrome.com/extensions/autoupdate I am unsure what happens (if
anything) when a theme is updated via the Web Store

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:897: previous_theme_id == kDefaultThemeID
|| previous_using_system_theme;
The system theme uses kDefaultThemeID

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:898: if (previous_theme_id !=
kDefaultThemeID &&
Can

previous_theme_id != kDefaultThemeId

be simplified to

!previous_theme_available

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:899: previous_theme_id != extension_id &&
I think that the following is always true

previous_theme_id != extension_id

due to early exit on line 883

[pavely, 2017-06-01 16:52:49]

c/b/sync/test lgtm

[Evan Stade, 2017-06-01 17:43:02]

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_ui_browsertest.cc:73:
EXPECT_EQ(num_before + (theme_exists ? 0 : 1), num_after);
On 2017/06/01 00:28:09, Devlin wrote:
> Adding a comment explaining the (theme_exists ? 0 : 1) would be helpful.

Done.

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:892: NotifyThemeChanged();
On 2017/06/01 03:21:08, pkotwicz wrote:
> Can this be simplified as:
> 
> const std::string previous_theme_id = GetThemeID();
> const bool previous_using_system_theme = UsingSystemTheme();
> 
> // Clear our image cache.
> FreePlatformCaches();
> SaveThemeID(extension->id());
> NotifyThemeChanged();
> 
> if (previous_theme_id != extension_id)
>   return;
> 
> I am unsure whether your current code works for themes which auto update
> themselves using the mechanism in
> https://developer.chrome.com/extensions/autoupdate I am unsure what happens
(if
> anything) when a theme is updated via the Web Store

seems safe to make that change. FreePlatformCaches is a no-op on !mac and
SaveThemeID with the same ID will be one as well. Seems like mac might want
FreePlatformCaches.

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:897: previous_theme_id == kDefaultThemeID
|| previous_using_system_theme;
On 2017/06/01 03:21:08, pkotwicz wrote:
> The system theme uses kDefaultThemeID
> 

pretty sure this is necessary for GTK to work

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:898: if (previous_theme_id !=
kDefaultThemeID &&
On 2017/06/01 03:21:08, pkotwicz wrote:
> Can
> 
> previous_theme_id != kDefaultThemeId
> 
> be simplified to
> 
> !previous_theme_available

perhaps, but that reads really strangely IMO, because in this case the previous
theme may actually be available.

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:899: previous_theme_id != extension_id &&
On 2017/06/01 03:21:09, pkotwicz wrote:
> I think that the following is always true
> 
> previous_theme_id != extension_id
> 
> due to early exit on line 883

Done.

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_install_ui_default.cc (left):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_install_ui_default.cc:174:
ThemeInstalledInfoBarDelegate::Create(
On 2017/06/01 00:28:09, Devlin wrote:
> Does moving this out of here and into
ThemeService::OnThemeBuiltFromExtension()
> result in the infobar being shown significantly later?  I think this method
was
> called prior to the theme initialization work, which meant there was some
> instant UI feedback (though as the CL description notes, that feedback was a
> little strange).  Is that correct?  If so, would it be worth keeping the bar
> showing early?

Before, the browser would immediately hang for a second or two, then when it
resumes functioning, the theme is present and the infobar is showing.

Now, the browser continues to work normally for a second or two, then the theme
and infobar show up after the unpacking delay.

The latter seems strictly preferable. Hanging as a means of "instant feedback"
doesn't seem like a win. :) I did not want to move the infobar's appearance to
before the theme actually installed because then you can press undo and nothing
happens.

[Devlin, 2017-06-01 18:19:58]

lgtm

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_install_ui_default.cc (left):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_install_ui_default.cc:174:
ThemeInstalledInfoBarDelegate::Create(
On 2017/06/01 17:43:02, Evan Stade wrote:
> On 2017/06/01 00:28:09, Devlin wrote:
> > Does moving this out of here and into
> ThemeService::OnThemeBuiltFromExtension()
> > result in the infobar being shown significantly later?  I think this method
> was
> > called prior to the theme initialization work, which meant there was some
> > instant UI feedback (though as the CL description notes, that feedback was a
> > little strange).  Is that correct?  If so, would it be worth keeping the bar
> > showing early?
> 
> Before, the browser would immediately hang for a second or two, then when it
> resumes functioning, the theme is present and the infobar is showing.
> 
> Now, the browser continues to work normally for a second or two, then the
theme
> and infobar show up after the unpacking delay.
> 
> The latter seems strictly preferable. Hanging as a means of "instant feedback"
> doesn't seem like a win. :) I did not want to move the infobar's appearance to
> before the theme actually installed because then you can press undo and
nothing
> happens.

Okay, cool.  My concern was if before the flow was:
Click install
Infobar shows
Browser hangs

and now it would be
click install
<second gap>
infobar shows

If the flow was
click install
browser hangs
infobar shows

Then yes, this is definitely better. :)  I do still like your suggestion from
the CL description about showing the infobar sooner with some kind of status;
can we file a bug for that (or add it to the existing one)?

https://codereview.chromium.org/2799003002/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

https://codereview.chromium.org/2799003002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_ui_browsertest.cc:65: size_t
num_before = extensions::ExtensionRegistry::Get(profile())
optional nit: cache registry?
extensions::ExtensionRegistry* registry =
extensions::ExtensionRegistry::Get(profile());
size_t num_before = registry->enabled_extensions().size();
...
size_t num_after = registry->enabled_extensions().size();

[pkotwicz, 2017-06-01 19:09:06]

LGTM with nit

Please merge with https://codereview.chromium.org/2893693002/

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:897: previous_theme_id == kDefaultThemeID
|| previous_using_system_theme;
That's untrue.

ThemeServiceAuraX11::UseSystemTheme() invokes the following call chain

ThemeServiceAuraX11::UseSystemTheme() -> ThemeService::SetCustomDefaultTheme()
-> ThemeService::ClearAllThemeData() -> SaveThemeID(kDefaultThemeID)

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:898: if (previous_theme_id !=
kDefaultThemeID &&
Fair enough

[Evan Stade, 2017-06-01 23:38:23]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 23:39:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-06-01 23:58:52]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Evan Stade, 2017-06-01 23:58:54]

+pkasting can you PTAL 
    chrome/browser/infobars/infobars_browsertest.cc
    chrome/browser/ui/search/instant_extended_interactive_uitest.cc

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2799003002/diff/260001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:897: previous_theme_id == kDefaultThemeID
|| previous_using_system_theme;
On 2017/06/01 19:09:05, pkotwicz wrote:
> That's untrue.
> 
> ThemeServiceAuraX11::UseSystemTheme() invokes the following call chain
> 
> ThemeServiceAuraX11::UseSystemTheme() -> ThemeService::SetCustomDefaultTheme()
> -> ThemeService::ClearAllThemeData() -> SaveThemeID(kDefaultThemeID)

ok, thanks for pointing this out.

[Evan Stade, 2017-06-01 23:59:04]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 23:59:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2017-06-02 00:01:43]

Those two tests LGTM

[Evan Stade, 2017-06-02 00:18:35]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-06-02 00:18:53]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-06-02 00:18:54]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
pavely@chromium.org, pkotwicz@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2799003002/#ps320001
(title: "fix gtk case")

[commit-bot: I haz the power, 2017-06-02 00:20:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 00:59:45]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1496362733792300,
"parent_rev": "30a6b0e99b44267c4798f54e46cf81f82f399630", "commit_rev":
"49c3b86554c09f298bd100c02c812be4aa2f0e15"}

[commit-bot: I haz the power, 2017-06-02 00:59:57]

Description was changed from

==========
Unpack theme data from extensions off of UI thread.

Even on a beefy workstation, resizing theme images takes a long time
(>1s). This causes a painful hiccup in the UI. This patch addresses that
by moving the operation to a helper thread. The actual application of
the theme won't be any faster but it won't block interaction.

As a future area of investigation it might be worth changing the "theme
installed" infobar to show earlier with a message like "installing
theme..." until this operation is completed, since otherwise it can feel
like nothing is happening.

BUG=316070
==========

to

==========
Unpack theme data from extensions off of UI thread.

Even on a beefy workstation, resizing theme images takes a long time
(>1s). This causes a painful hiccup in the UI. This patch addresses that
by moving the operation to a helper thread. The actual application of
the theme won't be any faster but it won't block interaction.

As a future area of investigation it might be worth changing the "theme
installed" infobar to show earlier with a message like "installing
theme..." until this operation is completed, since otherwise it can feel
like nothing is happening.

BUG=316070

Review-Url: https://codereview.chromium.org/2799003002
Cr-Commit-Position: refs/heads/master@{#476505}
Committed:
https://chromium.googlesource.com/chromium/src/+/49c3b86554c09f298bd100c02c81...
==========

[commit-bot: I haz the power, 2017-06-02 00:59:59]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/49c3b86554c09f298bd100c02c81...

[yoichio, 2017-06-02 04:37:05]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2919953002/ by yoichio@chromium.org.

The reason for reverting is: This causes ThemeServiceBrowserTest test failure on
Linux:
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom....