Add chrome.webstore API methods to allow sites to see progress of installation

Add chrome.webstore API methods to allow sites to see progress of installation
Add two chrome.webstore API methods for progress updates:
  chrome.webstore.setInstallStageListener(function(string stage))
  - stage is either "downloading" or "installing"
  chrome.webstore.setDownloadProgressListener(function(int percent_downloaded))
Update InstallObserver to have methods for install stages.
Make WebstoreInstaller provide more frequent download updates (current consumers
like WebstoreResult imply this was intended from the start).
BUG=308634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259483

[Devlin, 2014-02-25 22:55:17]

Ben, mind taking a look?  (It's not quite as big and scary as it seems).

And sorry I keep sending you reviews - you just seem to have written the areas
I'm working in. :)

[felt, 2014-02-25 23:30:40]

drive-by comment

https://codereview.chromium.org/175263003/diff/260001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log.h (left):

https://codereview.chromium.org/175263003/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log.h:85: virtual void
OnExtensionInstalled(const Extension* extension) OVERRIDE {}
Why is this disappearing with no replacement? Will this event no longer be
fired? It seems like this is going to break the AL.

[Devlin, 2014-02-25 23:40:45]

https://codereview.chromium.org/175263003/diff/260001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log.h (left):

https://codereview.chromium.org/175263003/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log.h:85: virtual void
OnExtensionInstalled(const Extension* extension) OVERRIDE {}
On 2014/02/25 23:30:40, felt wrote:
> Why is this disappearing with no replacement? Will this event no longer be
> fired? It seems like this is going to break the AL.

Shouldn't break it, I hope :)  Just removed the pure-virtualness of the
functions in InstallObserver, since usually consumers only needed to override a
few of them.  If they're overridden and not empty, they're still there.

On that note, though, unless I'm missing something, AL shouldn't care whether or
not this particular event is fired, right?  (It doesn't do anything here...)

[felt, 2014-02-25 23:43:40]

https://codereview.chromium.org/175263003/diff/260001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log.h (left):

https://codereview.chromium.org/175263003/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log.h:85: virtual void
OnExtensionInstalled(const Extension* extension) OVERRIDE {}
Ah, I see.

Also, it looks like someone has made previous changes that moved the Install
logic into Load... so nevermind anyway.

On 2014/02/25 23:40:46, D Cronin wrote:
> On 2014/02/25 23:30:40, felt wrote:
> > Why is this disappearing with no replacement? Will this event no longer be
> > fired? It seems like this is going to break the AL.
> 
> Shouldn't break it, I hope :)  Just removed the pure-virtualness of the
> functions in InstallObserver, since usually consumers only needed to override
a
> few of them.  If they're overridden and not empty, they're still there.
> 
> On that note, though, unless I'm missing something, AL shouldn't care whether
or
> not this particular event is fired, right?  (It doesn't do anything here...)

[not at google - send to devlin, 2014-02-26 00:05:26]

Heyyy so mind making that pure-virtual change in a pre-patch?

Also: WDYT about having progress be a number from 0-1 rather than an int from
0-100?

[Devlin, 2014-02-26 22:43:05]

On 2014/02/26 00:05:26, kalman wrote:
> Heyyy so mind making that pure-virtual change in a pre-patch?
> 
> Also: WDYT about having progress be a number from 0-1 rather than an int from
> 0-100?

Removed pure-virtual changes (now they're here:
https://codereview.chromium.org/176883003).

Re progress 0 - 1, changed it on the JS side (makes a lot of sense to have it as
a fraction), but kept it as an integer on the C++ side for consistency (with
DownloadItem, OnDownloadProgress, etc).

[not at google - send to devlin, 2014-02-27 21:49:55]

I had a look at the API implementation and the comment about the events will
probably involve some change. Adding Antony here to look at the specifics of the
CRX installer and so o, he's more familiar than me with that stuff.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:121: ++iter;
is this why you're using a std::list rather than a std::vector? without looking
at the code in too much detail, but observing that it kinda looks like you're
associating extension IDs with data, could you use a std::map<id, data> instead?

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.h (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.h:60: virtual void
Shutdown() OVERRIDE;
these OVERRIDE methods can be private

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.h:98:
std::list<ObservedInstallInfo> install_stage_listeners_;
ObservedInstallInfoList?

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.cc:33: void
InstallObserver::OnExtensionUninstalled(const Extension* extension) {}
as discussed, if you can delete all of these and move the {} to the header file
that would be idiomatic IIRC.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.h (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:41: // be downloaded, so we don't
want to group the two together incorrectly.
nit: this comment has too many commas in it.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:51: // Called when the necessary
downloads have completed, and the crx
nit: unnecessary comma

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:178:
switches::kAppsGalleryInstallAutoConfirmForTests, "accept");
TODO(devlin): Add an ExtensionInstallPrompt::ScopedAutoConfirmForTest method.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:326: // TODO(crbug.com/305343):
Query manifest of dependencises before
dependencises?

https://codereview.chromium.org/175263003/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:51: "name":
"setInstallStageListener",
It's pretty weird to have these special methods. make them events to be
consistent with the rest of the platform. You'll need to construct the events
and call dispatch() or whatever the method is yourself; but that's easy enough.

https://codereview.chromium.org/175263003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:79: "description": "The percentage of
the download which has been completed, between 0 and 1."
"percentage" is misleading. "The progress of the download. 0.0 means it hasn't
been started, 1.0 means completed" or something.

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
File chrome/renderer/extensions/webstore_bindings.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:53: "The listener callback for
download progress must be a function.";
these sort of checks can be handled by the Event object when you use that

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:102: return render_view;
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:115: if (args[0]->IsBoolean()) {
so - *you* are calling the Install method. Rather than dealing with bad input
from yourself, just CHECK here and do validation in JS if necessary.

(yes this whole method is broken in that way; we should fix that, it's
unnecessarily complicated)

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
File chrome/renderer/extensions/webstore_bindings.h (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.h:33: content::RenderView*
GetRenderView();
this method already exists on ChromeV8Extension

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20:
this._downloadProgressListener != null,
you can also do !!this._installStageListener etc.

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:100: }
see comment in the JSON which applies here too. you should be able to create
"anonymous" events with "new Event()" and then fire events on them yourself. See
how the messaging stuff works.

[Devlin, 2014-02-28 18:04:50]

Thanks, Ben, that cleaned it up a lot. :)  Didn't know we could use the JS
events without all the boilerplate on the backend.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:121: ++iter;
On 2014/02/27 21:49:56, kalman wrote:
> is this why you're using a std::list rather than a std::vector? without
looking
> at the code in too much detail, but observing that it kinda looks like you're
> associating extension IDs with data, could you use a std::map<id, data>
instead?

Yeah, I was using a list for the removal speed.  I thought about a map but
there's a small chance that we could actually have multiple listeners for the
same extension id - we have to check based on extension and routing ids (which
means that an extension id isn't a unique key into the map)

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.h (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.h:60: virtual void
Shutdown() OVERRIDE;
On 2014/02/27 21:49:56, kalman wrote:
> these OVERRIDE methods can be private 

Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.h:98:
std::list<ObservedInstallInfo> install_stage_listeners_;
On 2014/02/27 21:49:56, kalman wrote:
> ObservedInstallInfoList?

Whoops, done.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.cc:33: void
InstallObserver::OnExtensionUninstalled(const Extension* extension) {}
On 2014/02/27 21:49:56, kalman wrote:
> as discussed, if you can delete all of these and move the {} to the header
file
> that would be idiomatic IIRC.

Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.h (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:41: // be downloaded, so we don't
want to group the two together incorrectly.
On 2014/02/27 21:49:56, kalman wrote:
> nit: this comment has too many commas in it.

Haha fair enough, I suppose, though, really, there could have been, maybe, a few
more?

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:51: // Called when the necessary
downloads have completed, and the crx
On 2014/02/27 21:49:56, kalman wrote:
> nit: unnecessary comma

I'm defending this one! :P
http://www.hamilton.edu/writing/seven-sins-of-writing/the-second-deadly-sin-i...
Rule (a).

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:178:
switches::kAppsGalleryInstallAutoConfirmForTests, "accept");
On 2014/02/27 21:49:56, kalman wrote:
> TODO(devlin): Add an ExtensionInstallPrompt::ScopedAutoConfirmForTest method.

Done at the source (chrome/common/chrome_switches.cc)

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:197:
RunTest("add_and_remove_listener.html");
These tests are now irrelevant (tested in the event bindings)

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:326: // TODO(crbug.com/305343):
Query manifest of dependencises before
On 2014/02/27 21:49:56, kalman wrote:
> dependencises?

Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:51: "name":
"setInstallStageListener",
On 2014/02/27 21:49:56, kalman wrote:
> It's pretty weird to have these special methods. make them events to be
> consistent with the rest of the platform. You'll need to construct the events
> and call dispatch() or whatever the method is yourself; but that's easy
enough.

Done!  Thanks for the help on this :)

https://codereview.chromium.org/175263003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:79: "description": "The percentage of
the download which has been completed, between 0 and 1."
On 2014/02/27 21:49:56, kalman wrote:
> "percentage" is misleading. "The progress of the download. 0.0 means it hasn't
> been started, 1.0 means completed" or something.

Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
File chrome/renderer/extensions/webstore_bindings.cc (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:53: "The listener callback for
download progress must be a function.";
On 2014/02/27 21:49:56, kalman wrote:
> these sort of checks can be handled by the Event object when you use that

Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:102: return render_view;
On 2014/02/27 21:49:56, kalman wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:115: if (args[0]->IsBoolean()) {
On 2014/02/27 21:49:56, kalman wrote:
> so - *you* are calling the Install method. Rather than dealing with bad input
> from yourself, just CHECK here and do validation in JS if necessary.
> 
> (yes this whole method is broken in that way; we should fix that, it's
> unnecessarily complicated)

Done!

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
File chrome/renderer/extensions/webstore_bindings.h (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.h:33: content::RenderView*
GetRenderView();
On 2014/02/27 21:49:56, kalman wrote:
> this method already exists on ChromeV8Extension

Oh.  Whoops.  Done.

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20:
this._downloadProgressListener != null,
On 2014/02/27 21:49:56, kalman wrote:
> you can also do !!this._installStageListener etc.

(replaced with event.hasListeners())

https://codereview.chromium.org/175263003/diff/320001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:100: }
On 2014/02/27 21:49:56, kalman wrote:
> see comment in the JSON which applies here too. you should be able to create
> "anonymous" events with "new Event()" and then fire events on them yourself.
See
> how the messaging stuff works.

Done.

[not at google - send to devlin, 2014-03-01 01:42:06]

Thanks, that looks much better!

I still only looked at the renderer and part of the API implementation. That
part lgtm with comments.

Antony, if you won't have a chance to look at the installation stuff let me know
- but for now I'm leaving it to you as the owner of that code :)

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:83: iter->routing_id,
percent_downloaded));
you could actually save yourself the trouble of adding these new extension
messages, and the boilerplate handling on the renderer, and do:

ListValue args;
args.AppendDouble(percent_downloaded / 100.0);
iter->ipc_sender->Send(new ExtensionMsg_MessageInvoke(
    iter->rounting_id, "", "webstore", "onDownloadProgress", args, false));

same for the other message.

Reference:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/extensi...
File chrome/renderer/extensions/webstore_bindings.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:84: WebFrame* frame =
WebFrame::frameForContext(context()->v8_context());
you can use context()->web_frame().

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20: throw 'A
Chrome Web Store installation is already pending.';
throw new Error(...)

same for all the rest.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:30:
this.onDownloadProgress.hasListeners(),
make sure you document that listeners need to be added before install() is
called.

[Devlin, 2014-03-03 17:44:59]

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:83: iter->routing_id,
percent_downloaded));
On 2014/03/01 01:42:07, kalman wrote:
> you could actually save yourself the trouble of adding these new extension
> messages, and the boilerplate handling on the renderer, and do:
> 
> ListValue args;
> args.AppendDouble(percent_downloaded / 100.0);
> iter->ipc_sender->Send(new ExtensionMsg_MessageInvoke(
>     iter->rounting_id, "", "webstore", "onDownloadProgress", args, false));
> 
> same for the other message.
> 
> Reference:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

Yes and no.

We can't just do ExtensionMsg_MessageInvoke because that's only handled by the
Dispatcher and ExtensionHelper (which forwards it to the Dispatcher).  From the
Dispatcher, it's sent off to every tab, so if multiple tabs had listeners, we'd
erroneously send the message to them, too (versus now, where we only send the
message to the tab that initiated the installation).

We could, though, choose to handle ExtensionMsg_MessageInvoke within
WebstoreBindings, since in both of the calls, all we do is forward the message.
This lets us only implement one new handler in WebstoreBindings and not add any
new ExtensionMessages.  But it has the disadvantage of then _allowing_ people to
send MessageInvokes to Bindings mistakenly, and we won't get an IPC Unhandled
error (in Dispatcher, we don't worry because we basically handle any invoke
correctly).

Preferences? (and sorry if what you meant from the start was to use
MessageInvoke and handle it explicitly in WebstoreBindings...just wanna make
sure)

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/extensi...
File chrome/renderer/extensions/webstore_bindings.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/extensi...
chrome/renderer/extensions/webstore_bindings.cc:84: WebFrame* frame =
WebFrame::frameForContext(context()->v8_context());
On 2014/03/01 01:42:07, kalman wrote:
> you can use context()->web_frame().

Done.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20: throw 'A
Chrome Web Store installation is already pending.';
On 2014/03/01 01:42:07, kalman wrote:
> throw new Error(...)
> 
> same for all the rest.

Done.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:30:
this.onDownloadProgress.hasListeners(),
On 2014/03/01 01:42:07, kalman wrote:
> make sure you document that listeners need to be added before install() is
> called.

Doc'd here in a comment and in the Event description in the .json.

[asargent_no_longer_on_chrome, 2014-03-03 18:49:08]

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:39:
Profile::FromBrowserContext(browser_context))->AddObserver(this);
Should you be removing yourself as an observer in the destructor?

(Perhaps you could take advantage of base/scoped_observer.h)

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:105: const std::string&
install_stage) {
Looks like this only gets called with const char*, would it make sense to change
the parameter type to that? (Not sure off the top of my head if the compiler can
already arrange to avoid constructing 2 std::string's the way you have it -
maybe ask jyasskin who I'm sure could tell you)

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.h (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.h:71: };
See if you can move this inner struct into the .cc file per:

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....

Alternatively, you could make it a top-level struct instead of an inner one,
forward declare it above in this file, and still push the definition into the
.cc file.

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.h (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:66: virtual void
OnAppInstalledToAppList(const std::string& extension_id) {}
Hmm, some of these stretch the definition of "install". I realize this problem
was present before your changes, but I wonder if we should change the name of
this from InstallObserver to LifeCycleObserver or something like that.

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:629: percent = (percent +
finished_modules * 100) / total_modules_;
nit: add extra () to make intended order of arithmetic operations super explicit
for those who haven't memorized the precedence order of + vs *.

https://codereview.chromium.org/175263003/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:28: ]
For future proofing if we ever decided to allow multiple concurrent
installations, would it make sense to have these events also communicate the
installId that gets generated inside the implementation of webstore.install()?
That also might mean we'd want to return that id from install(), which we don't
currently.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:15: new
Event(null, [{name: 'progress', type: 'number'}], {unmanaged: true});
We'll want to update the documentation for this at 

https://developers.google.com/chrome/web-store/docs/inline_installation

which doesn't seem to use autogeneration based on the webstore.json file, and
for instance doesn't mention the already existing url and onSuccess/onFailure
arguments. 


BTW, did you already think about the API alternative of having these be passed
in as arguments to the install function like the onSuccess/onFailure callbacks?
I don't know if that would be better, but I guess there is some asymmetry in
doing it this way.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20: throw 'A
Chrome Web Store installation is already pending.';
If developers find themselves in a situation where there is a pending but
seemingly stuck install (eg no download progress for a while), would it make
sense to try and provide a cancel function? 

I guess the best they could do today is to either automatically reload the page,
or ask the user to do it manually?

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:85: // Called
by webstore_binding.cc.
nit: I assume you meant webstore_bindings.cc ?

[Devlin, 2014-03-03 21:15:12]

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:39:
Profile::FromBrowserContext(browser_context))->AddObserver(this);
On 2014/03/03 18:49:08, Antony Sargent wrote:
> Should you be removing yourself as an observer in the destructor?
> 
> (Perhaps you could take advantage of base/scoped_observer.h)
> 

Done, but see related comment.

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:83: iter->routing_id,
percent_downloaded));
On 2014/03/03 17:44:59, D Cronin wrote:
> On 2014/03/01 01:42:07, kalman wrote:
> > you could actually save yourself the trouble of adding these new extension
> > messages, and the boilerplate handling on the renderer, and do:
> > 
> > ListValue args;
> > args.AppendDouble(percent_downloaded / 100.0);
> > iter->ipc_sender->Send(new ExtensionMsg_MessageInvoke(
> >     iter->rounting_id, "", "webstore", "onDownloadProgress", args, false));
> > 
> > same for the other message.
> > 
> > Reference:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> Yes and no.
> 
> We can't just do ExtensionMsg_MessageInvoke because that's only handled by the
> Dispatcher and ExtensionHelper (which forwards it to the Dispatcher).  From
the
> Dispatcher, it's sent off to every tab, so if multiple tabs had listeners,
we'd
> erroneously send the message to them, too (versus now, where we only send the
> message to the tab that initiated the installation).
> 
> We could, though, choose to handle ExtensionMsg_MessageInvoke within
> WebstoreBindings, since in both of the calls, all we do is forward the
message.
> This lets us only implement one new handler in WebstoreBindings and not add
any
> new ExtensionMessages.  But it has the disadvantage of then _allowing_ people
to
> send MessageInvokes to Bindings mistakenly, and we won't get an IPC Unhandled
> error (in Dispatcher, we don't worry because we basically handle any invoke
> correctly).
> 
> Preferences? (and sorry if what you meant from the start was to use
> MessageInvoke and handle it explicitly in WebstoreBindings...just wanna make
> sure)

(Talked about this offline and decided that going with the current approach was
best. Using MessageInvoke resulted in more complexity than it saved.)

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:105: const std::string&
install_stage) {
On 2014/03/03 18:49:08, Antony Sargent wrote:
> Looks like this only gets called with const char*, would it make sense to
change
> the parameter type to that? (Not sure off the top of my head if the compiler
can
> already arrange to avoid constructing 2 std::string's the way you have it -
> maybe ask jyasskin who I'm sure could tell you)

Done.

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.h (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.h:71: };
On 2014/03/03 18:49:08, Antony Sargent wrote:
> See if you can move this inner struct into the .cc file per:
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....
> 
> Alternatively, you could make it a top-level struct instead of an inner one,
> forward declare it above in this file, and still push the definition into the
> .cc file. 

Cool; for some reason, I thought that you couldn't have containers of undefined
classes/structs.  Thanks. :)

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.h (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:66: virtual void
OnAppInstalledToAppList(const std::string& extension_id) {}
On 2014/03/03 18:49:08, Antony Sargent wrote:
> Hmm, some of these stretch the definition of "install". I realize this problem
> was present before your changes, but I wonder if we should change the name of
> this from InstallObserver to LifeCycleObserver or something like that. 

Good point.  If it's alright, I'll do that change separately right after this
one?  (If we change InstallObserver, we should also change InstallTracker, and
that would mean touching even more files).

If you like, though, I can bundle them in here.

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:629: percent = (percent +
finished_modules * 100) / total_modules_;
On 2014/03/03 18:49:08, Antony Sargent wrote:
> nit: add extra () to make intended order of arithmetic operations super
explicit
> for those who haven't memorized the precedence order of + vs *. 

Done.

https://codereview.chromium.org/175263003/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:28: ]
On 2014/03/03 18:49:08, Antony Sargent wrote:
> For future proofing if we ever decided to allow multiple concurrent
> installations, would it make sense to have these events also communicate the
> installId that gets generated inside the implementation of webstore.install()?
> That also might mean we'd want to return that id from install(), which we
don't
> currently. 
> 
> 

I was wondering about that as well.  If we ever do allow multiple installs, it
makes sense, but otherwise it just seems like exposing implementation that
should be exposed - so I don't know.  Ben (as author of the js bindings),
thoughts?

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:15: new
Event(null, [{name: 'progress', type: 'number'}], {unmanaged: true});
On 2014/03/03 18:49:08, Antony Sargent wrote:
> We'll want to update the documentation for this at 
> 
> https://developers.google.com/chrome/web-store/docs/inline_installation
> 
> which doesn't seem to use autogeneration based on the webstore.json file, and
> for instance doesn't mention the already existing url and onSuccess/onFailure
> arguments. 
> 
> 
> BTW, did you already think about the API alternative of having these be passed
> in as arguments to the install function like the onSuccess/onFailure
callbacks?
> I don't know if that would be better, but I guess there is some asymmetry in
> doing it this way. 

We discussed it, but we thought that would be too many methods shoved in
(especially when so many are optional, and you might want a subset of them), and
that would also prevent us from using the Event structure (which Ben has been a
proponent of using).

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20: throw 'A
Chrome Web Store installation is already pending.';
On 2014/03/03 18:49:08, Antony Sargent wrote:
> If developers find themselves in a situation where there is a pending but
> seemingly stuck install (eg no download progress for a while), would it make
> sense to try and provide a cancel function? 
> 
> I guess the best they could do today is to either automatically reload the
page,
> or ask the user to do it manually?

That's a good idea, but I think it's a separate issue (probably with its own API
doc and all), if that's okay.

That being said, Ben, initial thoughts?

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:85: // Called
by webstore_binding.cc.
On 2014/03/03 18:49:08, Antony Sargent wrote:
> nit: I assume you meant webstore_bindings.cc ?

Whoops, done.

https://codereview.chromium.org/175263003/diff/440001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:106:
install_observer_.Remove(InstallTrackerFactory::GetForProfile(
Is it okay to fetch a BCKS while it's being Shutdown() in order to remove an
observer?  Or do we need to keep track of the tracker while we observe it?

[asargent_no_longer_on_chrome, 2014-03-03 22:25:33]

lgtm

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
File chrome/browser/extensions/install_observer.h (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/install_observer.h:66: virtual void
OnAppInstalledToAppList(const std::string& extension_id) {}
On 2014/03/03 21:15:13, D Cronin wrote:
> On 2014/03/03 18:49:08, Antony Sargent wrote:
> > Hmm, some of these stretch the definition of "install". I realize this
problem
> > was present before your changes, but I wonder if we should change the name
of
> > this from InstallObserver to LifeCycleObserver or something like that. 
> 
> Good point.  If it's alright, I'll do that change separately right after this
> one?  (If we change InstallObserver, we should also change InstallTracker, and
> that would mean touching even more files).
> 
> If you like, though, I can bundle them in here.

Separate CL would be fine.

https://codereview.chromium.org/175263003/diff/340001/chrome/common/extension...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:28: ]
On 2014/03/03 21:15:13, D Cronin wrote:
> On 2014/03/03 18:49:08, Antony Sargent wrote:
> > For future proofing if we ever decided to allow multiple concurrent
> > installations, would it make sense to have these events also communicate the
> > installId that gets generated inside the implementation of
webstore.install()?
> > That also might mean we'd want to return that id from install(), which we
> don't
> > currently. 
> > 
> > 
> 
> I was wondering about that as well.  If we ever do allow multiple installs, it
> makes sense, but otherwise it just seems like exposing implementation that
> should be exposed - so I don't know.  Ben (as author of the js bindings),
> thoughts?

Yeah, it would probably be ok to leave this for a follow-up.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:15: new
Event(null, [{name: 'progress', type: 'number'}], {unmanaged: true});
On 2014/03/03 21:15:13, D Cronin wrote:
> On 2014/03/03 18:49:08, Antony Sargent wrote:
> > We'll want to update the documentation for this at 
> > 
> > https://developers.google.com/chrome/web-store/docs/inline_installation
> > 
> > which doesn't seem to use autogeneration based on the webstore.json file,
and
> > for instance doesn't mention the already existing url and
onSuccess/onFailure
> > arguments. 
> > 
> > 
> > BTW, did you already think about the API alternative of having these be
passed
> > in as arguments to the install function like the onSuccess/onFailure
> callbacks?
> > I don't know if that would be better, but I guess there is some asymmetry in
> > doing it this way. 
> 
> We discussed it, but we thought that would be too many methods shoved in
> (especially when so many are optional, and you might want a subset of them),
and
> that would also prevent us from using the Event structure (which Ben has been
a
> proponent of using).

Ok, sgtm.

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20: throw 'A
Chrome Web Store installation is already pending.';
On 2014/03/03 21:15:13, D Cronin wrote:
> On 2014/03/03 18:49:08, Antony Sargent wrote:
> > If developers find themselves in a situation where there is a pending but
> > seemingly stuck install (eg no download progress for a while), would it make
> > sense to try and provide a cancel function? 
> > 
> > I guess the best they could do today is to either automatically reload the
> page,
> > or ask the user to do it manually?
> 
> That's a good idea, but I think it's a separate issue (probably with its own
API
> doc and all), if that's okay.
> 
> That being said, Ben, initial thoughts?

Maybe just log a bug for follow-up and we can have a discussion there.

https://codereview.chromium.org/175263003/diff/440001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:106:
install_observer_.Remove(InstallTrackerFactory::GetForProfile(
On 2014/03/03 21:15:13, D Cronin wrote:
> Is it okay to fetch a BCKS while it's being Shutdown() in order to remove an
> observer?  Or do we need to keep track of the tracker while we observe it?

I think so. From
components/browser_context_keyed_service/browser_context_keyed_service.h:

"
// Many services that hang off BrowserContext have a two-pass shutdown. Many
// subsystems need a first pass shutdown phase where they drop references. Not
// all services will need this, so there's a default implementation. Only once
// every system has been given a chance to drop references do we start deleting
// objects.

"


BTW, another approach you might consider: just hold the install_observer_ inside
a scoped_ptr, and here instead of explicitly calling Remove with a Profile* that
you look up, just clear the scoped_ptr causing ~ScopedObserver() to run and
clean these up automatically.

[Devlin, 2014-03-03 22:49:15]

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/175263003/diff/340001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:20: throw 'A
Chrome Web Store installation is already pending.';
On 2014/03/03 22:25:33, Antony Sargent wrote:
> On 2014/03/03 21:15:13, D Cronin wrote:
> > On 2014/03/03 18:49:08, Antony Sargent wrote:
> > > If developers find themselves in a situation where there is a pending but
> > > seemingly stuck install (eg no download progress for a while), would it
make
> > > sense to try and provide a cancel function? 
> > > 
> > > I guess the best they could do today is to either automatically reload the
> > page,
> > > or ask the user to do it manually?
> > 
> > That's a good idea, but I think it's a separate issue (probably with its own
> API
> > doc and all), if that's okay.
> > 
> > That being said, Ben, initial thoughts?
> 
> Maybe just log a bug for follow-up and we can have a discussion there. 
> 
> 

Done. crbug.com/348752

https://codereview.chromium.org/175263003/diff/440001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore/webstore_api.cc (right):

https://codereview.chromium.org/175263003/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore/webstore_api.cc:106:
install_observer_.Remove(InstallTrackerFactory::GetForProfile(
On 2014/03/03 22:25:33, Antony Sargent wrote:
> On 2014/03/03 21:15:13, D Cronin wrote:
> > Is it okay to fetch a BCKS while it's being Shutdown() in order to remove an
> > observer?  Or do we need to keep track of the tracker while we observe it?
> 
> I think so. From
> components/browser_context_keyed_service/browser_context_keyed_service.h:
> 
> "
> // Many services that hang off BrowserContext have a two-pass shutdown. Many
> // subsystems need a first pass shutdown phase where they drop references. Not
> // all services will need this, so there's a default implementation. Only once
> // every system has been given a chance to drop references do we start
deleting
> // objects.
> 
> "
> 
> 
> BTW, another approach you might consider: just hold the install_observer_
inside
> a scoped_ptr, and here instead of explicitly calling Remove with a Profile*
that
> you look up, just clear the scoped_ptr causing ~ScopedObserver() to run and
> clean these up automatically. 

Yep, that was my contingency plan :)  And actually, I think it does make it
almost look cleaner - I just wish it would get rid of the include in the .h. 
But, at least it saves a lookup for the BCKS.

[Devlin, 2014-03-03 22:50:33]

+jln: chrome/common/extensions/extension_messages.h
+davemoore: chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

[Devlin, 2014-03-07 16:28:46]

friendly ping to jln@ and davemoore@.  The Play team is eagerly awaiting these
API methods, so we'd like to get them ready as soon as we can :)

[Devlin, 2014-03-10 21:24:18]

+jschuh@ for extension_messages
+erg@ for chrome_browser_browser_main_extra_parts_profiles

(Assuming that davemoore@ and jln@ are review-swamped, given the radio silence)

[Elliot Glaysher, 2014-03-10 21:34:24]

trivial profiles change lgtm

[jln (very slow on Chromium), 2014-03-10 23:35:40]

Sorry this slept through.

https://chromiumcodereview.appspot.com/175263003/diff/500001/chrome/browser/e...
File chrome/browser/extensions/tab_helper.cc (right):

https://chromiumcodereview.appspot.com/175263003/diff/500001/chrome/browser/e...
chrome/browser/extensions/tab_helper.cc:572: #if defined(ENABLE_EXTENSIONS)
Could you check that listeners_mask is something that you expect?

If I understood the code correctly, you only need to check that

0 == listeners_mask & (api::webstore::INSTALL_STAGE_LISTENER |
api::webstore::DOWNLOAD_PROGRESS_LISTENER)

We should not defer what amounts to a type-check to this far away from the IPC
handling code.

https://chromiumcodereview.appspot.com/175263003/diff/500001/chrome/common/ex...
File chrome/common/extensions/extension_messages.h (right):

https://chromiumcodereview.appspot.com/175263003/diff/500001/chrome/common/ex...
chrome/common/extensions/extension_messages.h:614: std::string /* stage */)
Could you make this an enum instad of a string?

We strongly discourage sending strings over IPCs:
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

[Devlin, 2014-03-11 17:22:06]

https://codereview.chromium.org/175263003/diff/500001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/175263003/diff/500001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:572: #if defined(ENABLE_EXTENSIONS)
On 2014/03/10 23:35:41, jln (OOO-til-March-17) wrote:
> Could you check that listeners_mask is something that you expect?
> 
> If I understood the code correctly, you only need to check that
> 
> 0 == listeners_mask & (api::webstore::INSTALL_STAGE_LISTENER |
> api::webstore::DOWNLOAD_PROGRESS_LISTENER)
> 
> We should not defer what amounts to a type-check to this far away from the IPC
> handling code.

Checked.  But (unless my bitwise-logic is wrong, which is totally possible), I
think it would have to be

0 == listeners_mask & ~(INSTALL_STAGE_LISTENER | DOWNLOAD_PROGRESS_LISTENER)

since we could have both (this will check that there are no other bits checked,
other than one or both of those).

https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
chrome/common/extensions/extension_messages.h:614: std::string /* stage */)
On 2014/03/10 23:35:41, jln (OOO-til-March-17) wrote:
> Could you make this an enum instad of a string?
> 
> We strongly discourage sending strings over IPCs:
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

Done, and thanks for the tip! :)

[Devlin, 2014-03-18 20:20:08]

On 2014/03/11 17:22:06, D Cronin wrote:
>
https://codereview.chromium.org/175263003/diff/500001/chrome/browser/extensio...
> File chrome/browser/extensions/tab_helper.cc (right):
> 
>
https://codereview.chromium.org/175263003/diff/500001/chrome/browser/extensio...
> chrome/browser/extensions/tab_helper.cc:572: #if defined(ENABLE_EXTENSIONS)
> On 2014/03/10 23:35:41, jln (OOO-til-March-17) wrote:
> > Could you check that listeners_mask is something that you expect?
> > 
> > If I understood the code correctly, you only need to check that
> > 
> > 0 == listeners_mask & (api::webstore::INSTALL_STAGE_LISTENER |
> > api::webstore::DOWNLOAD_PROGRESS_LISTENER)
> > 
> > We should not defer what amounts to a type-check to this far away from the
IPC
> > handling code.
> 
> Checked.  But (unless my bitwise-logic is wrong, which is totally possible), I
> think it would have to be
> 
> 0 == listeners_mask & ~(INSTALL_STAGE_LISTENER | DOWNLOAD_PROGRESS_LISTENER)
> 
> since we could have both (this will check that there are no other bits
checked,
> other than one or both of those).
> 
>
https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
> File chrome/common/extensions/extension_messages.h (right):
> 
>
https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
> chrome/common/extensions/extension_messages.h:614: std::string /* stage */)
> On 2014/03/10 23:35:41, jln (OOO-til-March-17) wrote:
> > Could you make this an enum instad of a string?
> > 
> > We strongly discourage sending strings over IPCs:
> >
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....
> 
> Done, and thanks for the tip! :)

jln: friendly ping?

[jln (very slow on Chromium), 2014-03-20 00:05:39]

https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
chrome/common/extensions/extension_messages.h:614: std::string /* stage */)
Sorry, I should have been more clear. You need to serialize the enum properly,
please use IPC_ENUM_TRAITS_MAX_VALUE or IPC_ENUM_TRAITS_VALIDATE.

https://codereview.chromium.org/175263003/diff/530001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/175263003/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:576:
api::webstore::DOWNLOAD_PROGRESS_LISTENER));
This is required for security reasons. So we cannot have a check that is only
built in debug mode.

CHECK() is also frowned upon, because it allows trivial DoS. So instead you need
to ignore the message in this case. You can add NOTREACHED() in the condition if
you want to.

[Devlin, 2014-03-20 16:54:36]

Comments addressed.  Thanks for the attention, Julien - I'll try to make the
next patch less bumpy for security. :)

Apologies for the noise in the review - chrome/common/extension_messages.h moved
to extensions/common/messages.h.  jamescook@ and I agreed that InlineInstall is
a very Chrome-y aspect, and should remain in
chrome/common/extensions/chrome_extension_messages.h, so the two new messages
(and the old inline install messages) are there, instead of in
extensions/common.

https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
chrome/common/extensions/extension_messages.h:614: std::string /* stage */)
On 2014/03/20 00:05:40, jln wrote:
> Sorry, I should have been more clear. You need to serialize the enum properly,
> please use IPC_ENUM_TRAITS_MAX_VALUE or IPC_ENUM_TRAITS_VALIDATE.

Ah, I see.  Like this?

https://codereview.chromium.org/175263003/diff/530001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/175263003/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:576:
api::webstore::DOWNLOAD_PROGRESS_LISTENER));
On 2014/03/20 00:05:40, jln wrote:
> This is required for security reasons. So we cannot have a check that is only
> built in debug mode.
> 
> CHECK() is also frowned upon, because it allows trivial DoS. So instead you
need
> to ignore the message in this case. You can add NOTREACHED() in the condition
if
> you want to.

Amended.

[Devlin, 2014-03-24 21:03:08]

On 2014/03/20 16:54:36, D Cronin wrote:
> Comments addressed.  Thanks for the attention, Julien - I'll try to make the
> next patch less bumpy for security. :)
> 
> Apologies for the noise in the review - chrome/common/extension_messages.h
moved
> to extensions/common/messages.h.  jamescook@ and I agreed that InlineInstall
is
> a very Chrome-y aspect, and should remain in
> chrome/common/extensions/chrome_extension_messages.h, so the two new messages
> (and the old inline install messages) are there, instead of in
> extensions/common.
> 
>
https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
> File chrome/common/extensions/extension_messages.h (right):
> 
>
https://codereview.chromium.org/175263003/diff/500001/chrome/common/extension...
> chrome/common/extensions/extension_messages.h:614: std::string /* stage */)
> On 2014/03/20 00:05:40, jln wrote:
> > Sorry, I should have been more clear. You need to serialize the enum
properly,
> > please use IPC_ENUM_TRAITS_MAX_VALUE or IPC_ENUM_TRAITS_VALIDATE.
> 
> Ah, I see.  Like this?
> 
>
https://codereview.chromium.org/175263003/diff/530001/chrome/browser/extensio...
> File chrome/browser/extensions/tab_helper.cc (right):
> 
>
https://codereview.chromium.org/175263003/diff/530001/chrome/browser/extensio...
> chrome/browser/extensions/tab_helper.cc:576:
> api::webstore::DOWNLOAD_PROGRESS_LISTENER));
> On 2014/03/20 00:05:40, jln wrote:
> > This is required for security reasons. So we cannot have a check that is
only
> > built in debug mode.
> > 
> > CHECK() is also frowned upon, because it allows trivial DoS. So instead you
> need
> > to ignore the message in this case. You can add NOTREACHED() in the
condition
> if
> > you want to.
> 
> Amended.

jln: friendly ping?

[jln (very slow on Chromium), 2014-03-25 02:17:59]

chrome/common/extensions/chrome_extension_messages.h lgtm for the new messages.

However some of the old messages which are being moved are sending strings. It's
especially problematic in the renderer -> browser direction. Could this be
changed?

https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
File chrome/common/extensions/chrome_extension_messages.h (right):

https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:45: std::string /* Web
Store item ID */,
Is there any way we could type this properly rather than use a string?

https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:64: std::string /* error
*/)
Any way this could be an error message index instead of a string?
(Non-blocking since old)

[Devlin, 2014-03-25 17:31:58]

Julien, if it's alright with you, I'd like to handle your last two comments in a
separate CL, since both the IPC messages are old?  (I see you realized this
about InlineWebstoreInstallResponse, but want to confirm that it's okay for
InlineWebstoreInstall as well.)  If that's alright, I make a bug and assign it
to myself, and try to get a CL up soon.

https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
File chrome/common/extensions/chrome_extension_messages.h (right):

https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:45: std::string /* Web
Store item ID */,
On 2014/03/25 02:18:00, jln wrote:
> Is there any way we could type this properly rather than use a string?

Probably not. "Web Store item ID" is a fancy way of saying "extension id", which
is just a unique string.  But we can do some checking on it (e.g., IsIdValid(),
etc) that should hopefully help.

https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:64: std::string /* error
*/)
On 2014/03/25 02:18:00, jln wrote:
> Any way this could be an error message index instead of a string?
> (Non-blocking since old)

Not sure I follow on "error message index"?

[jln (very slow on Chromium), 2014-03-25 20:11:26]

On 2014/03/25 17:31:58, D Cronin wrote:
> Julien, if it's alright with you, I'd like to handle your last two comments in
a
> separate CL, since both the IPC messages are old?  (I see you realized this
> about InlineWebstoreInstallResponse, but want to confirm that it's okay for
> InlineWebstoreInstall as well.)  If that's alright, I make a bug and assign it
> to myself, and try to get a CL up soon.

Yes, looks good, thanks!

>
https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
> File chrome/common/extensions/chrome_extension_messages.h (right):
> 
>
https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
> chrome/common/extensions/chrome_extension_messages.h:45: std::string /* Web
> Store item ID */,
> On 2014/03/25 02:18:00, jln wrote:
> > Is there any way we could type this properly rather than use a string?
> 
> Probably not. "Web Store item ID" is a fancy way of saying "extension id",
which
> is just a unique string.  But we can do some checking on it (e.g.,
IsIdValid(),
> etc) that should hopefully help.

Yeah, IsIdValid() would be great!

>
https://codereview.chromium.org/175263003/diff/570001/chrome/common/extension...
> chrome/common/extensions/chrome_extension_messages.h:64: std::string /* error
> */)
> On 2014/03/25 02:18:00, jln wrote:
> > Any way this could be an error message index instead of a string?
> > (Non-blocking since old)
> 
> Not sure I follow on "error message index"?

Is there a way that the error messages could be stored in a (std::map for
instance) and that only a key would be sent over IPC? We don't like allowing
strings over IPC
(http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip...),
because they give too much control to an attacker over the recipient's address
space, and also because they tend to be less safe than using more constrained
types.

[Devlin, 2014-03-25 23:30:07]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-03-25 23:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/175263003/...

[commit-bot: I haz the power, 2014-03-26 04:36:34]

Change committed as 259483