Upon execution of the App Host, ask Omaha to install the Chrome Binaries if they are not present on…

Upon execution of the App Host, ask Omaha to install the Chrome Binaries if they are not present on the system.

BUG=138313
TEST=Install Chrome at system level. Install the app host. Uninstall Chrome. Launch the app host. NB: The installation will fail until r151283 makes it to dev channel (or whichever channel Chrome is originally installed at).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154627

[erikwright (departed), 2012-08-15 14:43:02]

Hi Robert,

Please draw upon your Omaha and COM wisdom for an initial review. I will ask an
Apps team member to do the final review.

NB that I assume this will eventually want to display some kind of UI, possibly
implying a message loop, but that it is intentionally absent in this cut.

Thanks,

Erik

[robertshield, 2012-08-16 13:08:16]

nice looking patch :) comments and musings below

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/app_host_main.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/app_host_main.cc:22: LOG(INFO) << "No Chrome
executable could be found. Let's install it.";
Does the app host / Omaha show any kind of UI while this is happening?

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/binaries_installer.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer.cc:24: // Blocks until the
installation process completes, without message pumping.
What's the end state of the Omaha registry keys if this fails? Does it leave
behind any binaries-related keys in Clients or ClientState?

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer.cc:26: HRESULT hr =
::CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
Suggest using the stuff in base/win/scoped_com_initializer.h

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:89: return
true;
as a readability thing, could we reduce the number of return statements? I'd
specifically prefer if there were no return statements in the switch block as it
imo makes it harder to figure which cases fall through below and which don't.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:110: *hr =
S_OK;
this should be warning about unreachable code

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:194: return
hr;
I would encourage you to rewrite stuff like this with fewer return statements as
well, though I don't feel as strongly for shorter functions.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:210: HRESULT
CreateBinariesIApp(IAppBundle* app_bundle, BSTR ap, IApp** app) {
app, ap, and app_bundle :) maybe ap -> "channel_value"?

[erikwright (departed), 2012-08-16 21:05:15]

PTAL.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/app_host_main.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/app_host_main.cc:22: LOG(INFO) << "No Chrome
executable could be found. Let's install it.";
On 2012/08/16 13:08:16, robertshield wrote:
> Does the app host / Omaha show any kind of UI while this is happening?

Not at the moment. App Host probably should before a full consumer launch
though.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/binaries_installer.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer.cc:24: // Blocks until the
installation process completes, without message pumping.
On 2012/08/16 13:08:16, robertshield wrote:
> What's the end state of the Omaha registry keys if this fails? Does it leave
> behind any binaries-related keys in Clients or ClientState?

It's presumably the equivalent of a failure after a download of the tagged meta
installer.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer.cc:26: HRESULT hr =
::CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
On 2012/08/16 13:08:16, robertshield wrote:
> Suggest using the stuff in base/win/scoped_com_initializer.h

I was shocked such a thing didn't exist, but I guess I missed this one in my
grepping somehow.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:89: return
true;
On 2012/08/16 13:08:16, robertshield wrote:
> as a readability thing, could we reduce the number of return statements? I'd
> specifically prefer if there were no return statements in the switch block as
it
> imo makes it harder to figure which cases fall through below and which don't.

Done.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:110: *hr =
S_OK;
On 2012/08/16 13:08:16, robertshield wrote:
> this should be warning about unreachable code

Done.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:194: return
hr;
On 2012/08/16 13:08:16, robertshield wrote:
> I would encourage you to rewrite stuff like this with fewer return statements
as
> well, though I don't feel as strongly for shorter functions.

Done.

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:210: HRESULT
CreateBinariesIApp(IAppBundle* app_bundle, BSTR ap, IApp** app) {
On 2012/08/16 13:08:16, robertshield wrote:
> app, ap, and app_bundle :) maybe ap -> "channel_value"?

What does AP stand for?

[robertshield, 2012-08-16 21:15:53]

most excellent return scrubbing, one last question

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:210: HRESULT
CreateBinariesIApp(IAppBundle* app_bundle, BSTR ap, IApp** app) {
On 2012/08/16 21:05:15, erikwright wrote:
> On 2012/08/16 13:08:16, robertshield wrote:
> > app, ap, and app_bundle :) maybe ap -> "channel_value"?
> 
> What does AP stand for?

That knowledge is lost in the sands of time. Omaha folk might know.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:37: HRESULT hr
= CheckIsBusy(app_bundle, &is_busy);
what does it mean for the app  bundle to be 'busy' on receiving one of the
states that call OnUpdateAvailable or OnReadyToInstall? are these states
guaranteed to occur again while the app bundle is in a non-busy state?

[erikwright (departed), 2012-08-16 21:49:36]

On 2012/08/16 21:15:53, robertshield wrote:
> most excellent return scrubbing, one last question
> 
>
http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
> File chrome/browser/extensions/app_host/binaries_installer_internal.cc
(right):
> 
>
http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
> chrome/browser/extensions/app_host/binaries_installer_internal.cc:210: HRESULT
> CreateBinariesIApp(IAppBundle* app_bundle, BSTR ap, IApp** app) {
> On 2012/08/16 21:05:15, erikwright wrote:
> > On 2012/08/16 13:08:16, robertshield wrote:
> > > app, ap, and app_bundle :) maybe ap -> "channel_value"?
> > 
> > What does AP stand for?
> 
> That knowledge is lost in the sands of time. Omaha folk might know.
> 
>
http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
> File chrome/browser/extensions/app_host/binaries_installer_internal.cc
(right):
> 
>
http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
> chrome/browser/extensions/app_host/binaries_installer_internal.cc:37: HRESULT
hr
> = CheckIsBusy(app_bundle, &is_busy);
> what does it mean for the app  bundle to be 'busy' on receiving one of the
> states that call OnUpdateAvailable or OnReadyToInstall? are these states
> guaranteed to occur again while the app bundle is in a non-busy state?

That is a great question. The state applies to the app, and my experience was
that the app might be in a useful state but the bundle might not be. Since a
bundle could contain multiple apps, it makes sense that you would have to wait
until all apps were ready to move forward. In this case, there is only one app,
but I guess there is a bit of a race condition where the app is ready but the
bundle isn't quite there yet. In any case, the app should not change state
during that time.

In my experience, with no timeout (busy loop checking app state), I did hit a
busy bundle (which fails upon attempts to transition to the next state) even
though the app was ready, and adding this fixed it.

[robertshield, 2012-08-17 00:14:08]

On 2012/08/16 21:49:36, erikwright wrote:
> On 2012/08/16 21:15:53, robertshield wrote:
> > most excellent return scrubbing, one last question
> > 
> >
>
http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
> > File chrome/browser/extensions/app_host/binaries_installer_internal.cc
> (right):
> > 
> >
>
http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
> > chrome/browser/extensions/app_host/binaries_installer_internal.cc:210:
HRESULT
> > CreateBinariesIApp(IAppBundle* app_bundle, BSTR ap, IApp** app) {
> > On 2012/08/16 21:05:15, erikwright wrote:
> > > On 2012/08/16 13:08:16, robertshield wrote:
> > > > app, ap, and app_bundle :) maybe ap -> "channel_value"?
> > > 
> > > What does AP stand for?
> > 
> > That knowledge is lost in the sands of time. Omaha folk might know.
> > 
> >
>
http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
> > File chrome/browser/extensions/app_host/binaries_installer_internal.cc
> (right):
> > 
> >
>
http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
> > chrome/browser/extensions/app_host/binaries_installer_internal.cc:37:
HRESULT
> hr
> > = CheckIsBusy(app_bundle, &is_busy);
> > what does it mean for the app  bundle to be 'busy' on receiving one of the
> > states that call OnUpdateAvailable or OnReadyToInstall? are these states
> > guaranteed to occur again while the app bundle is in a non-busy state?
> 
> That is a great question. The state applies to the app, and my experience was
> that the app might be in a useful state but the bundle might not be. Since a
> bundle could contain multiple apps, it makes sense that you would have to wait
> until all apps were ready to move forward. In this case, there is only one
app,
> but I guess there is a bit of a race condition where the app is ready but the
> bundle isn't quite there yet. In any case, the app should not change state
> during that time.
> 
> In my experience, with no timeout (busy loop checking app state), I did hit a
> busy bundle (which fails upon attempts to transition to the next state) even
> though the app was ready, and adding this fixed it.

Ok, interesting. Makes sense to keep it then if this helps get through a
transient state. One last question (I promise!): what guarantee is there that
the app helper install process terminates? The CheckIfDone() loop in
binaries_installer.cc doesn't have a timeout, it looks like we rely on Omaha
transitioning correctly between states. I'm thinking that adding a timeout here
as well would be prudent. What say you?

[robertshield, 2012-08-17 18:55:40]

On 2012/08/17 00:14:08, robertshield wrote:
> On 2012/08/16 21:49:36, erikwright wrote:
> > On 2012/08/16 21:15:53, robertshield wrote:
> > > most excellent return scrubbing, one last question
> > > 
> > >
> >
>
http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
> > > File chrome/browser/extensions/app_host/binaries_installer_internal.cc
> > (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10825364/diff/1/chrome/browser/extensions/app_...
> > > chrome/browser/extensions/app_host/binaries_installer_internal.cc:210:
> HRESULT
> > > CreateBinariesIApp(IAppBundle* app_bundle, BSTR ap, IApp** app) {
> > > On 2012/08/16 21:05:15, erikwright wrote:
> > > > On 2012/08/16 13:08:16, robertshield wrote:
> > > > > app, ap, and app_bundle :) maybe ap -> "channel_value"?
> > > > 
> > > > What does AP stand for?
> > > 
> > > That knowledge is lost in the sands of time. Omaha folk might know.
> > > 
> > >
> >
>
http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
> > > File chrome/browser/extensions/app_host/binaries_installer_internal.cc
> > (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
> > > chrome/browser/extensions/app_host/binaries_installer_internal.cc:37:
> HRESULT
> > hr
> > > = CheckIsBusy(app_bundle, &is_busy);
> > > what does it mean for the app  bundle to be 'busy' on receiving one of the
> > > states that call OnUpdateAvailable or OnReadyToInstall? are these states
> > > guaranteed to occur again while the app bundle is in a non-busy state?
> > 
> > That is a great question. The state applies to the app, and my experience
was
> > that the app might be in a useful state but the bundle might not be. Since a
> > bundle could contain multiple apps, it makes sense that you would have to
wait
> > until all apps were ready to move forward. In this case, there is only one
> app,
> > but I guess there is a bit of a race condition where the app is ready but
the
> > bundle isn't quite there yet. In any case, the app should not change state
> > during that time.
> > 
> > In my experience, with no timeout (busy loop checking app state), I did hit
a
> > busy bundle (which fails upon attempts to transition to the next state) even
> > though the app was ready, and adding this fixed it.
> 
> Ok, interesting. Makes sense to keep it then if this helps get through a
> transient state. One last question (I promise!): what guarantee is there that
> the app helper install process terminates? The CheckIfDone() loop in
> binaries_installer.cc doesn't have a timeout, it looks like we rely on Omaha
> transitioning correctly between states. I'm thinking that adding a timeout
here
> as well would be prudent. What say you?

As discussed, LGTM

[erikwright (departed), 2012-08-21 16:17:27]

jhawkins: chrome/chrome_browser_extensions.gypi only
miket: the rest.

[miket_OOO, 2012-08-23 23:51:00]

LGTM. I haven't written Win-specific Chrome code, so my review might be missing
some platform-specific style idioms.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_host/binaries_installer.cc (right):

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer.cc:13: #include
"google_update/google_update_idl.h"
Please alphabetize.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer.cc:22: }  // namespace
You can leave out the closing comment in short cases like this where it's
unlikely the reader will get lost.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer.cc:33: }
Since you're not early-returning on failure, I expect to see unconditional work
done somewhere in this method, but there isn't. Why not just return hr in case
of failure? This is a bit of a style question, and if it feels better to you as
it is, it's not a big deal. It does read a little weird to me... waiting for the
other shoe to drop.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer.cc:63: while
(!internal::CheckIfDone(app_bundle, app, &hr)) {
Is this guaranteed to return true eventually? If not, we should probably pick a
reasonable timeout instead of looping potentially forever.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_host/binaries_installer.h (right):

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer.h:14: // be used, if any.
It reduces to the same thing in either case, but to reduce ambiguity, can you
move "if any" to directly follow the thing it's referring to? (probably AP
value)

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:27: LOG(ERROR)
<< "Failed to check app_bundle->isBusy: " << hr;
No braces needed for these one-line blocks.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:35: // If the
app bundle is 'busy' we will just wait some more.
Why quotes around busy? Does it mean something different from its usual meaning?

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:72: <<
completion_message_hr;
one-liner braces

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:96: LOG(ERROR)
<< "Unexpected error: " << hr;
Might as well give a little more context for this log statement, so that users
don't report "I got Unexpected error 12345, fix my bug and BTW you suck" that we
can't easily trace back to the source.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:134: bool
CheckIfDone(IAppBundle* app_bundle, IApp* app, HRESULT* hr) {
Suggestion: if you have a local hr that's copied to *hr at the bottom of the
block only if *hr is non-null, then in your sleep-wait back in the other file,
you can pass in null instead of receiving an ignored result (I'll have to look
back to check, but I don't think you do anything with that result).

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:262: *app =
temp_app.Detach();
This could slide, but I can see cases where temp_app leaks.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:270: if
(FAILED(hr)) {
It's slightly easier to read if the SUCCEEDED() case comes first.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_host/binaries_installer_internal.h (right):

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.h:26: HRESULT
GetAppHostAPValue(IGoogleUpdate3* update3, BSTR* ap_value);
Be consistent in capitalization of AP in this file's function names.

[James Hawkins, 2012-08-24 16:10:57]

.gypi lgtm

[erikwright (departed), 2012-08-31 21:04:48]

FYI.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/app_host/binaries_installer.cc (right):

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer.cc:13: #include
"google_update/google_update_idl.h"
On 2012/08/23 23:51:01, miket wrote:
> Please alphabetize.

Done.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer.cc:33: }
On 2012/08/23 23:51:01, miket wrote:
> Since you're not early-returning on failure, I expect to see unconditional
work
> done somewhere in this method, but there isn't. Why not just return hr in case
> of failure? This is a bit of a style question, and if it feels better to you
as
> it is, it's not a big deal. It does read a little weird to me... waiting for
the
> other shoe to drop.

Other reviewer's (robertshield) preference to minimize return points for
readability. Since I tend towards the same, I'll let him win :)

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer.cc:63: while
(!internal::CheckIfDone(app_bundle, app, &hr)) {
On 2012/08/23 23:51:01, miket wrote:
> Is this guaranteed to return true eventually? If not, we should probably pick
a
> reasonable timeout instead of looping potentially forever.

Yes, the assumption is that Omaha has appropriate timeouts for the different
phases (check for update, download, install) and will transition to a failed
state when those are exceeded.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/app_host/binaries_installer.h (right):

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer.h:14: // be used, if any.
On 2012/08/23 23:51:01, miket wrote:
> It reduces to the same thing in either case, but to reduce ambiguity, can you
> move "if any" to directly follow the thing it's referring to? (probably AP
> value)

Done.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:27: LOG(ERROR)
<< "Failed to check app_bundle->isBusy: " << hr;
On 2012/08/23 23:51:01, miket wrote:
> No braces needed for these one-line blocks.

Done.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:35: // If the
app bundle is 'busy' we will just wait some more.
On 2012/08/23 23:51:01, miket wrote:
> Why quotes around busy? Does it mean something different from its usual
meaning?

Done.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:72: <<
completion_message_hr;
On 2012/08/23 23:51:01, miket wrote:
> one-liner braces

I don't think these qualify as one-liner according to the style-guide.
Certainly, I've almost made the mistake of adding a statement to a block like
this, thinking it must be in braces already.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:96: LOG(ERROR)
<< "Unexpected error: " << hr;
On 2012/08/23 23:51:01, miket wrote:
> Might as well give a little more context for this log statement, so that users
> don't report "I got Unexpected error 12345, fix my bug and BTW you suck" that
we
> can't easily trace back to the source.

Done.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:134: bool
CheckIfDone(IAppBundle* app_bundle, IApp* app, HRESULT* hr) {
On 2012/08/23 23:51:01, miket wrote:
> Suggestion: if you have a local hr that's copied to *hr at the bottom of the
> block only if *hr is non-null, then in your sleep-wait back in the other file,
> you can pass in null instead of receiving an ignored result (I'll have to look
> back to check, but I don't think you do anything with that result).

No, hr is returned to the caller of InstallBinaries, and app_host_main uses it
to tell if the install succeeded or failed (i.e., whether to try to locate the
binaries again or to just give up).

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:262: *app =
temp_app.Detach();
On 2012/08/23 23:51:01, miket wrote:
> This could slide, but I can see cases where temp_app leaks.

Are you suggesting that a potential code change could introduce such a thing?
Because as it stands now, we only Detach it immediately before returning.

I could do something like:

if (FAILED(hr)) {
  return hr;
} else {
  *app = temp_app.Detach();
  return S_OK;
}

Let me know if that's what you were thinking of.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:270: if
(FAILED(hr)) {
On 2012/08/23 23:51:01, miket wrote:
> It's slightly easier to read if the SUCCEEDED() case comes first.

Done.

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/app_host/binaries_installer_internal.h (right):

https://chromiumcodereview.appspot.com/10825364/diff/6001/chrome/browser/exte...
chrome/browser/extensions/app_host/binaries_installer_internal.h:26: HRESULT
GetAppHostAPValue(IGoogleUpdate3* update3, BSTR* ap_value);
On 2012/08/23 23:51:01, miket wrote:
> Be consistent in capitalization of AP in this file's function names.

Done.

[commit-bot: I haz the power, 2012-08-31 21:11:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/10825364/27001

[miket_OOO, 2012-08-31 21:48:43]

Great, thanks!

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_host/binaries_installer_internal.cc (right):

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:134: bool
CheckIfDone(IAppBundle* app_bundle, IApp* app, HRESULT* hr) {
> No, hr is returned to the caller of InstallBinaries, and app_host_main uses it
> to tell if the install succeeded or failed (i.e., whether to try to locate the
> binaries again or to just give up).

OK, good. I should have looked but I'd convinced myself it was ignored.

http://codereview.chromium.org/10825364/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/app_host/binaries_installer_internal.cc:262: *app =
temp_app.Detach();
> Are you suggesting that a potential code change could introduce such a thing?
> Because as it stands now, we only Detach it immediately before returning.

Ah, OK. I was confused. It belongs to the RAII guy, and in the cases I was
thinking of it remains his responsibility (which he takes care of just fine). I
retract my comment.

[commit-bot: I haz the power, 2012-08-31 22:18:18]

Try job failure for 10825364-27001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-01 18:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/10825364/17012

[commit-bot: I haz the power, 2012-09-01 22:49:57]

Change committed as 154627