Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(238)

Issue 662073002: Fix crash when user closes window prior to the "Confirm Install" prompt showing (Closed)

Created:
6 years, 2 months ago by pkotwicz
Modified:
6 years, 1 month ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, Ken Rockot(use gerrit already)
Base URL:
https://chromium.googlesource.com/chromium/src.git/+/install_prompt_navigator
Project:
chromium
Visibility:
Public.

Description

Fix crash when user closes window prior to the "Confirm Install" prompt showing. This CL moves ExtensionInstallPrompt::ShowParams to its own class. The new class observes the content::WebContents and gfx::NativeWindow for their destruction. If the window is closed prior to the "Confirm Install" prompt showing, the prompt is shown as a non-modal free-standing dialog. BUG=422814 TEST=ExtensionInstallPromptBrowserTest.* Committed: https://crrev.com/d53564d3743bc2da768b38f0db34090a7bb43fea Cr-Commit-Position: refs/heads/master@{#301674}

Patch Set 1 : #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 5

Patch Set 4 : #

Patch Set 5 : #

Total comments: 1

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 2

Patch Set 9 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+572 lines, -76 lines) Patch
M chrome/browser/extensions/extension_install_prompt.h View 1 4 chunks +3 lines, -22 lines 0 comments Download
M chrome/browser/extensions/extension_install_prompt.cc View 1 2 3 4 5 6 chunks +11 lines, -24 lines 0 comments Download
A chrome/browser/extensions/extension_install_prompt_browsertest.cc View 1 1 chunk +140 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_install_prompt_show_params.h View 1 2 3 1 chunk +64 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_install_prompt_show_params.cc View 1 2 3 4 5 1 chunk +100 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_install_prompt_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/external_install_error.h View 1 3 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/external_install_error.cc View 1 3 chunks +5 lines, -3 lines 0 comments Download
A chrome/browser/ui/aura/native_window_tracker_aura.h View 1 2 3 4 5 1 chunk +29 lines, -0 lines 0 comments Download
A chrome/browser/ui/aura/native_window_tracker_aura.cc View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm View 1 4 chunks +7 lines, -6 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_browsertest.mm View 1 2 3 4 5 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/windowed_install_dialog_controller.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/extensions/windowed_install_dialog_controller.mm View 1 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/windowed_install_dialog_controller_browsertest.mm View 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/ui/cocoa/native_window_tracker_cocoa.h View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
A chrome/browser/ui/cocoa/native_window_tracker_cocoa.mm View 1 2 3 4 5 1 chunk +66 lines, -0 lines 0 comments Download
A chrome/browser/ui/native_window_tracker.h View 1 2 3 4 5 6 7 8 1 chunk +22 lines, -0 lines 0 comments Download
A chrome/browser/ui/native_window_tracker_browsertest.cc View 1 2 3 4 5 6 7 8 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_install_dialog_view.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/extensions/extension_install_dialog_view.cc View 1 2 3 2 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (10 generated)
pkotwicz
asargent@ can you please take a look? This CL shows a toplevel non-modal dialog if ...
6 years, 2 months ago (2014-10-18 00:34:10 UTC) #5
asargent_no_longer_on_chrome
On 2014/10/18 00:34:10, pkotwicz wrote: > asargent@ can you please take a look? > > ...
6 years, 2 months ago (2014-10-20 21:17:57 UTC) #7
pkotwicz
How do you suggest implementing "canceling the install if the tab gets closed"? There are ...
6 years, 2 months ago (2014-10-20 21:57:54 UTC) #8
asargent_no_longer_on_chrome
On 2014/10/20 21:57:54, pkotwicz wrote: > How do you suggest implementing "canceling the install if ...
6 years, 2 months ago (2014-10-20 23:27:06 UTC) #9
pkotwicz
I do not know of any places which use arbitrary parent web contents except for ...
6 years, 2 months ago (2014-10-20 23:55:14 UTC) #10
asargent_no_longer_on_chrome
On 2014/10/20 23:55:14, pkotwicz wrote: > I do not know of any places which use ...
6 years, 2 months ago (2014-10-21 20:33:06 UTC) #11
pkotwicz
asargent@ can you please take another look? I have changed the logic such that the ...
6 years, 2 months ago (2014-10-23 01:19:21 UTC) #12
pkotwicz
asargent@ can you please take another look? I have changed the logic such that the ...
6 years, 2 months ago (2014-10-23 01:19:22 UTC) #13
asargent_no_longer_on_chrome
lgtm
6 years, 1 month ago (2014-10-23 20:43:45 UTC) #14
pkotwicz
Scott, can you please look at the changes in ui/base? I am not sure where ...
6 years, 1 month ago (2014-10-23 21:14:20 UTC) #16
pkotwicz
Scott, can you please look at the changes in ui/base? I am not sure where ...
6 years, 1 month ago (2014-10-23 21:14:22 UTC) #17
sky
Is there a reason window listener needs to be in ui/base and not chrome?
6 years, 1 month ago (2014-10-23 22:33:46 UTC) #18
pkotwicz
I put the code in ui/base because I thought it was a good place given ...
6 years, 1 month ago (2014-10-23 23:40:45 UTC) #19
sky
On Thu, Oct 23, 2014 at 4:40 PM, <pkotwicz@chromium.org> wrote: > I put the code ...
6 years, 1 month ago (2014-10-24 04:09:38 UTC) #20
pkotwicz
Scott can you please take another look?
6 years, 1 month ago (2014-10-24 19:14:37 UTC) #21
sky
LGTM https://codereview.chromium.org/662073002/diff/140001/chrome/browser/ui/aura/native_window_deletion_observer_aura.cc File chrome/browser/ui/aura/native_window_deletion_observer_aura.cc (right): https://codereview.chromium.org/662073002/diff/140001/chrome/browser/ui/aura/native_window_deletion_observer_aura.cc#newcode11 chrome/browser/ui/aura/native_window_deletion_observer_aura.cc:11: : window_(window) { nit: indent 2 more. https://codereview.chromium.org/662073002/diff/140001/chrome/browser/ui/aura/native_window_deletion_observer_aura.cc#newcode21 ...
6 years, 1 month ago (2014-10-24 20:22:56 UTC) #23
pkotwicz
thakis@ for chrome/browser/ui/cocoa native_window_tracker_cocoa.* in particular
6 years, 1 month ago (2014-10-27 20:56:31 UTC) #26
Nico
test for the new cocoa class? https://codereview.chromium.org/662073002/diff/200001/chrome/browser/ui/cocoa/native_window_tracker_cocoa.mm File chrome/browser/ui/cocoa/native_window_tracker_cocoa.mm (right): https://codereview.chromium.org/662073002/diff/200001/chrome/browser/ui/cocoa/native_window_tracker_cocoa.mm#newcode59 chrome/browser/ui/cocoa/native_window_tracker_cocoa.mm:59: return [bridge_ isNSWindowAlive]; ...
6 years, 1 month ago (2014-10-27 21:06:11 UTC) #27
pkotwicz
I am not sure if I understand your comment. What do you mean by an ...
6 years, 1 month ago (2014-10-27 21:16:50 UTC) #28
pkotwicz
I am not sure if I understand your comment. What do you mean by an ...
6 years, 1 month ago (2014-10-27 21:16:52 UTC) #29
Nico
On 2014/10/27 21:16:52, pkotwicz wrote: > I am not sure if I understand your comment. ...
6 years, 1 month ago (2014-10-27 21:32:24 UTC) #30
Nico
On 2014/10/27 21:32:24, Nico wrote: > On 2014/10/27 21:16:52, pkotwicz wrote: > > I am ...
6 years, 1 month ago (2014-10-27 21:33:20 UTC) #31
pkotwicz
Nico, can you please take another look? I have added NativeWindowTrackerTest and have renamed NativeWindowTracker::IsWindowAlive() ...
6 years, 1 month ago (2014-10-28 17:09:43 UTC) #32
Nico
lgtm, thanks! https://codereview.chromium.org/662073002/diff/260001/chrome/browser/ui/native_window_tracker.h File chrome/browser/ui/native_window_tracker.h (right): https://codereview.chromium.org/662073002/diff/260001/chrome/browser/ui/native_window_tracker.h#newcode13 chrome/browser/ui/native_window_tracker.h:13: public: nit: indent looks off for most ...
6 years, 1 month ago (2014-10-28 17:19:28 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/662073002/270001
6 years, 1 month ago (2014-10-28 18:09:57 UTC) #35
commit-bot: I haz the power
Committed patchset #9 (id:270001)
6 years, 1 month ago (2014-10-28 19:03:18 UTC) #36
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/d53564d3743bc2da768b38f0db34090a7bb43fea Cr-Commit-Position: refs/heads/master@{#301674}
6 years, 1 month ago (2014-10-28 19:04:00 UTC) #37
pkotwicz
A revert of this CL (patchset #9 id:270001) has been created in https://codereview.chromium.org/687463003/ by pkotwicz@chromium.org. ...
6 years, 1 month ago (2014-10-28 20:38:36 UTC) #38
pkotwicz
6 years, 1 month ago (2014-10-28 20:38:49 UTC) #39
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:270001) has been created in
https://codereview.chromium.org/683993002/ by pkotwicz@chromium.org.

The reason for reverting is: Broke test on the GPU waterfall
http://build.chromium.org/p/chromium.gpu/builders/GPU%20Mac%20Builder%20%28db....

Powered by Google App Engine
This is Rietveld 408576698