Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
382133003:
Refactored ExtensionUninstallDialog to take a NativeWindow instead of a Browser (Closed)
|
Created:
6 years, 5 months ago by sashab Modified:
6 years, 5 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, benwells, tmdiep Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRefactored UninstallDialog to take a NativeWindow instead of a Browser
Originally, ExtensionUninstallDialog took a Browser argument (which
determined which browser window it should be modal to), and a special
case was added so the dialog would be modal to the app list if the
Browser argument was NULL. Updated this constructor to take the parent
NativeWindow directly instead of a Browser, so the dialog can be modal
to any parent window. For example, this allows it to be parent to the
App Info dialog, and be a proper standalone dialog in
ExtensionStorageMonitor. Further refactors can be made as a result of
this CL since some of the callsites could be getting a Browser just to
be able to launch the dialog, whereas that is not needed anymore if the
intention is to launch it as a non-modal dialog.
Updated uninstall dialogs launched from the following callsites:
- The management Chrome API
- The extensions context menu
- The 'app disabled' message for when it has requested new permissions
- The chrome://extensions page
- The context menu of apps in the app list
- The 'remove' button in the App Info dialog
- The context menu on the New Tab page
BUG=388746, 362308
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285541
Patch Set 1 #
Total comments: 13
Patch Set 2 : Added CreateAsStandaloneDialog for extension_storage_monitor #
Total comments: 6
Patch Set 3 : Review feedback #
Total comments: 9
Patch Set 4 : Moved Browser out of ExtensionUninstallDialogViews #
Total comments: 9
Patch Set 5 : Moved Browser and GetParent() logic into constructor #
Total comments: 5
Patch Set 6 : Changed ExtensionUninstallDialog to take a NativeWindow instead of a Browser #
Total comments: 4
Patch Set 7 : Small change: Display the default icon while the real icon is being loaded #
Total comments: 6
Patch Set 8 : Removed the BrowserFinder method, and kept the dialog unconstructed until it's shown #
Total comments: 9
Patch Set 9 : Small review fixes from feedback #
Total comments: 24
Patch Set 10 : Review feedback & todos for unfixed issues #Messages
Total messages: 34 (0 generated)
tapted@ please initially review whole CL.
https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_storage_monitor.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_storage_monitor.cc:557: AppListService::Get(chrome::GetActiveDesktop())->GetAppListWindow(), I don't think this call site is intending to use the app launcher r275262 suggests it could be from a piece of notification UI, in which case the right solution is probably to center it in the screen. (Thanh-Mai probably knows the answer :) https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_uninstall_dialog.h:55: static ExtensionUninstallDialog* CreateForNonBrowserWindow( I'd probably still call it CreateForAppList - it's still design for this (just with the parent sometimes the root app list window, and sometimes a child widget, but that's about to change when the info dialog becomes a subview rather than a widget. I would still pass in |parent| - it allows the GetActiveDesktop thing to go away. https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:135: if (browser_ && !GetParent(browser_)) { From the earlier CL, this needed a comment - If I understand the comment correctly, something like // Callers of ExtensionUninstallDialog::Create() expect a non-NULL result, but it's possible that a browser window couldn't be found. In that case, immediately cancel the dialog when shown. https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:142: CreateBrowserModalDialogViews(view_, parent_ ? parent_ : GetParent(browser_)) What happens if both browser_ and parent_ are NULL? I think this will segfault. Currently I *think* CreateBrowserModalDialogViews has a fallback to center the dialog in the screen, which you might need to keep. https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:256: extensions::ExtensionUninstallDialog::CreateForNonBrowserWindow( You'll need a mac version of this too
tapted: Please complete review tmdiep: Please look at extension_storage_monitor and advise :) https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_storage_monitor.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_storage_monitor.cc:557: AppListService::Get(chrome::GetActiveDesktop())->GetAppListWindow(), On 2014/07/11 04:39:15, tapted wrote: > I don't think this call site is intending to use the app launcher r275262 > suggests it could be from a piece of notification UI, in which case the right > solution is probably to center it in the screen. (Thanh-Mai probably knows the > answer :) Done. https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_uninstall_dialog.h:55: static ExtensionUninstallDialog* CreateForNonBrowserWindow( On 2014/07/11 04:39:15, tapted wrote: > I'd probably still call it CreateForAppList - it's still design for this (just > with the parent sometimes the root app list window, and sometimes a child > widget, but that's about to change when the info dialog becomes a subview rather > than a widget. > > I would still pass in |parent| - it allows the GetActiveDesktop thing to go > away. Done. https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:135: if (browser_ && !GetParent(browser_)) { On 2014/07/11 04:39:15, tapted wrote: > From the earlier CL, this needed a comment - If I understand the comment > correctly, something like > > // Callers of ExtensionUninstallDialog::Create() expect a non-NULL result, but > it's possible that a browser window couldn't be found. In that case, immediately > cancel the dialog when shown. Yup, I understand. Done. https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:142: CreateBrowserModalDialogViews(view_, parent_ ? parent_ : GetParent(browser_)) On 2014/07/11 04:39:15, tapted wrote: > What happens if both browser_ and parent_ are NULL? I think this will segfault. > Currently I *think* CreateBrowserModalDialogViews has a fallback to center the > dialog in the screen, which you might need to keep. If you follow the logic carefully, this is actually the exact same behaviour as before (show_in_app_list_ is !browser_). The comment on the constructor says that exactly one of |browser_| and |parent_| must be NULL, so they can't both be NULL. We know GetParent(browser_) is not-NULL because the previous if-statement returns. CreateBrowserModalDialogViews is actually safe to call with NULL anyway - it just creates a non-parented window (so an independent window). So even if they could both be NULL, this wouldn't segfault. I originally had a DCHECK(parent_ ^ browser_) somewhere, but thought it was too obscure/reflected badly in the class's design. So I left it open here, and as you can see, it's probably safe if they're both NULL anyway. https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:256: extensions::ExtensionUninstallDialog::CreateForNonBrowserWindow( On 2014/07/11 04:39:15, tapted wrote: > You'll need a mac version of this too Done. Please test that the Uninstall dialog still works from the Mac app list for me :)
lgtm. don't forget to update the CL description. haven't checked mac yet, but it looks pretty safe https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:142: CreateBrowserModalDialogViews(view_, parent_ ? parent_ : GetParent(browser_)) On 2014/07/11 05:58:57, sasha_b wrote: > On 2014/07/11 04:39:15, tapted wrote: > > What happens if both browser_ and parent_ are NULL? I think this will > segfault. > > Currently I *think* CreateBrowserModalDialogViews has a fallback to center the > > dialog in the screen, which you might need to keep. > > If you follow the logic carefully, this is actually the exact same behaviour as > before (show_in_app_list_ is !browser_). The comment on the constructor says > that exactly one of |browser_| and |parent_| must be NULL, so they can't both be > NULL. We know GetParent(browser_) is not-NULL because the previous if-statement > returns. > > CreateBrowserModalDialogViews is actually safe to call with NULL anyway - it > just creates a non-parented window (so an independent window). So even if they > could both be NULL, this wouldn't segfault. > > I originally had a DCHECK(parent_ ^ browser_) somewhere, but thought it was too > obscure/reflected badly in the class's design. So I left it open here, and as > you can see, it's probably safe if they're both NULL anyway. ah - I think I read it as ... : GetParent(browser) ->Show()); instead of ... : GetParent(browser)) ->Show(); I think a separate variable will help here - we're spread across 2 lines anyway. i.e. gfx::NativeWindow parent = parent ? parent : GetParent(browser_); CreateBrowserModalDialogViews(view_, parent)->Show(); https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:53: // Create an platform-specific implementation of ExtensionUninstallDialog for nit: an -> a https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:59: // Create an platform-specific implementation of ExtensionUninstallDialog with nit: an -> a
lgtm for extension_storage_monitor, but just remove the includes. https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_storage_monitor.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_storage_monitor.cc:557: AppListService::Get(chrome::GetActiveDesktop())->GetAppListWindow(), On 2014/07/11 04:39:15, tapted wrote: > I don't think this call site is intending to use the app launcher r275262 > suggests it could be from a piece of notification UI, in which case the right > solution is probably to center it in the screen. (Thanh-Mai probably knows the > answer :) Yep, this isn't related to the app launcher. The uninstall is initiated by an action in a message center notification. We don't have a parent window for this, so just center it in the screen. https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_storage_monitor.cc (right): https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_storage_monitor.cc:18: #include "chrome/browser/ui/host_desktop.h" Looks like you don't need these includes anymore.
kalman@chromium.org: Please review changes in chrome/browser/extensions msw@chromium.org: Please review changes in chrome/browser/ui https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:142: CreateBrowserModalDialogViews(view_, parent_ ? parent_ : GetParent(browser_)) On 2014/07/11 06:10:40, tapted wrote: > On 2014/07/11 05:58:57, sasha_b wrote: > > On 2014/07/11 04:39:15, tapted wrote: > > > What happens if both browser_ and parent_ are NULL? I think this will > > segfault. > > > Currently I *think* CreateBrowserModalDialogViews has a fallback to center > the > > > dialog in the screen, which you might need to keep. > > > > If you follow the logic carefully, this is actually the exact same behaviour > as > > before (show_in_app_list_ is !browser_). The comment on the constructor says > > that exactly one of |browser_| and |parent_| must be NULL, so they can't both > be > > NULL. We know GetParent(browser_) is not-NULL because the previous > if-statement > > returns. > > > > CreateBrowserModalDialogViews is actually safe to call with NULL anyway - it > > just creates a non-parented window (so an independent window). So even if they > > could both be NULL, this wouldn't segfault. > > > > I originally had a DCHECK(parent_ ^ browser_) somewhere, but thought it was > too > > obscure/reflected badly in the class's design. So I left it open here, and as > > you can see, it's probably safe if they're both NULL anyway. > > ah - I think I read it as > > ... : GetParent(browser) > ->Show()); > > instead of > > ... : GetParent(browser)) > ->Show(); > > > I think a separate variable will help here - we're spread across 2 lines anyway. > i.e. > > gfx::NativeWindow parent = parent ? parent : GetParent(browser_); > CreateBrowserModalDialogViews(view_, parent)->Show(); > > Done. https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_storage_monitor.cc (right): https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_storage_monitor.cc:18: #include "chrome/browser/ui/host_desktop.h" On 2014/07/11 06:13:29, tmdiep wrote: > Looks like you don't need these includes anymore. Done. https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:53: // Create an platform-specific implementation of ExtensionUninstallDialog for On 2014/07/11 06:10:41, tapted wrote: > nit: an -> a Done. https://codereview.chromium.org/382133003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:59: // Create an platform-specific implementation of ExtensionUninstallDialog with On 2014/07/11 06:10:40, tapted wrote: > nit: an -> a Done.
https://codereview.chromium.org/382133003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:54: // the app list (or a child widget in the app list). nit: explain high-level how/why the dialog is different in this case. https://codereview.chromium.org/382133003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:61: static ExtensionUninstallDialog* CreateAsStandaloneDialog(Profile* profile, Can't users just call Create(profile, NULL, delegate)? https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:120: Browser* browser, The |browser| param is only ever used to get it's NativeWindow to use as |parent_|. Why not remove the |browser| param from the [Views] ctors and just pass in GetParent(browser)? https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:137: // Callers of ExtensionUninstallDialog::Create() expect a non-NULL result, but nit: why not just return a "standalone" dialog in that case?
https://codereview.chromium.org/382133003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:54: // the app list (or a child widget in the app list). On 2014/07/14 17:38:31, msw wrote: > nit: explain high-level how/why the dialog is different in this case. is it possible to refactor ::Create to take a gfx::NativeWindow and not need this at all? what's the Browser needed for there? https://codereview.chromium.org/382133003/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:61: static ExtensionUninstallDialog* CreateAsStandaloneDialog(Profile* profile, On 2014/07/14 17:38:31, msw wrote: > Can't users just call Create(profile, NULL, delegate)? TBH if a parameter is nullable I prefer providing an overloaded method without that parameter. what does it mean for a dialogue to "not have a parent window"? that it's not modal? anyway, I think Create() is a better name.
kalman and msw: Just before I proceed with other review feedback, you both made a similar comment about modifying Create to just take a NativeWindow. Please take a look at my comment below. https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:120: Browser* browser, On 2014/07/14 17:38:31, msw wrote: > The |browser| param is only ever used to get it's NativeWindow to use as > |parent_|. Why not remove the |browser| param from the [Views] ctors and just > pass in GetParent(browser)? Not quite (I also thought this as I was refactoring). If you look closely in ExtensionUninstallDialogViews::Show() (the existing or current implementation), there's a check to see if browser_ is set but browser_->window() is not (ie CreateBrowserWindow has not finished), and if so, cancel the dialog. I guess we could move this check upwards, ie callers check browser()->window() and pass GetNativeWindow(), but I think there could be a code path where browser()->window() is not set when this object is constructed, but it will be set by the time Show() is called.
https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:120: Browser* browser, On 2014/07/17 23:24:41, sasha_b wrote: > On 2014/07/14 17:38:31, msw wrote: > > The |browser| param is only ever used to get it's NativeWindow to use as > > |parent_|. Why not remove the |browser| param from the [Views] ctors and just > > pass in GetParent(browser)? > > Not quite (I also thought this as I was refactoring). If you look closely in > ExtensionUninstallDialogViews::Show() (the existing or current implementation), > there's a check to see if browser_ is set but browser_->window() is not (ie > CreateBrowserWindow has not finished), and if so, cancel the dialog. Yeah, I asked: "nit: why not just return a "standalone" dialog in that case?" > I guess we could move this check upwards, ie callers check browser()->window() > and pass GetNativeWindow(), but I think there could be a code path where > browser()->window() is not set when this object is constructed, but it will be > set by the time Show() is called. I think that's unlikely, and not worth supporting without evidence.
Alright, I've done my best :) Please take a look, kalman@ and msw@. The Browser object is needed for registering an observer that closes the dialog when the browser is closed (which can happen if the browser is closed from, for example, the taskbar jump menu). I've also left in the functionality that closes the dialog immediately if no browser windows are detected. I think this *could* happen - an API can open an Uninstall dialog, for example - and if it happens when no browser windows are open, we probably shouldn't be displaying that dialog. There are a few improvements I could make if we *don't* want to support this case (ie just return a standalone dialog, as suggested), but I think we should. Let me know what you think :) Thanks, Sasha https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:120: Browser* browser, > I think that's unlikely, and not worth supporting without evidence. I'm of mixed opinions on this one. Technically, if the uninstall dialog *is* launched without a browser window, we probably want to display it as a standalone dialog anyway. But the code is there to close the dialog immediately if there is no browser window. I guess if an API creates an uninstall dialog while Chrome is closing, we don't want to open the dialog at all (we want to cancel it immediately. I've left that case in there for now, with comments, for this reason. https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.cc:59: void ExtensionUninstallDialog::RegisterObserver(Browser* browser) { We can actually remove the need for the Browser object at all, using chrome::FindBrowserWithWindow(parent). However, if parent is NULL, we have no way of knowing whether it was meant to be a browser or not, since the parent can be NULL but the browser can be not null.
(don't forget to update the CL description :) https://codereview.chromium.org/382133003/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm (right): https://codereview.chromium.org/382133003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:79: new ExtensionUninstallDialogCocoa(profile, GetParent(browser), delegate); I don't think GetParent is available here https://codereview.chromium.org/382133003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:83: // static nit: keep blank line before
My next comment(s) depend on the answer to the "I find this confusing" question. https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:54: // a modal to the given parent window. If |parent| is NULL, creates a I find this comment a bit confusing, since you say that it's modal to the given parent window, but parent can be NULL. if parent is NULL does that mean that it's modal to the entire desktop? does it mean it's not modal at all? https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:82: virtual void RegisterObserver(Browser* browser); I prefer the Browser-in-constructor approach to needing this. The inheritance and construction behaviour is pretty convoluted already.
Sorry about the back-and-forth; this is more complicated than I originally thought. https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:54: // a modal to the given parent window. If |parent| is NULL, creates a On 2014/07/21 20:07:48, kalman wrote: > I find this comment a bit confusing, since you say that it's modal to the given > parent window, but parent can be NULL. if parent is NULL does that mean that > it's modal to the entire desktop? does it mean it's not modal at all? Yeah... It's a tradeoff between making another method, and keeping this one with the nullable parameter. I've changed the comment for now, but I'm happy to make CreateStandaloneDialog again, which just has the second parameter as NULL. https://codereview.chromium.org/382133003/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:82: virtual void RegisterObserver(Browser* browser); On 2014/07/21 20:07:48, kalman wrote: > I prefer the Browser-in-constructor approach to needing this. The inheritance > and construction behaviour is pretty convoluted already. Agreed. In that case, I also moved the GetParent() logic into the ExtensionUninstallDialog constructor, but now the constructor is more complicated. It's a tough call... https://codereview.chromium.org/382133003/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm (right): https://codereview.chromium.org/382133003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:79: new ExtensionUninstallDialogCocoa(profile, GetParent(browser), delegate); On 2014/07/21 03:41:26, tapted wrote: > I don't think GetParent is available here No longer needed; GetParent logic is now in the constructor. Done. https://codereview.chromium.org/382133003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:83: // static On 2014/07/21 03:41:26, tapted wrote: > nit: keep blank line before Done. https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:77: // not needed; the native window of |browser| is used instead. If this is too complicated, we can move the GetParent() logic back out of this constructor - that way, only parent is used to determine the parent window, but Browser is used for registering the observer.
https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.cc:73: // If there are no open browser windows, close the dialog immediately. is this logic that you actually need? ... I ask because you can run into problems calling into a Delegate from the constructor directly if that Delegate is currently being constructed, like, class FancyDialog : public ExtensionUninstallDialog::Delegate { public: FancyDialog(Profile* profile, Browser* browser) : impl_(profile, browser, this) {} private: ExtensionUninstallDialog impl_; }; https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:55: // this parent window. comment still not clear. this method is called "CreateModal" but the comment indicates that it's only *sometimes* modal. is the parent-NULL situation not the same as Create()? should I be thinking of "Create" as "CreateStandalone" because I'm struggling a bit to keep the inheritance structure of this paged in. I'm leaning on comments. https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:77: // not needed; the native window of |browser| is used instead. On 2014/07/22 00:30:43, sasha_b wrote: > If this is too complicated, we can move the GetParent() logic back out of this > constructor - that way, only parent is used to determine the parent window, but > Browser is used for registering the observer. I think it's too complicated, but only because you've phrased it that way. Perhaps say just "Constructor used by derived classes. If |browser| is non-NULL then this dialogue will automatically close when |browser| is destroyed". that decouples it from |parent| and it's less for me to process. https://codereview.chromium.org/382133003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_uninstall_dialog.h:85: Browser* browser_; I don't actually see |browser_| being used anywhere anymore.
Question mainly for kalman, but all feedback welcome: Is it worth taking the super-naive approach here, and just adding 'gfx::NativeWindow parent' as another parameter to Create, changing all the callsites, and leaving all the other code as it is? Maybe the problem is that we're trying to change and refactor this code in one go. To avoid changing the callsites, we could make one small step and have the CreateModal method take the window, but still add the parameter to ExtensionUninstallDialog.
+sky who is doing a similar review and may be interested in this as well Okay, it looks like the dialog is using the observer to account for the special case where the dialog is closed but hasn't been created yet because the image is being loaded. This isn't going to work for non-browser windows anyway (such as the app list, because they don't have observers), and if we're going to use this logic we should really extend it to work for any given parent window. How about the following radical (but necessary) changes: - Remove the Browser parameter from Create altogether, replacing it with a gfx::NativeWindow (the parent window to use) - As suggested in the TODO in extension_uninstall_dialog.cc:106, call Show() immediately (so the dialog is actually created when Create() is called), load the image lazily, and finally remove the notification observer on the browser (and hence remove the need for a Browser object) - Update all callsites to pass in the window, rather than the browser. For those that need the browser window, add a function to c/b/ui/browser_finder.h 'gfx::NativeWindow TryFindActiveWindowForBrowser(Browser *)' which is analogous to GetParent() currently. It'll be a bit of work, but it sounds like there are a lot of bugs associated with this dialog, so might as well fix them all in one go. Also, one more thing - in terms of the 'autoclose' behaviour (the dialog closing when the browser is closed) - as long as the widget is modal to something, it should be closed when it's parent is closed. This is not true for standalone dialogs, but we don't need to automatically close a standalone dialog anyway. The only problem I can see is that on Mac (I believe) no dialogs are modal, so the browser can be closed but the dialog will remain open. This should be OK because the dialog won't actually break in any way (shouldn't segfault or anything, because no memory has been freed and it has no dependencies on the browser window), but it may not be what the user expects. I still think it's OK though, at least for now, but I guess I'll have to test it pretty thoroughly to make sure it's behaving in the way I think it is.
On 2014/07/23 05:14:24, sasha_b wrote: > +sky who is doing a similar review and may be interested in this as well > > Okay, it looks like the dialog is using the observer to account for the special > case where the dialog is closed but hasn't been created yet because the image is > being loaded. This isn't going to work for non-browser windows anyway (such as > the app list, because they don't have observers), and if we're going to use this > logic we should really extend it to work for any given parent window. > > How about the following radical (but necessary) changes: > - Remove the Browser parameter from Create altogether, replacing it with a > gfx::NativeWindow (the parent window to use) > - As suggested in the TODO in extension_uninstall_dialog.cc:106, call Show() > immediately (so the dialog is actually created when Create() is called), load > the image lazily, and finally remove the notification observer on the browser > (and hence remove the need for a Browser object) I like this, but I don't know the history of this code. Why didn't it do as you're suggesting in the first place? > - Update all callsites to pass in the window, rather than the browser. For > those that need the browser window, add a function to c/b/ui/browser_finder.h > 'gfx::NativeWindow TryFindActiveWindowForBrowser(Browser *)' which is analogous > to GetParent() currently. > > It'll be a bit of work, but it sounds like there are a lot of bugs associated > with this dialog, so might as well fix them all in one go. > > Also, one more thing - in terms of the 'autoclose' behaviour (the dialog closing > when the browser is closed) - as long as the widget is modal to something, it > should be closed when it's parent is closed. This is not true for standalone > dialogs, but we don't need to automatically close a standalone dialog anyway. > The only problem I can see is that on Mac (I believe) no dialogs are modal, so > the browser can be closed but the dialog will remain open. This should be OK > because the dialog won't actually break in any way (shouldn't segfault or > anything, because no memory has been freed and it has no dependencies on the > browser window), but it may not be what the user expects. I still think it's OK > though, at least for now, but I guess I'll have to test it pretty thoroughly to > make sure it's behaving in the way I think it is.
Okay, done a proper refactor now, PTAL. ExtensionUninstallDialog now only takes a NativeWindow (and not a Browser) and the app icon is lazily-loaded, so all the browser notification logic has been removed. Updated all callsites that need the parent window to now call a helper utility in browser_finder.cc. This now works perfectly for standalone dialogs and the app list, where different parent windows are passed. https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm (right): https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm:50: profile_, chrome::FindWindowForBrowser(browser), this)); tapted: Seeing as how Mac doesn't even need the parent window, and this is a Mac-only file, is it even worth passing this? I guess that's an implementation abstraction that may change in the future. https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm (right): https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:73: // TODO: Refresh the icon in Cocoa somehow tapted: Is this possible?
Sorry, to answer your question scott - I didn't do it in the first place because A) I wanted this to be a small patch, not having to update all the callsites and B) I didn't fully understand why the Browser object was needed - I thought it was there so that the dialog was closed when the browser closes - but actually it was for a much more subtle reason (safely responding to the icon loading callback if the browser had closed in the meantime, so the dialog had not yet been constructed). The new code is safe against this because it lazily loads the icon, so the dialog is constructed immediately. On Thu, Jul 24, 2014 at 1:51 AM, <sky@chromium.org> wrote: > On 2014/07/23 05:14:24, sasha_b wrote: > >> +sky who is doing a similar review and may be interested in this as well >> > > Okay, it looks like the dialog is using the observer to account for the >> > special > >> case where the dialog is closed but hasn't been created yet because the >> image >> > is > >> being loaded. This isn't going to work for non-browser windows anyway >> (such as >> the app list, because they don't have observers), and if we're going to >> use >> > this > >> logic we should really extend it to work for any given parent window. >> > > How about the following radical (but necessary) changes: >> - Remove the Browser parameter from Create altogether, replacing it >> with a >> gfx::NativeWindow (the parent window to use) >> - As suggested in the TODO in extension_uninstall_dialog.cc:106, call >> Show() >> immediately (so the dialog is actually created when Create() is called), >> load >> the image lazily, and finally remove the notification observer on the >> browser >> (and hence remove the need for a Browser object) >> > > I like this, but I don't know the history of this code. Why didn't it do as > you're suggesting in the first place? > > > > - Update all callsites to pass in the window, rather than the browser. >> For >> those that need the browser window, add a function to >> c/b/ui/browser_finder.h >> 'gfx::NativeWindow TryFindActiveWindowForBrowser(Browser *)' which is >> > analogous > >> to GetParent() currently. >> > > It'll be a bit of work, but it sounds like there are a lot of bugs >> associated >> with this dialog, so might as well fix them all in one go. >> > > Also, one more thing - in terms of the 'autoclose' behaviour (the dialog >> > closing > >> when the browser is closed) - as long as the widget is modal to >> something, it >> should be closed when it's parent is closed. This is not true for >> standalone >> dialogs, but we don't need to automatically close a standalone dialog >> anyway. >> The only problem I can see is that on Mac (I believe) no dialogs are >> modal, so >> the browser can be closed but the dialog will remain open. This should be >> OK >> because the dialog won't actually break in any way (shouldn't segfault or >> anything, because no memory has been freed and it has no dependencies on >> the >> browser window), but it may not be what the user expects. I still think >> it's >> > OK > >> though, at least for now, but I guess I'll have to test it pretty >> thoroughly >> > to > >> make sure it's behaving in the way I think it is. >> > > > > https://codereview.chromium.org/382133003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(don't forget to update the CL description too) https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm (right): https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm:50: profile_, chrome::FindWindowForBrowser(browser), this)); On 2014/07/23 23:57:26, sasha_b wrote: > tapted: Seeing as how Mac doesn't even need the parent window, and this is a > Mac-only file, is it even worth passing this? I guess that's an implementation > abstraction that may change in the future. I'm inclined to keep it, just because it's jumping through so many API boundaries. Also, it could for example try to pick a more correct display to pop up the dialog (NSAlert tries to pick the display "in use" by the user, which is usually right). But hopefully this entire file will disappear in a milestone or three when we have toolkit-views on Mac. https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm (right): https://codereview.chromium.org/382133003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm:73: // TODO: Refresh the icon in Cocoa somehow On 2014/07/23 23:57:26, sasha_b wrote: > tapted: Is this possible? Sadly not :/ - we don't have much control over `NSAlert`, and it's run application modally so no way* to react to the icon loaded callback while it's shown. (This might answer sky's question). Although I'm pretty sure all the _install_ (and permissions) dialogs simply delay showing themselves until the ImageLoader triggers its OnImageLoaded callback as well. But for an install it's much more likely that the icon is not ready: for an _uninstall_ it would be uncommon for an icon to require loading - app launcher would almost certainly have loaded it already, but extensions can also be uninstalled from the toolbar action button via right-click. In this case, a null icon is more probable in this case. *no way = Cocoa has some threading support - there's a chance altering the underlying NSImage in a background thread does the right thing, but there be dragons. Many dragons. https://codereview.chromium.org/382133003/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/382133003/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:121: RefreshIcon(); I think you can just move the `Show()` down here, and remove `RefreshIcon()` altogether. This is consistent with what happens for install/permissions prompts -- see ExtensionInstallPrompt::OnImageLoaded https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/brows... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder.cc:161: gfx::NativeWindow FindWindowForBrowser(Browser* browser) { I don't think this belongs here (it's not really a "find"). I think you can be certain that for the sites currently passing a non-null browser, they are guaranteed to have a non-null browser->window(), so I would just use `browser->window()->GetNativeWindow()` instead of `FindWindowForBrowser(browser)`. https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:55: bool show_in_app_list_; unused?
Removed the BrowserFinder method, and made all callsites get the NativeWindow manually (they should all have an open browser window). Also removed the logic to refresh the icon - now the dialog is just unconstructed, as it was before, until the image is loaded. https://codereview.chromium.org/382133003/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/382133003/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:121: RefreshIcon(); On 2014/07/24 00:35:36, tapted wrote: > I think you can just move the `Show()` down here, and remove `RefreshIcon()` > altogether. This is consistent with what happens for install/permissions prompts > -- see ExtensionInstallPrompt::OnImageLoaded Done. The original reason I did it this way was to prevent problems if OnImageLoaded() runs after ExtensionInstallPrompt is closed. Since we are no longer dependent on the browser, however, this seems to no longer be a problem. https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/brows... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder.cc:161: gfx::NativeWindow FindWindowForBrowser(Browser* browser) { On 2014/07/24 00:35:36, tapted wrote: > I don't think this belongs here (it's not really a "find"). I think you can be > certain that for the sites currently passing a non-null browser, they are > guaranteed to have a non-null browser->window(), so I would just use > `browser->window()->GetNativeWindow()` instead of > `FindWindowForBrowser(browser)`. Done. https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:55: bool show_in_app_list_; On 2014/07/24 00:35:36, tapted wrote: > unused? Done.
lgtm with the following https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/management/management_api.cc (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/management/management_api.cc:615: GetCurrentBrowser()->window()->GetNativeWindow(), So.. I think there's a pre-existing crash lurking here (http://crbug.com/362308 ). I think the best fix is extensions::WindowController* controller = GetExtensionWindowController(); extension_uninstall_dialog_.reset(ExtensionUninstallDialog::Create( GetProfile(), controller ? controller->window()->GetNativeWindow() : NULL, this); https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:142: gfx::ImageSkia* icon) nit: not stuff you're adding, but it's really confusing having `icon` (an ImageSkia), and `icon_` (an ImageView). Perhaps rename the ImageSkia `icon` argument to `image`
sorry - thought of some more possible issues.. perhaps it's worth adding something to extension_install_dialog_view_browsertest https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:73: Profile* const profile_; Hm - I just noticed these protected data members are against the style guide. that's confusing :/. Since this is a refactor, it would be nice to make these private. https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:75: gfx::NativeWindow parent_; it would be nice to get rid of this too (I think there's a lifetime issue) - see later comment. https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:123: CreateBrowserModalDialogViews(view_, parent_)->Show(); Actually .. I think there's still an annoying lifetime issue here. parent_ could be a destroyed NativeWindow at this point :/ You could add a WidgetObserver (if it is non-null)... but that kinda defeats the point of the refactor. I think an alternative might be to create the widget with CreateBrowserModalDialogViews in the constructor. Then override DialogDelegate::OnClosed() in ExtensionUninstallDialogDelegateView, which calls dialog_->ExtensionUninstallCanceled(). Then Show() should be: if (view) view_->GetWidget()->Show(); But if you go this route, I think you will still need that `refresh icon` call. Maybe wait for the DialogDelegate experts to weigh in :) Part of me is even thinking a nicer solution would just be to scrap the uninstall dialog code completely and just make "uninstall" another sub-type of the "install" dialog that doesn't show permissions and has different buttons.
https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/management/management_api.cc (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/management/management_api.cc:615: GetCurrentBrowser()->window()->GetNativeWindow(), On 2014/07/24 07:09:55, tapted wrote: > So.. I think there's a pre-existing crash lurking here (http://crbug.com/362308 > ). > > I think the best fix is > > extensions::WindowController* controller = GetExtensionWindowController(); > extension_uninstall_dialog_.reset(ExtensionUninstallDialog::Create( > GetProfile(), > controller ? controller->window()->GetNativeWindow() : NULL, > this); Nice! Done. kalman: Is this the crash you were talking about before? https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:142: gfx::ImageSkia* icon) On 2014/07/24 07:09:55, tapted wrote: > nit: not stuff you're adding, but it's really confusing having `icon` (an > ImageSkia), and `icon_` (an ImageView). Perhaps rename the ImageSkia `icon` > argument to `image` Np, agreed, this was confusing me earlier. Done.
lgtm but you should get somebody familiar with the views code to sanity check :) https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:48: ExtensionUninstallDialogDelegateView* view() { return view_; } where is this used?
Nice work! lgtm with a comment nit and with the removal of some extraneous includes. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:11: #include "chrome/browser/chrome_notification_types.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:14: #include "chrome/browser/ui/browser.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:15: #include "content/public/browser/notification_service.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:16: #include "content/public/browser/notification_source.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:10: #include "base/memory/scoped_ptr.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:12: #include "content/public/browser/notification_registrar.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:16: class Browser; Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:46: // |profile| and |delegate| can never be NULL. If |parent| is not NULL, makes Consider: "The dialog will be modal to |parent|, or a non-modal standalone dialog if |parent| is NULL." I may not even bother with "standalone" since non-modal is probably a better way of conveying a lack of modality. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:103: content::NotificationRegistrar registrar_; Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:12: #include "chrome/browser/ui/browser.h" Remove this. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:13: #include "chrome/browser/ui/browser_window.h" Remove this.
Thank you all for your input :) tapted: Please take a look at the feedback replies. I have opened bugs for the issues you mentioned. Also let me know if the commit message is OK. https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:73: Profile* const profile_; On 2014/07/24 07:46:33, tapted wrote: > Hm - I just noticed these protected data members are against the style guide. > that's confusing :/. Since this is a refactor, it would be nice to make these > private. Opened crbug.com/397395 to track this. Simple fix but I'd rather do it in a separate CL. https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:123: CreateBrowserModalDialogViews(view_, parent_)->Show(); Opened crbug.com/397396 to track this and added a todo. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:11: #include "chrome/browser/chrome_notification_types.h" On 2014/07/24 19:20:56, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:14: #include "chrome/browser/ui/browser.h" On 2014/07/24 19:20:56, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:15: #include "content/public/browser/notification_service.h" On 2014/07/24 19:20:56, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:16: #include "content/public/browser/notification_source.h" On 2014/07/24 19:20:56, msw wrote: > Remove this. Done. Thanks for that :) https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:10: #include "base/memory/scoped_ptr.h" On 2014/07/24 19:20:57, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:12: #include "content/public/browser/notification_registrar.h" On 2014/07/24 19:20:57, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:16: class Browser; On 2014/07/24 19:20:57, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:46: // |profile| and |delegate| can never be NULL. If |parent| is not NULL, makes On 2014/07/24 19:20:57, msw wrote: > Consider: "The dialog will be modal to |parent|, or a non-modal standalone > dialog if |parent| is NULL." I may not even bother with "standalone" since > non-modal is probably a better way of conveying a lack of modality. Thanks; agreed & removed the standalone bit. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:103: content::NotificationRegistrar registrar_; On 2014/07/24 19:20:57, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc (right): https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:12: #include "chrome/browser/ui/browser.h" On 2014/07/24 19:20:57, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:13: #include "chrome/browser/ui/browser_window.h" On 2014/07/24 19:20:57, msw wrote: > Remove this. Done. https://codereview.chromium.org/382133003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:48: ExtensionUninstallDialogDelegateView* view() { return view_; } On 2014/07/24 17:16:24, kalman wrote: > where is this used? It's not :) Removed.
slgtm you can add 362308 to the CL description's BUG= to track in there too the outstanding lifetime issue might just be hypothetical - as discussed it looks like the install dialog would suffer from the same problem, since it uses the same construction flow. (Although there are obscure bugs like http://crbug.com/279632 for the install prompt too...). In any case, something better to explore in a follow-up.
The CQ bit was checked by sashab@chromium.org
The CQ bit was unchecked by sashab@chromium.org
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/382133003/180001
Message was sent while issue was closed.
Change committed as 285541 |
