Prevent inline CRX installs in popup windows.

Prevent inline CRX installs in popup windows.

BUG=416929
TEST=WebstoreInlineInstallerTest.ShouldBlockInlineInstallFromPopupWindow

Committed: https://crrev.com/92aa131bcb2699d8d6184a9dabcf5f256002dc65
Cr-Commit-Position: refs/heads/master@{#316679}

[meacer, 2015-02-11 21:44:13]

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

[meacer, 2015-02-11 21:44:14]

Devlin, can you PTAL? Thanks!

[Devlin, 2015-02-12 17:18:21]

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/webstore_inline_installer.cc:134: if (browser &&
browser->is_type_popup()) {
Under what circumstances can browser be null?  If it can be, do we really want
to proceed with installation?

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:14: #include
"chrome/browser/ui/browser_finder.h"
needed?

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/webstore_installer_test.h (right):

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/webstore_installer_test.h:20: }  // namespace contents
nit: We usually don't have the "// namespace foo" for these really tiny forward
declarations.  Why don't we take this one out, and also the one on line 16.

https://codereview.chromium.org/908063002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/webstore_inline_install/install_from_popup.html
(right):

https://codereview.chromium.org/908063002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/webstore_inline_install/install_from_popup.html:24:
console.log('Ran test, sending response');
I know that we do this a lot, but we really shouldn't log much in the case of a
successful test.  We can keep the messages in the failure cases, but please take
out this one.

Also, we should probably update the other logs to be console.errors.

[meacer, 2015-02-12 21:12:05]

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/webstore_inline_installer.cc:134: if (browser &&
browser->is_type_popup()) {
On 2015/02/12 17:18:21, Devlin wrote:
> Under what circumstances can browser be null?  If it can be, do we really want
> to proceed with installation?

Not sure if it can ever be null, but if it is I agree that the install should
not proceed. I added a DCHECK, we can revisit if we see crashes (that would
indicate there is some bug elsewhere anyways).

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:14: #include
"chrome/browser/ui/browser_finder.h"
On 2015/02/12 17:18:21, Devlin wrote:
> needed?

Not anymore :)

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/webstore_installer_test.h (right):

https://codereview.chromium.org/908063002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/webstore_installer_test.h:20: }  // namespace contents
On 2015/02/12 17:18:21, Devlin wrote:
> nit: We usually don't have the "// namespace foo" for these really tiny
forward
> declarations.  Why don't we take this one out, and also the one on line 16.

Yeah, this looked odd to me too. Done.

https://codereview.chromium.org/908063002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/webstore_inline_install/install_from_popup.html
(right):

https://codereview.chromium.org/908063002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/webstore_inline_install/install_from_popup.html:24:
console.log('Ran test, sending response');
On 2015/02/12 17:18:21, Devlin wrote:
> I know that we do this a lot, but we really shouldn't log much in the case of
a
> successful test.  We can keep the messages in the failure cases, but please
take
> out this one.
> 
> Also, we should probably update the other logs to be console.errors.

Done.

[Devlin, 2015-02-12 21:17:28]

lgtm

[meacer, 2015-02-12 21:40:17]

Thanks Devlin.

[meacer, 2015-02-12 21:40:22]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2015-02-12 21:41:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908063002/60001

[commit-bot: I haz the power, 2015-02-12 22:47:40]

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

[commit-bot: I haz the power, 2015-02-12 22:47:41]

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

[meacer, 2015-02-13 23:42:03]

On 2015/02/12 22:47:41, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

WebstoreInlineInstallerTest.ShouldBlockInlineInstallFromPopupWindow is failing
on a ChromeOS bot. Looking at it.

[meacer, 2015-02-14 02:27:48]

New patchsets have been uploaded after l-g-t-m from rdevlin.cronin@chromium.org

[meacer, 2015-02-14 02:29:55]

Devlin, fyi: I changed the browser test so that inline_install_popup.html now
opens its own popup window which does tries to do the installation. Just opening
inline_install_popup.html as a popup didn't work on a ChromeOS bot.

[meacer, 2015-02-17 21:40:32]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2015-02-17 21:42:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908063002/80001

[commit-bot: I haz the power, 2015-02-17 22:36:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-17 22:37:38]

Patchset 5 (id:??) landed as
https://crrev.com/92aa131bcb2699d8d6184a9dabcf5f256002dc65
Cr-Commit-Position: refs/heads/master@{#316679}