Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(29)

Issue 1403293008: Don't allow inline install if frame is deleted before user accepts (Closed)

Created:
3 years, 7 months ago by asargent_no_longer_on_chrome
Modified:
3 years, 7 months ago
Reviewers:
Devlin
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't allow inline install if frame is deleted before user accepts If the frame that called the chrome.webstore.install method to begin an inline install gets deleted before the user accepts from the dialog, we don't want the install to continue because a navigation could make it look like the install request was coming from some unrelated site. One downside of this approach is that the dialog stays around even after the frame is deleted, and hitting either accept or cancel buttons both just cancel the install. It would be better if the dialog is automatically cancelled, but doing that would involve a lot more refactoring. The approach in this CL was easier and is probably worth getting out, and we can improve on it in the future. BUG=550047 Committed: https://crrev.com/bbe84115d3dc969bfcf6ca87bebd1f5608db6ecf Cr-Commit-Position: refs/heads/master@{#361218}

Patch Set 1 #

Total comments: 8

Patch Set 2 : codereview feedback #

Total comments: 6

Patch Set 3 : more fixes #

Total comments: 4

Patch Set 4 : better way of handling install result checking #

Patch Set 5 : fix unit test compile failure #

Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -22 lines) Patch
M chrome/browser/extensions/tab_helper.cc View 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/extensions/webstore_inline_installer.h View 1 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/extensions/webstore_inline_installer.cc View 1 5 chunks +15 lines, -4 lines 0 comments Download
M chrome/browser/extensions/webstore_inline_installer_browsertest.cc View 1 2 3 4 chunks +89 lines, -8 lines 0 comments Download
M chrome/browser/extensions/webstore_inline_installer_factory.h View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/webstore_inline_installer_factory.cc View 1 chunk +7 lines, -6 lines 0 comments Download
M chrome/browser/extensions/webstore_inline_installer_unittest.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/webstore_inline_install/empty.html View 1 0 chunks +-1 lines, --1 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
asargent_no_longer_on_chrome
PTAL
3 years, 7 months ago (2015-11-05 01:17:02 UTC) #2
Devlin
https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/webstore_inline_installer.cc File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/webstore_inline_installer.cc#newcode180 chrome/browser/extensions/webstore_inline_installer.cc:180: void WebstoreInlineInstaller::FrameDeleted( There's been a ton of confusion over ...
3 years, 7 months ago (2015-11-05 17:40:40 UTC) #3
asargent_no_longer_on_chrome
ok, this is ready for another look https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/webstore_inline_installer.cc File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/webstore_inline_installer.cc#newcode180 chrome/browser/extensions/webstore_inline_installer.cc:180: void WebstoreInlineInstaller::FrameDeleted( ...
3 years, 7 months ago (2015-11-18 21:42:44 UTC) #4
Devlin
lgtm https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensions/webstore_inline_installer_browsertest.cc File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensions/webstore_inline_installer_browsertest.cc#newcode131 chrome/browser/extensions/webstore_inline_installer_browsertest.cc:131: // Returns a pointer to the last installer ...
3 years, 7 months ago (2015-11-18 21:48:26 UTC) #5
asargent_no_longer_on_chrome
Addressed comments - the last one required adding a bit of extra code to the ...
3 years, 7 months ago (2015-11-20 01:11:48 UTC) #6
Devlin
still lgtm https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensions/webstore_inline_installer_browsertest.cc File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensions/webstore_inline_installer_browsertest.cc#newcode116 chrome/browser/extensions/webstore_inline_installer_browsertest.cc:116: bool success; nitty nit: please initialize these ...
3 years, 7 months ago (2015-11-20 01:27:19 UTC) #7
asargent_no_longer_on_chrome
thanks for review, sending to CQ now https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensions/webstore_inline_installer_browsertest.cc File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensions/webstore_inline_installer_browsertest.cc#newcode116 chrome/browser/extensions/webstore_inline_installer_browsertest.cc:116: bool success; ...
3 years, 7 months ago (2015-11-20 23:56:46 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403293008/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403293008/60001
3 years, 7 months ago (2015-11-20 23:58:59 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/16983) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, ...
3 years, 7 months ago (2015-11-21 00:15:21 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403293008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403293008/80001
3 years, 7 months ago (2015-11-23 21:39:11 UTC) #17
commit-bot: I haz the power
Committed patchset #5 (id:80001)
3 years, 7 months ago (2015-11-23 23:40:14 UTC) #18
commit-bot: I haz the power
3 years, 7 months ago (2015-11-23 23:41:05 UTC) #19
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/bbe84115d3dc969bfcf6ca87bebd1f5608db6ecf
Cr-Commit-Position: refs/heads/master@{#361218}

Powered by Google App Engine
This is Rietveld 408576698