[Extensions] Rework inline installation observation

[Extensions] Rework inline installation observation

Instead of observing through the WebstoreAPI, observe directly in the TabHelper.
This is a great deal less code, more direct, and also fixes a lifetime issue
with the TabHelper being deleted before the inline installation completes.

BUG=613949
Committed: https://crrev.com/d30a8bd191f17b61938fc87890bffc80049b0774
Cr-Commit-Position: refs/heads/master@{#403188}

[Devlin, 2016-06-28 23:36:44]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-06-28 23:36:52]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-06-29 00:49:22]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Devlin, 2016-06-29 00:49:23]

Istiaque, mind taking a look?

[lazyboy, 2016-06-29 21:03:04]

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

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:94: DCHECK(observe_download_progress ||
observe_install_stage);
DCHECK(tab_helper_)

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:132: TabHelper* tab_helper_;
Mark this and last two bool members const.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:447: if
(pending_inline_installations_.count(webstore_item_id) != 0) {
This seems like a new behavior? Test would be helpful :)

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:452: "An install is already in
progress",
This needs to be in a constant, pull out the constant from
webstore_standalone_installer.cc/kInstallInProgressError to
webstore_constants.cc

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:470: extension_reenabler_ =
ExtensionReenabler::PromptForReenable(
It's not obvious to me how this will end up clearing webstore_item_id from
pending_inline_installations_.

nvm, I see it will eventually call OnInlineInstallComplete

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:481: // TODO(devlin): We should adddress
the case of the extension already
Doesn't the beginning of the function prevent this (extension already being
installed) now?

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:488:
DCHECK(install_observers_.find(webstore_item_id) ==
can use install_observers_.count(webstore_item_id) == 0

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:625: // extension id; delete it last.
|extension_id|?

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:277: std::map<std::string,
std::unique_ptr<InlineInstallObserver>>
Add comment describing this member. or maybe use ExtensionId instead of
std::string to self describe.

[Devlin, 2016-06-29 22:32:09]

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

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:94: DCHECK(observe_download_progress ||
observe_install_stage);
On 2016/06/29 21:03:03, lazyboy wrote:
> DCHECK(tab_helper_)

Sure, can't hurt.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:132: TabHelper* tab_helper_;
On 2016/06/29 21:03:04, lazyboy wrote:
> Mark this and last two bool members const.

Done (assuming you mean TabHelper* const tab_helper_, and not const TabHelper*
tab_helper_ - Send() is not const so the latter is impossible).

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:447: if
(pending_inline_installations_.count(webstore_item_id) != 0) {
On 2016/06/29 21:03:03, lazyboy wrote:
> This seems like a new behavior? Test would be helpful :)

D'oh, I knew you would say that! ;)

So, looking into this, it's actually rather painful.
- We have a check renderer side as well that prevents duplicate installs early.
- But that won't catch all cases - e.g., if two separate JS contexts call it.
- They emit separate error messages. :(

I added a test for the most common case - two calls from the same JS context -
but I'm not sure it's worth it to manufacture a test for the other.  Let me know
what you think.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:452: "An install is already in
progress",
On 2016/06/29 21:03:03, lazyboy wrote:
> This needs to be in a constant, pull out the constant from
> webstore_standalone_installer.cc/kInstallInProgressError to
> webstore_constants.cc

Done.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:470: extension_reenabler_ =
ExtensionReenabler::PromptForReenable(
On 2016/06/29 21:03:03, lazyboy wrote:
> It's not obvious to me how this will end up clearing webstore_item_id from
> pending_inline_installations_.
> 
> nvm, I see it will eventually call OnInlineInstallComplete

Acknowledged.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:481: // TODO(devlin): We should adddress
the case of the extension already
On 2016/06/29 21:03:04, lazyboy wrote:
> Doesn't the beginning of the function prevent this (extension already being
> installed) now?

Nope.  The beginning prevents two inline installations at the same time, but if
the item is already installed and enabled, it doesn't.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:488:
DCHECK(install_observers_.find(webstore_item_id) ==
On 2016/06/29 21:03:03, lazyboy wrote:
> can use install_observers_.count(webstore_item_id) == 0

Done.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:625: // extension id; delete it last.
On 2016/06/29 21:03:03, lazyboy wrote:
> |extension_id|?

Done.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:277: std::map<std::string,
std::unique_ptr<InlineInstallObserver>>
On 2016/06/29 21:03:04, lazyboy wrote:
> Add comment describing this member. or maybe use ExtensionId instead of
> std::string to self describe. 

Done. Opted against ExtensionId because a) it would involve #including the
heavy-weight extension.h file* and b) it's inconsistent with the rest of this
file.

* We should probably pull it out into extension_id.h
** We should probably fix that, too, but out of scope for this change.

[lazyboy, 2016-06-29 23:10:28]

lgtm

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

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:132: TabHelper* tab_helper_;
On 2016/06/29 22:32:09, Devlin wrote:
> On 2016/06/29 21:03:04, lazyboy wrote:
> > Mark this and last two bool members const.
> 
> Done (assuming you mean TabHelper* const tab_helper_
Yes that's what I meant.
, and not const TabHelper*
> tab_helper_ - Send() is not const so the latter is impossible).

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:447: if
(pending_inline_installations_.count(webstore_item_id) != 0) {
On 2016/06/29 22:32:09, Devlin wrote:
> On 2016/06/29 21:03:03, lazyboy wrote:
> > This seems like a new behavior? Test would be helpful :)
> 
> D'oh, I knew you would say that! ;)
> 
> So, looking into this, it's actually rather painful.
> - We have a check renderer side as well that prevents duplicate installs
early.
> - But that won't catch all cases - e.g., if two separate JS contexts call it.
> - They emit separate error messages. :(
> 
> I added a test for the most common case - two calls from the same JS context -
Thanks!
> but I'm not sure it's worth it to manufacture a test for the other.  
Maybe file a bug (GoodFirstBug?) to add the test?
I can clearly see how the test can be painful to write :), I'd file a bug
instead then with some notes about this as this is not an uncommon use case.
Just for a note to put in the bug: I'd say window.open and window.opener might
be helpful for the test to execute 2 install calls from 2 different JS contexts.
Let me know
> what you think.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:481: // TODO(devlin): We should adddress
the case of the extension already
On 2016/06/29 22:32:09, Devlin wrote:
> On 2016/06/29 21:03:04, lazyboy wrote:
> > Doesn't the beginning of the function prevent this (extension already being
> > installed) now?
> 
> Nope.  The beginning prevents two inline installations at the same time, but
if
> the item is already installed and enabled, it doesn't.

Acknowledged.

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:277: std::map<std::string,
std::unique_ptr<InlineInstallObserver>>
On 2016/06/29 22:32:09, Devlin wrote:
> On 2016/06/29 21:03:04, lazyboy wrote:
> > Add comment describing this member. or maybe use ExtensionId instead of
> > std::string to self describe. 
> 
> Done. Opted against ExtensionId because a) it would involve #including the
> heavy-weight extension.h file* and b) it's inconsistent with the rest of this
> file.
> 
> * We should probably pull it out into extension_id.h
> ** We should probably fix that, too, but out of scope for this change.

Acknowledged.

https://codereview.chromium.org/2103663002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webstore_inline_install/double_install.html
(right):

https://codereview.chromium.org/2103663002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webstore_inline_install/double_install.html:25:
function(e) {
nit: Is "errorCode" passed as 2nd param here? Then it's nitwise better to
compare that string instead which is supposed to be stable.

[Devlin, 2016-06-30 14:46:55]

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

https://codereview.chromium.org/2103663002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:447: if
(pending_inline_installations_.count(webstore_item_id) != 0) {
On 2016/06/29 23:10:27, lazyboy wrote:
> On 2016/06/29 22:32:09, Devlin wrote:
> > On 2016/06/29 21:03:03, lazyboy wrote:
> > > This seems like a new behavior? Test would be helpful :)
> > 
> > D'oh, I knew you would say that! ;)
> > 
> > So, looking into this, it's actually rather painful.
> > - We have a check renderer side as well that prevents duplicate installs
> early.
> > - But that won't catch all cases - e.g., if two separate JS contexts call
it.
> > - They emit separate error messages. :(
> > 
> > I added a test for the most common case - two calls from the same JS context
-
> Thanks!
> > but I'm not sure it's worth it to manufacture a test for the other.  
> Maybe file a bug (GoodFirstBug?) to add the test?
> I can clearly see how the test can be painful to write :), I'd file a bug
> instead then with some notes about this as this is not an uncommon use case.
> Just for a note to put in the bug: I'd say window.open and window.opener might
> be helpful for the test to execute 2 install calls from 2 different JS
contexts.
> Let me know
> > what you think.
> 

Filed a bug: 624614

https://codereview.chromium.org/2103663002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webstore_inline_install/double_install.html
(right):

https://codereview.chromium.org/2103663002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webstore_inline_install/double_install.html:25:
function(e) {
On 2016/06/29 23:10:27, lazyboy wrote:
> nit: Is "errorCode" passed as 2nd param here? Then it's nitwise better to
> compare that string instead which is supposed to be stable.

Done.

[Devlin, 2016-06-30 14:47:04]

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

[Devlin, 2016-06-30 14:47:05]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2103663002/#ps100001
(title: "missingfile")

[commit-bot: I haz the power, 2016-06-30 14:47:18]

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, 2016-06-30 16:02:48]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-30 16:02:59]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 16:04:51]

Description was changed from

==========
[Extensions] Rework inline installation observation

Instead of observing through the WebstoreAPI, observe directly in the TabHelper.
This is a great deal less code, more direct, and also fixes a lifetime issue
with the TabHelper being deleted before the inline installation completes.

BUG=613949
==========

to

==========
[Extensions] Rework inline installation observation

Instead of observing through the WebstoreAPI, observe directly in the TabHelper.
This is a great deal less code, more direct, and also fixes a lifetime issue
with the TabHelper being deleted before the inline installation completes.

BUG=613949

Committed: https://crrev.com/d30a8bd191f17b61938fc87890bffc80049b0774
Cr-Commit-Position: refs/heads/master@{#403188}
==========

[commit-bot: I haz the power, 2016-06-30 16:04:52]

Patchset 4 (id:??) landed as
https://crrev.com/d30a8bd191f17b61938fc87890bffc80049b0774
Cr-Commit-Position: refs/heads/master@{#403188}