Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2172363002:
Created min size for print preview dialog and modified to allow the
DescriptionCreated min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal.
Currently ChromeOS only.
Patch Set 1 #Patch Set 2 : Changed window bounds checking to include possible out-of-bounds children. #Patch Set 3 : reduced diff #
Total comments: 15
Patch Set 4 : Fixed comments. Made ChromeOS only. Separated top level and bounds logic. #Patch Set 5 : Track independent bounds as native window property not member of widget class #
Total comments: 8
Patch Set 6 : Track unclipped windows in modal dialog manager #Patch Set 7 : Change to CrOS only. #
Total comments: 35
Patch Set 8 : Various fixes #
Total comments: 14
Patch Set 9 : Edit deps to fix compile error. #Patch Set 10 : Fix ifdef logic, return deps to normal #
Total comments: 10
Patch Set 11 : Fix variable names and comments. #Patch Set 12 : Fix build and compiler errors #Patch Set 13 : Add check for independent bounds key in targeter #Patch Set 14 : Fix targeting and missing widget on unit tests. #
Total comments: 29
Patch Set 15 : Fix comments and deal with merge conflict. #Patch Set 16 : Fix Mac compile error. #
Total comments: 27
Patch Set 17 : Fix comments #Patch Set 18 : Modify tracking of nonclipped state for new code structure. #
Total comments: 4
Patch Set 19 : Fix nits. #Patch Set 20 : Initial version with top level window. #Patch Set 21 : Switched to make new dialogs window modal. #Patch Set 22 : Top level window modal windows w/child modal behavior #Patch Set 23 : Fix address bar bug + compile error. #Patch Set 24 : Fix bad window modal return from constrained web dialogs. #Patch Set 25 : Fixes to print preview tests. #Patch Set 26 : Fix mac build break. #Patch Set 27 : Fix mac build break. #Patch Set 28 : fix mac build error #
Total comments: 3
Patch Set 29 : Windows modifications #Patch Set 30 : Merge in Windows changes #Patch Set 31 : Fix test errors #Patch Set 32 : Fix test memory leak #Patch Set 33 : fix mac #Patch Set 34 : Remove activation change observation #Patch Set 35 : Try to fix build again #Patch Set 36 : Fix Mac build error #Patch Set 37 : Fix unused variable for mac #Patch Set 38 : Merge ConstrainedWebDialog functions #Messages
Total messages: 201 (167 generated)
rbpotter@chromium.org changed reviewers: + sadrul@chromium.org, tapted@chromium.org, thestig@chromium.org, wittman@chromium.org
Current set of changes. Have made some updates to reduce the diff size. See comments for a few questions. Also, this has one additional somewhat odd behavior: It allows the user to resize the parent window through the child window if the child is sticking out of the parent window. Not sure if the user should just not be able to resize the window in the direction of the dialog in that case or if this is okay (all the buttons/controls on the dialog seem to still work). https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:287: #if !defined(OS_MACOSX) // May want to make this Chrome OS only rather In what cases should the new set of code (allowing the dialog to stick out) run? This flag is what enables the dialog size to not be tied to the parent window. https://codereview.chromium.org/2172363002/diff/80001/ui/aura/window_targeter.cc File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2172363002/diff/80001/ui/aura/window_targeter... ui/aura/window_targeter.cc:137: EventLocationInsideBoundsRecursive(window, event.location(), Should a flag be added to the aura::Window class to ensure this only happens on windows that have flagged that they may have a child that is not fully contained by their bounds?
Patchset #4 (id:60001) has been deleted
I defer to the UI experts. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:116: int expandWidth = 2*kBorder; Take a look at the Google C++ style guide. non-const variables are named_like_this, and there should be spaces around '*'. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:118: if (size->width() - kMinDialogSize.width() < 2*kBorder) { This can be combined into: int expand_width = std::min(size->width() - kMinDialogSize.width(), 2 * kBorder); https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:124: size->Enlarge(-1*expandWidth, -1*expandHeight); I think you can just write -var instead of -1 * var. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:104: ConstrainedWebDialogDelegate* ShowTopLevelConstrainedWebDialog( Please add a comment similiar to the functions above/below to explain what it does. All 3 can probably use comments saying "The returned object deletes itself when the dialog closes."
Have you tested on Windows? I don't think this has the intended effect on that platform. Patching in to my checkout, the dialog is still clipped to the parent window and most mouse and keyboard events are not being processed with the dialog displayed. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:368: new PrintPreviewDialogDelegate(initiator), nit: update indendation https://codereview.chromium.org/2172363002/diff/80001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:100: if (widget->IsModalTypeChild()) { nit: if else https://codereview.chromium.org/2172363002/diff/80001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/2172363002/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.h:782: bool IsModalTypeChild() const override; This function is only used in one place, so I don't think expanding the Widget interface is justified. Better to just check the delegate modal type directly in UpdateModalDialogPosition.
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
Fixed comments and fixed a bug due to using is_top_level as the indicator for the widget independent bounds - now the bounds being constrained to the parent is fully independent of the widget being top level. Changed the min size, no scaling behavior to be Chrome OS only as in Linux bringing up things like the destination search box can cause the user to get "stuck" as the scroll bar and cancel button get cut off. Still need to test on Windows to see if things look better there or if the window targeter changes also need to be allowed to run only for this case (could add a flag in window.h). https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:116: int expandWidth = 2*kBorder; On 2016/07/25 21:35:10, Lei Zhang wrote: > Take a look at the Google C++ style guide. non-const variables are > named_like_this, and there should be spaces around '*'. Done. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:118: if (size->width() - kMinDialogSize.width() < 2*kBorder) { On 2016/07/25 21:35:10, Lei Zhang wrote: > This can be combined into: > > int expand_width = std::min(size->width() - kMinDialogSize.width(), 2 * > kBorder); Done. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:124: size->Enlarge(-1*expandWidth, -1*expandHeight); On 2016/07/25 21:35:10, Lei Zhang wrote: > I think you can just write -var instead of -1 * var. Done. https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:104: ConstrainedWebDialogDelegate* ShowTopLevelConstrainedWebDialog( On 2016/07/25 21:35:10, Lei Zhang wrote: > Please add a comment similiar to the functions above/below to explain what it > does. All 3 can probably use comments saying "The returned object deletes itself > when the dialog closes." Done. https://codereview.chromium.org/2172363002/diff/80001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:100: if (widget->IsModalTypeChild()) { On 2016/07/26 17:27:35, Mike Wittman wrote: > nit: if else Acknowledged. https://codereview.chromium.org/2172363002/diff/80001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/2172363002/diff/80001/ui/views/widget/widget.... ui/views/widget/widget.h:782: bool IsModalTypeChild() const override; On 2016/07/26 17:27:35, Mike Wittman wrote: > This function is only used in one place, so I don't think expanding the Widget > interface is justified. Better to just check the delegate modal type directly in > UpdateModalDialogPosition. Done.
On 2016/07/26 21:48:19, rbpotter wrote: > Fixed comments and fixed a bug due to using is_top_level as the indicator for > the widget independent bounds - now the bounds being constrained to the parent > is fully independent of the widget being top level. Changed the min size, no > scaling behavior to be Chrome OS only as in Linux bringing up things like the > destination search box can cause the user to get "stuck" as the scroll bar and > cancel button get cut off. Still need to test on Windows to see if things look > better there or if the window targeter changes also need to be allowed to run > only for this case (could add a flag in window.h). > > https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... > File chrome/browser/printing/print_preview_dialog_controller.cc (right): > > https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... > chrome/browser/printing/print_preview_dialog_controller.cc:116: int expandWidth > = 2*kBorder; > On 2016/07/25 21:35:10, Lei Zhang wrote: > > Take a look at the Google C++ style guide. non-const variables are > > named_like_this, and there should be spaces around '*'. > > Done. > > https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... > chrome/browser/printing/print_preview_dialog_controller.cc:118: if > (size->width() - kMinDialogSize.width() < 2*kBorder) { > On 2016/07/25 21:35:10, Lei Zhang wrote: > > This can be combined into: > > > > int expand_width = std::min(size->width() - kMinDialogSize.width(), 2 * > > kBorder); > > Done. > > https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/printing... > chrome/browser/printing/print_preview_dialog_controller.cc:124: > size->Enlarge(-1*expandWidth, -1*expandHeight); > On 2016/07/25 21:35:10, Lei Zhang wrote: > > I think you can just write -var instead of -1 * var. > > Done. > > https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): > > https://codereview.chromium.org/2172363002/diff/80001/chrome/browser/ui/webui... > chrome/browser/ui/webui/constrained_web_dialog_ui.h:104: > ConstrainedWebDialogDelegate* ShowTopLevelConstrainedWebDialog( > On 2016/07/25 21:35:10, Lei Zhang wrote: > > Please add a comment similiar to the functions above/below to explain what it > > does. All 3 can probably use comments saying "The returned object deletes > itself > > when the dialog closes." > > Done. > > https://codereview.chromium.org/2172363002/diff/80001/components/constrained_... > File components/constrained_window/constrained_window_views.cc (right): > > https://codereview.chromium.org/2172363002/diff/80001/components/constrained_... > components/constrained_window/constrained_window_views.cc:100: if > (widget->IsModalTypeChild()) { > On 2016/07/26 17:27:35, Mike Wittman wrote: > > nit: if else > > Acknowledged. > > https://codereview.chromium.org/2172363002/diff/80001/ui/views/widget/widget.h > File ui/views/widget/widget.h (right): > > https://codereview.chromium.org/2172363002/diff/80001/ui/views/widget/widget.... > ui/views/widget/widget.h:782: bool IsModalTypeChild() const override; > On 2016/07/26 17:27:35, Mike Wittman wrote: > > This function is only used in one place, so I don't think expanding the Widget > > interface is justified. Better to just check the delegate modal type directly > in > > UpdateModalDialogPosition. > > Done. Tested on Windows briefly - mouse click/keyboard issues seem to be resolved now that the dialog is confined to the parent box. Can still add flag to window class to prevent the recursive search for children happening for all windows; please let me know if this is what I should do. It would prevent any new code from running any time other than print previews on ChromeOS.
A general comment: the form of this approach seems reasonable, but the Views changes feel like they're at the wrong level in the code. It doesn't make sense to me to add interface functions to Widget/WidgetDelegate that are only supported on one platform. I'm not sure where the right place is, unfortunately, but I suggest getting feedback from someone like sky@ who has good knowledge of both Views and Aura/Ash.
Description was changed from ========== Created min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal. Fixed earlier issue with mouse events outside parent window not working by removing reparenting step in web modal dialog manager. However now have problems with crashing on focus shift. BUG=622940 ========== to ========== Created min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal. Fixed earlier issue with mouse events outside parent window not working by removing reparenting step in web modal dialog manager. However now have problems with crashing on focus shift. BUG=622940 ==========
rbpotter@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== Created min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal. Fixed earlier issue with mouse events outside parent window not working by removing reparenting step in web modal dialog manager. However now have problems with crashing on focus shift. BUG=622940 ========== to ========== Created min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal. Required modifications to widget to add a flag indicating the bounds could be extended and the the window targeter logic to search for child windows outside the parent for located events. ==========
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #3 (id:40001) has been deleted
Modified so that the independent bounds parameter is a native window property and not a member of the widget or widget delegate class.
https://codereview.chromium.org/2172363002/diff/240001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/240001/components/constrained... components/constrained_window/constrained_window_views.cc:26: const char kHasIndependentBoundsKey[] = "__INDEPENDENT_BOUNDS__"; If I understood Scott correctly in the off-review discussion, this should go in wm. If there's a DEPS violation accessing it here, we can update the constrained_window component's DEPS. https://codereview.chromium.org/2172363002/diff/240001/components/constrained... components/constrained_window/constrained_window_views.cc:103: independent_bounds = reinterpret_cast<bool*>( static_cast<> is preferred when casting from void* to typed pointers, since it's a stricter cast. https://codereview.chromium.org/2172363002/diff/240001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/240001/components/constrained... components/constrained_window/constrained_window_views.h:51: views::Widget* ShowWebModalDialogWithIndBoundsViews( abbreviations in identifier names should be avoided, even if it makes the names long (per the style guide) https://codereview.chromium.org/2172363002/diff/240001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/240001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:41: const char * kHasIndependentBoundsKey); I think you can avoid all this threading of independent bounds state with the following: - add a new function here, like ShowNonClippedModalDialog - keep track of which gfx::NativeWindows want non-clipped display within WebContentsModalDialogManager - add a IsNonClipped(gfx::NativeWindow) function on WebContentsModalDialogManagerDelegate and implement it here - in NativeWebContentsModalDialogManagerViews::ManageDialog: if (native_delegate_->IsNonClipped(dialog())) child->SetNativeWindowProperty(...);
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:340001) has been deleted
rbpotter@chromium.org changed reviewers: - sadrul@chromium.org, sky@chromium.org, tapted@chromium.org
Description was changed from ========== Created min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal. Required modifications to widget to add a flag indicating the bounds could be extended and the the window targeter logic to search for child windows outside the parent for located events. ========== to ========== Created min size for print preview dialog and modified to allow the constrained web dialog to be larger than the parent window while still remaining child modal. Currently ChromeOS only. ==========
https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:305: #if !defined(OS_CHROMEOS) When testing on Windows, dialogs still get cut off at the edge of parent windows. Not sure why this is occurring but do not currently have a Windows machine to debug with - will try to return to this and get it working on windows once I have one. In the mean time, enabling for chrome OS only.
https://codereview.chromium.org/2172363002/diff/240001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/240001/components/constrained... components/constrained_window/constrained_window_views.cc:26: const char kHasIndependentBoundsKey[] = "__INDEPENDENT_BOUNDS__"; On 2016/08/05 16:03:46, Mike Wittman wrote: > If I understood Scott correctly in the off-review discussion, this should go in > wm. If there's a DEPS violation accessing it here, we can update the > constrained_window component's DEPS. Done. https://codereview.chromium.org/2172363002/diff/240001/components/constrained... components/constrained_window/constrained_window_views.cc:103: independent_bounds = reinterpret_cast<bool*>( On 2016/08/05 16:03:46, Mike Wittman wrote: > static_cast<> is preferred when casting from void* to typed pointers, since it's > a stricter cast. Done. https://codereview.chromium.org/2172363002/diff/240001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/240001/components/constrained... components/constrained_window/constrained_window_views.h:51: views::Widget* ShowWebModalDialogWithIndBoundsViews( On 2016/08/05 16:03:46, Mike Wittman wrote: > abbreviations in identifier names should be avoided, even if it makes the names > long (per the style guide) Done. https://codereview.chromium.org/2172363002/diff/240001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/240001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:41: const char * kHasIndependentBoundsKey); On 2016/08/05 16:03:46, Mike Wittman wrote: > I think you can avoid all this threading of independent bounds state with the > following: > > - add a new function here, like ShowNonClippedModalDialog > - keep track of which gfx::NativeWindows want non-clipped display within > WebContentsModalDialogManager > - add a IsNonClipped(gfx::NativeWindow) function on > WebContentsModalDialogManagerDelegate and implement it here > - in NativeWebContentsModalDialogManagerViews::ManageDialog: > if (native_delegate_->IsNonClipped(dialog())) > child->SetNativeWindowProperty(...); Done.
https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:100: // Modified to match requested size. Previously: 800, 480. This comment likely can be removed. Anyone who wants to understand the history of kMinDialogSize can look at the file's git history. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:101: const gfx::Size kMinDialogSize(438, 455); Should this be sized differently depending on whether wm::kHasIndependentBoundsKey is supported? https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:116: int expand_width = Same question here. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:370: #if defined(OS_MACOSX) Can we drop the #if part here and just have ShowTopLevelConstrainedWebDialog invoke ShowConstrainedWebDialog within its implementation on Mac? https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:304: // Eventually, should try to enable this for other Windows. ... other *platforms*? https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.cc:81: if (native_delegate_->IsNonClipped(dialog())) { We should only enable this code on the platforms where wm::kHasIndependentBoundsKey is supported. Setting the property in cases where it has no effect will be confusing to anyone looking at the native window properties while debugging. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.cc:83: new bool(true)); For ease of use and to avoid leaking memory, I would encode the truth value in the property value itself: reinterpret_cast<void*>(true). Then users can directly check the property value: if (widget->GetNativeWindowProperty(wm::kHasIndependentBoundsKey)) { // ... } https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/webu... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/webu... chrome/browser/ui/webui/constrained_web_dialog_ui.h:115: // |overshadowed| is the tab being overshadowed by the dialog. This parameter does not exist. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/webu... chrome/browser/ui/webui/constrained_web_dialog_ui.h:116: ConstrainedWebDialogDelegate* ShowTopLevelConstrainedWebDialog( I would call this ShowNonClippedConstrainedWebDialog. To me a top-level dialog is one with a null parent. https://codereview.chromium.org/2172363002/diff/380001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/components/constrained... components/constrained_window/constrained_window_views.cc:113: if (bound.width() - border->GetInsets().left() < size.width()) { comment what this logic is doing https://codereview.chromium.org/2172363002/diff/380001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/380001/components/constrained... components/constrained_window/constrained_window_views.h:52: // platforms that do not support unclipped dialogs, has the same behavior as Mention which platforms support non-clipped dialogs in the comment. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/s... File components/web_modal/single_web_contents_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/s... components/web_modal/single_web_contents_dialog_manager.h:32: // Check if the dialog should have a non clipped display (bounds not Comments should be descriptive, rather than imperative ("checks" rather than "check"). The above comment also violates this. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/s... components/web_modal/single_web_contents_dialog_manager.h:34: virtual bool IsNonClipped(gfx::NativeWindow dialog) = 0; This function probably should be const. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:82: void WebContentsModalDialogManager::WillClose(gfx::NativeWindow dialog) { The dialog should be removed from nonclipped_child_dialogs_ here. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:95: typedef std::deque<gfx::NativeWindow> NativeWindowDialogList; std::set would be better given how this is used. Also unless you have a specific reason for it, this shouldn't have a typedef. The existing typedef is a legacy of pre-auto C++. https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... File ui/wm/core/window_modality_controller.cc (right): https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... ui/wm/core/window_modality_controller.cc:25: // Key for native window property that indicates if the window should have No need to repeat the comment in the implementation. https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... File ui/wm/core/window_modality_controller.h (right): https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... ui/wm/core/window_modality_controller.h:24: // bounds independent of its modal parent. Mention which platforms support this in the comment.
Patchset #8 (id:400001) has been deleted
https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:100: // Modified to match requested size. Previously: 800, 480. On 2016/08/12 22:51:34, Mike Wittman wrote: > This comment likely can be removed. Anyone who wants to understand the history > of kMinDialogSize can look at the file's git history. Done. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:101: const gfx::Size kMinDialogSize(438, 455); On 2016/08/12 22:51:34, Mike Wittman wrote: > Should this be sized differently depending on whether > wm::kHasIndependentBoundsKey is supported? If there's not independent bounds key, the dialog ignores its minimum size entirely and just sizes down to the size of the parent window. The only platform that could be affected is Mac, so I am adding a couple Mac only lines to ensure that on Mac the behavior is the same as before. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:116: int expand_width = On 2016/08/12 22:51:34, Mike Wittman wrote: > Same question here. See above https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:370: #if defined(OS_MACOSX) On 2016/08/12 22:51:34, Mike Wittman wrote: > Can we drop the #if part here and just have ShowTopLevelConstrainedWebDialog > invoke ShowConstrainedWebDialog within its implementation on Mac? Done. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:304: // Eventually, should try to enable this for other Windows. On 2016/08/12 22:51:34, Mike Wittman wrote: > ... other *platforms*? Done. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.cc:81: if (native_delegate_->IsNonClipped(dialog())) { On 2016/08/12 22:51:34, Mike Wittman wrote: > We should only enable this code on the platforms where > wm::kHasIndependentBoundsKey is supported. Setting the property in cases where > it has no effect will be confusing to anyone looking at the native window > properties while debugging. Done. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.cc:83: new bool(true)); On 2016/08/12 22:51:34, Mike Wittman wrote: > For ease of use and to avoid leaking memory, I would encode the truth value in > the property value itself: reinterpret_cast<void*>(true). > > Then users can directly check the property value: > if (widget->GetNativeWindowProperty(wm::kHasIndependentBoundsKey)) { > // ... > } Done. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/webu... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/webu... chrome/browser/ui/webui/constrained_web_dialog_ui.h:115: // |overshadowed| is the tab being overshadowed by the dialog. On 2016/08/12 22:51:34, Mike Wittman wrote: > This parameter does not exist. Acknowledged. https://codereview.chromium.org/2172363002/diff/380001/chrome/browser/ui/webu... chrome/browser/ui/webui/constrained_web_dialog_ui.h:116: ConstrainedWebDialogDelegate* ShowTopLevelConstrainedWebDialog( On 2016/08/12 22:51:34, Mike Wittman wrote: > I would call this ShowNonClippedConstrainedWebDialog. To me a top-level dialog > is one with a null parent. Done. https://codereview.chromium.org/2172363002/diff/380001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/380001/components/constrained... components/constrained_window/constrained_window_views.cc:113: if (bound.width() - border->GetInsets().left() < size.width()) { On 2016/08/12 22:51:34, Mike Wittman wrote: > comment what this logic is doing Done. https://codereview.chromium.org/2172363002/diff/380001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/380001/components/constrained... components/constrained_window/constrained_window_views.h:52: // platforms that do not support unclipped dialogs, has the same behavior as On 2016/08/12 22:51:34, Mike Wittman wrote: > Mention which platforms support non-clipped dialogs in the comment. Done. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/s... File components/web_modal/single_web_contents_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/s... components/web_modal/single_web_contents_dialog_manager.h:32: // Check if the dialog should have a non clipped display (bounds not On 2016/08/12 22:51:34, Mike Wittman wrote: > Comments should be descriptive, rather than imperative ("checks" rather than > "check"). The above comment also violates this. Done. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/s... components/web_modal/single_web_contents_dialog_manager.h:34: virtual bool IsNonClipped(gfx::NativeWindow dialog) = 0; On 2016/08/12 22:51:34, Mike Wittman wrote: > This function probably should be const. Done. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:82: void WebContentsModalDialogManager::WillClose(gfx::NativeWindow dialog) { On 2016/08/12 22:51:34, Mike Wittman wrote: > The dialog should be removed from nonclipped_child_dialogs_ here. Done. https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/380001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:95: typedef std::deque<gfx::NativeWindow> NativeWindowDialogList; On 2016/08/12 22:51:34, Mike Wittman wrote: > std::set would be better given how this is used. Also unless you have a specific > reason for it, this shouldn't have a typedef. The existing typedef is a legacy > of pre-auto C++. Done. https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... File ui/wm/core/window_modality_controller.cc (right): https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... ui/wm/core/window_modality_controller.cc:25: // Key for native window property that indicates if the window should have On 2016/08/12 22:51:35, Mike Wittman wrote: > No need to repeat the comment in the implementation. Done. https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... File ui/wm/core/window_modality_controller.h (right): https://codereview.chromium.org/2172363002/diff/380001/ui/wm/core/window_moda... ui/wm/core/window_modality_controller.h:24: // bounds independent of its modal parent. On 2016/08/12 22:51:35, Mike Wittman wrote: > Mention which platforms support this in the comment. Done.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:102: // keeps Mac behavior the same as before new unclipped dialogs Can you provide more context here on the differences between Mac and non-Mac behavior, to justify the different setting? Also, comments should be a full sentence, capitalized with proper punctuation (same thing below). https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:104: kMinDialogSize.set_width(800); I don't think this will compile on Mac. Better would be: #if defined(OS_MACOSX) const gfx::Size kMinDialogSize(800, 480); #else const gfx::Size kMinDialogSize(438, 455); #endif That said, I'm not qualified to judge the content of the Mac-specific settings here... I'll leave that to thestig@. https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:125: expand_width = 2*kBorder; #ifdef's are much easier to understand when the platform-specific logic is cleanly factored into each branch. e.g. #if defined(OS_MACOSX) size->Enlarge(-2*kBorder, -kBorder); #else int expand_width = std::min(size->width() - kMinDialogSize.width(), 2 * kBorder); int expand_height = std::min(size->height() - kMinDialogSize.height(), kBorder); size->Enlarge(-expand_width, -expand_height); #endif https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:305: #if !defined(OS_CHROMEOS) The #if !defined(OS_CHROMEOS) section can be removed now that the platform check is in NativeWebContentsModalDialogManagerViews, correct? https://codereview.chromium.org/2172363002/diff/420001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/420001/components/constrained... components/constrained_window/constrained_window_views.cc:104: void* independent_bounds = NULL; Enclosing all this in the preprocessor conditional is a little cleaner: #if defined(USE_AURA) if (widget->GetNativeWindowProperty(wm::kHasIndependentBoundsKey)) { // ... } #endif https://codereview.chromium.org/2172363002/diff/420001/components/constrained... components/constrained_window/constrained_window_views.cc:149: void* independent_bounds = NULL; This variable is clearer as a bool, and set separately in the Aura and non-Aura cases: #if defined(USE_AURA) bool has_independent_bounds = widget->GetNativeWindowProperty(wm::kHasIndependentBoundsKey) #else bool has_independent_bounds = false; #endif https://codereview.chromium.org/2172363002/diff/420001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/2172363002/diff/420001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:104: if (nonclipped_child_dialogs_.find(dialog) != return nonclipped_child_dialogs_.find(dialog) != nonclipped_child_dialogs_.end();
Patchset #11 (id:480001) has been deleted
Patchset #10 (id:460001) has been deleted
Fixed last set of comments. Also, despite fixing the DEPS file (which allowed
the CL to upload) I am still having issues with the #include from
wm/core/window_modality_controller.h - the bots are failing with the following
error:
ERROR at //components/constrained_window/constrained_window_views.cc:22:11:
Can't include this header from here.
#include "ui/wm/core/window_modality_controller.h"
^--------------------------------------
The target:
//components/constrained_window:constrained_window
is including a file from the target:
//ui/wm:wm
It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.
Dependency chain (there may also be others):
//components/constrained_window:constrained_window -->
//ui/views:views --[private]-->
//ui/wm:wm
GN gen failed: 1
I tried switching the DEPS from ui/wm/core to just ui/wm, which failed, and I
checked the DEPS file in ui/views - it already includes ui/wm/core and
ui/wm/public, so I'm not sure what is going on. Any suggestions?
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin...
File chrome/browser/printing/print_preview_dialog_controller.cc (right):
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin...
chrome/browser/printing/print_preview_dialog_controller.cc:102: // keeps Mac
behavior the same as before new unclipped dialogs
On 2016/08/15 19:36:37, Mike Wittman wrote:
> Can you provide more context here on the differences between Mac and non-Mac
> behavior, to justify the different setting? Also, comments should be a full
> sentence, capitalized with proper punctuation (same thing below).
Done.
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin...
chrome/browser/printing/print_preview_dialog_controller.cc:104:
kMinDialogSize.set_width(800);
On 2016/08/15 19:36:37, Mike Wittman wrote:
> I don't think this will compile on Mac. Better would be:
>
> #if defined(OS_MACOSX)
> const gfx::Size kMinDialogSize(800, 480);
> #else
> const gfx::Size kMinDialogSize(438, 455);
> #endif
>
> That said, I'm not qualified to judge the content of the Mac-specific settings
> here... I'll leave that to thestig@.
Done.
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/printin...
chrome/browser/printing/print_preview_dialog_controller.cc:125: expand_width =
2*kBorder;
On 2016/08/15 19:36:37, Mike Wittman wrote:
> #ifdef's are much easier to understand when the platform-specific logic is
> cleanly factored into each branch. e.g.
>
> #if defined(OS_MACOSX)
> size->Enlarge(-2*kBorder, -kBorder);
> #else
> int expand_width =
> std::min(size->width() - kMinDialogSize.width(), 2 * kBorder);
> int expand_height =
> std::min(size->height() - kMinDialogSize.height(), kBorder);
> size->Enlarge(-expand_width, -expand_height);
> #endif
Done.
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/ui/view...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):
https://codereview.chromium.org/2172363002/diff/420001/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:305: #if
!defined(OS_CHROMEOS)
On 2016/08/15 19:36:37, Mike Wittman wrote:
> The #if !defined(OS_CHROMEOS) section can be removed now that the platform
check
> is in NativeWebContentsModalDialogManagerViews, correct?
Done. Thought I still needed this as the dialogs will be added to the nonclipped
list if the next couple lines get called, but it doesn't affect the sizing logic
as long as the property isn't set.
https://codereview.chromium.org/2172363002/diff/420001/components/constrained...
File components/constrained_window/constrained_window_views.cc (right):
https://codereview.chromium.org/2172363002/diff/420001/components/constrained...
components/constrained_window/constrained_window_views.cc:104: void*
independent_bounds = NULL;
On 2016/08/15 19:36:37, Mike Wittman wrote:
> Enclosing all this in the preprocessor conditional is a little cleaner:
>
> #if defined(USE_AURA)
> if (widget->GetNativeWindowProperty(wm::kHasIndependentBoundsKey)) {
> // ...
> }
> #endif
Done.
https://codereview.chromium.org/2172363002/diff/420001/components/constrained...
components/constrained_window/constrained_window_views.cc:149: void*
independent_bounds = NULL;
On 2016/08/15 19:36:37, Mike Wittman wrote:
> This variable is clearer as a bool, and set separately in the Aura and
non-Aura
> cases:
>
> #if defined(USE_AURA)
> bool has_independent_bounds =
> widget->GetNativeWindowProperty(wm::kHasIndependentBoundsKey)
> #else
> bool has_independent_bounds = false;
> #endif
Done.
https://codereview.chromium.org/2172363002/diff/420001/components/web_modal/w...
File components/web_modal/web_contents_modal_dialog_manager.cc (right):
https://codereview.chromium.org/2172363002/diff/420001/components/web_modal/w...
components/web_modal/web_contents_modal_dialog_manager.cc:104: if
(nonclipped_child_dialogs_.find(dialog) !=
On 2016/08/15 19:36:37, Mike Wittman wrote:
> return nonclipped_child_dialogs_.find(dialog) !=
> nonclipped_child_dialogs_.end();
Done.
On 2016/08/15 21:32:54, rbpotter wrote: > Fixed last set of comments. Also, despite fixing the DEPS file (which allowed > the CL to upload) I am still having issues with the #include from > wm/core/window_modality_controller.h - the bots are failing with the following > error: > > ERROR at //components/constrained_window/constrained_window_views.cc:22:11: > Can't include this header from here. > #include "ui/wm/core/window_modality_controller.h" > ^-------------------------------------- > The target: > //components/constrained_window:constrained_window > is including a file from the target: > //ui/wm:wm > It's usually best to depend directly on the destination target. > In some cases, the destination target is considered a subcomponent > of an intermediate target. In this case, the intermediate target > should depend publicly on the destination to forward the ability > to include headers. > Dependency chain (there may also be others): > //components/constrained_window:constrained_window --> > //ui/views:views --[private]--> > //ui/wm:wm > GN gen failed: 1 > > I tried switching the DEPS from ui/wm/core to just ui/wm, which failed, and I > checked the DEPS file in ui/views - it already includes ui/wm/core and > ui/wm/public, so I'm not sure what is going on. Any suggestions? This is complaining about the BUILD.gn rules, rather than the contents of the DEPS file. I believe the right solution is adding a deps on //ui/wm for constrained_window. https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right): https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:252: // Deleted when the dialog closes. This comment is defunct and can be removed. https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:152: bool independent_bounds) is_nonclipped, to be consistent with the terminology used in the layers immediately below this (same change elsewhere in this file as well) https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:248: if (GetIndependentBounds()) { the member variable can be used directly since it's internal to the class; no need to define an accessor https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:279: bool independent_bounds_; const https://codereview.chromium.org/2172363002/diff/500001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/500001/components/constrained... components/constrained_window/constrained_window_views.cc:106: // horizontal position cetnered on the center of the parent window. centered
https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right): https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:252: // Deleted when the dialog closes. On 2016/08/16 01:07:07, Mike Wittman wrote: > This comment is defunct and can be removed. Done. https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:152: bool independent_bounds) On 2016/08/16 01:07:07, Mike Wittman wrote: > is_nonclipped, to be consistent with the terminology used in the layers > immediately below this (same change elsewhere in this file as well) Done. Should the naming on the native windows (kHasIndependentBoundsKey and the variables in constrained_window_views.cc) also be changed? https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:248: if (GetIndependentBounds()) { On 2016/08/16 01:07:07, Mike Wittman wrote: > the member variable can be used directly since it's internal to the class; no > need to define an accessor Done. https://codereview.chromium.org/2172363002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:279: bool independent_bounds_; On 2016/08/16 01:07:07, Mike Wittman wrote: > const Done. https://codereview.chromium.org/2172363002/diff/500001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/500001/components/constrained... components/constrained_window/constrained_window_views.cc:106: // horizontal position cetnered on the center of the parent window. On 2016/08/16 01:07:08, Mike Wittman wrote: > centered Done.
These changes look good. I think the build fix is the only outstanding issue.
Patchset #12 (id:540001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #12 (id:560001) has been deleted
Patchset #12 (id:580001) has been deleted
Patchset #13 (id:620001) has been deleted
rbpotter@chromium.org changed reviewers: + sky@chromium.org
Modified to check for the independent bounds flag in the window targeter. Without this extra check, the targeting changes to make clicks outside the parent window register were breaking various targeting unit tests. Adding the check required moving the definition of the property to a new location to avoid a circular dependency.
components/{constrained_window,web_modal} lgtm, with nits
https://codereview.chromium.org/2172363002/diff/660001/components/constrained...
File components/constrained_window/constrained_window_views.cc (right):
https://codereview.chromium.org/2172363002/diff/660001/components/constrained...
components/constrained_window/constrained_window_views.cc:146:
(widget->GetNativeWindowProperty(aura::kHasIndependentBoundsKey) != NULL);
nits: no need for the enclosing parens, and compare to nullptr rather than NULL
(nullptr is preferred in new Chromium code)
https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:103: // is later overriden by the size of the parent window. overridden? https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:107: const gfx::Size kMinDialogSize(438, 455); So no behavior changes on Windows/Linux, right? https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:128: size->Enlarge(-expand_width, -expand_height); Why not just calculate |expand_width| so it's negative in the first place? And rename to shrink_width? https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:248: web_modal::WebContentsModalDialogManager::FromWebContents( Combine the same code in the if and else branch for getting the WebContentsModalDialogManager*. https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:275: // bounds, so can extend outside of the parent window when resized s/so/so it/ https://codereview.chromium.org/2172363002/diff/660001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/2172363002/diff/660001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:104: return nonclipped_child_dialogs_.find(dialog) != use base::ContainsKey() https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.cc:69: return child->GetNativeWindowProperty(aura::kHasIndependentBoundsKey) != NULL; nit: no NULLs in new code, use nullptr. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.cc:76: bool canAccept = true; nit: can_accept, in_bounds below. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targeter.h File ui/aura/window_targeter.h (right): https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:21: namespace aura { nit: leave a blank line after the namespace like before, ditto elsewhere. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:71: bool ChildHasBoundsOutsideParent(Window* parent, Window* child); static method since it doesn't depend on |this| at all? https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:73: // Returns whether the location of the event is in an actionable region of s/the location of the event/|point|/ https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:76: // kHasIndependentBounds property is set. If in_parent_coordinates is true nit: refer to variables as |var| https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:80: gfx::Point point, Pass by const-ref
Patchset #15 (id:680001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #16 (id:720001) has been deleted
Finished updating to reflect comments. Note that there are several other changes as well. I found out this morning that a patch that did some significant shifting of the constrained window/modal dialog code went through last night, so I had to change some things to reflect the new state of the code. https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:103: // is later overriden by the size of the parent window. On 2016/08/18 20:30:57, Lei Zhang wrote: > overridden? Done. https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:107: const gfx::Size kMinDialogSize(438, 455); On 2016/08/18 20:30:57, Lei Zhang wrote: > So no behavior changes on Windows/Linux, right? There will be no behavior changes in terms of the minimum size - it will still shrink to the size of the parent window. The only thing that might change is that it will keep its border within the parent window a little longer - once the parent window gets small enough that the minimum size gets ignored the dialog goes almost to the edge of the parent window since it no longer has its border. Is this a problem? https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:128: size->Enlarge(-expand_width, -expand_height); On 2016/08/18 20:30:57, Lei Zhang wrote: > Why not just calculate |expand_width| so it's negative in the first place? And > rename to shrink_width? Done. I thought it was more readable to take the minimum of the desired border and the remaining size rather than doing a max of two negative values, and renaming to "shrink" while making them negative would seem like we are shrinking by a negative amount, when really we are expanding by a negative amount. https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/ui/view... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:248: web_modal::WebContentsModalDialogManager::FromWebContents( On 2016/08/18 20:30:57, Lei Zhang wrote: > Combine the same code in the if and else branch for getting the > WebContentsModalDialogManager*. Non applicable at this point, this code was refactored in a patch that landed early this morning... https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/ui/view... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:275: // bounds, so can extend outside of the parent window when resized On 2016/08/18 20:30:57, Lei Zhang wrote: > s/so/so it/ Done. https://codereview.chromium.org/2172363002/diff/660001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/660001/components/constrained... components/constrained_window/constrained_window_views.cc:146: (widget->GetNativeWindowProperty(aura::kHasIndependentBoundsKey) != NULL); On 2016/08/18 19:22:54, Mike Wittman wrote: > nits: no need for the enclosing parens, and compare to nullptr rather than NULL > (nullptr is preferred in new Chromium code) Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.cc:69: return child->GetNativeWindowProperty(aura::kHasIndependentBoundsKey) != NULL; On 2016/08/18 20:30:57, Lei Zhang wrote: > nit: no NULLs in new code, use nullptr. Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.cc:76: bool canAccept = true; On 2016/08/18 20:30:57, Lei Zhang wrote: > nit: can_accept, in_bounds below. Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targeter.h File ui/aura/window_targeter.h (right): https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:21: namespace aura { On 2016/08/18 20:30:57, Lei Zhang wrote: > nit: leave a blank line after the namespace like before, ditto elsewhere. Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:71: bool ChildHasBoundsOutsideParent(Window* parent, Window* child); On 2016/08/18 20:30:57, Lei Zhang wrote: > static method since it doesn't depend on |this| at all? Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:73: // Returns whether the location of the event is in an actionable region of On 2016/08/18 20:30:58, Lei Zhang wrote: > s/the location of the event/|point|/ Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:76: // kHasIndependentBounds property is set. If in_parent_coordinates is true On 2016/08/18 20:30:57, Lei Zhang wrote: > nit: refer to variables as |var| Done. https://codereview.chromium.org/2172363002/diff/660001/ui/aura/window_targete... ui/aura/window_targeter.h:80: gfx::Point point, On 2016/08/18 20:30:57, Lei Zhang wrote: > Pass by const-ref Done.
nits nits nits https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:107: const gfx::Size kMinDialogSize(438, 455); On 2016/08/19 01:59:09, rbpotter wrote: > On 2016/08/18 20:30:57, Lei Zhang wrote: > > So no behavior changes on Windows/Linux, right? > > There will be no behavior changes in terms of the minimum size - it will still > shrink to the size of the parent window. The only thing that might change is > that it will keep its border within the parent window a little longer - once the > parent window gets small enough that the minimum size gets ignored the dialog > goes almost to the edge of the parent window since it no longer has its border. > Is this a problem? No, seems ok. https://codereview.chromium.org/2172363002/diff/660001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:128: size->Enlarge(-expand_width, -expand_height); On 2016/08/19 01:59:09, rbpotter wrote: > On 2016/08/18 20:30:57, Lei Zhang wrote: > > Why not just calculate |expand_width| so it's negative in the first place? And > > rename to shrink_width? > > Done. I thought it was more readable to take the minimum of the desired border > and the remaining size rather than doing a max of two negative values, and > renaming to "shrink" while making them negative would seem like we are shrinking > by a negative amount, when really we are expanding by a negative amount. *shrug* If you like it better the other way, feel free to go back. https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:101: // Gives ChromeOS dialogs the requested minimum sizei of 438x455 without s/sizei/size/ https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:126: std::max(-size->width() + kMinDialogSize.width(), -2 * kBorder); If you stick with |shrink_width|, do kMinDialogSize.width() - size->width() https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right): https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:249: content::BrowserContext* browser_context, bad indent https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/constrained_window_views.cc:152: LOG(ERROR) << "No independent bounds key, setting to parent size."; Is this intended to be checked in? https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/constrained_window_views.h:65: // nonclipped dialogs by default, this has the same behavior as non-clipped https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:87: #endif // OS_CHROMEOS // defined(OS_CHROMEOS) https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:88: #endif label this one too. // defined(USE_AURA) https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/s... File components/web_modal/single_web_contents_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/s... components/web_modal/single_web_contents_dialog_manager.h:28: // Notify the delegate that the dialog is closing. The native This was "Notifies" before... https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... ui/aura/window_targeter.cc:70: child->GetNativeWindowProperty(aura::kHasIndependentBoundsKey) != nullptr; Or !!foo is you want it to fit on one line again. https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... ui/aura/window_targeter.cc:103: } bad indent https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targeter.h File ui/aura/window_targeter.h (right): https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... ui/aura/window_targeter.h:72: static bool ChildHasBoundsOutsideParent(Window* parent, Window *child); '*' goes with the type, not the name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
not lgtm (while reviewing merged changes) https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/show_modal_dialog_cocoa.cc (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/show_modal_dialog_cocoa.cc:36: // Mac uses sheets, so just call the normal show modal dialog, and the dialog nit: indentation https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:59: void AddNonClippedDialog(gfx::NativeWindow dialog) override; why do we need this function?
https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:101: // Gives ChromeOS dialogs the requested minimum sizei of 438x455 without On 2016/08/19 03:28:46, Lei Zhang wrote: > s/sizei/size/ Done. https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:126: std::max(-size->width() + kMinDialogSize.width(), -2 * kBorder); On 2016/08/19 03:28:46, Lei Zhang wrote: > If you stick with |shrink_width|, do kMinDialogSize.width() - size->width() Done. https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm (right): https://codereview.chromium.org/2172363002/diff/740001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm:249: content::BrowserContext* browser_context, On 2016/08/19 03:28:46, Lei Zhang wrote: > bad indent Done. https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/constrained_window_views.cc:152: LOG(ERROR) << "No independent bounds key, setting to parent size."; On 2016/08/19 03:28:47, Lei Zhang wrote: > Is this intended to be checked in? Done. https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/constrained_window_views.h:65: // nonclipped dialogs by default, this has the same behavior as On 2016/08/19 03:28:47, Lei Zhang wrote: > non-clipped Done. https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:87: #endif // OS_CHROMEOS On 2016/08/19 03:28:47, Lei Zhang wrote: > // defined(OS_CHROMEOS) Done. https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:88: #endif On 2016/08/19 03:28:47, Lei Zhang wrote: > label this one too. // defined(USE_AURA) Done. https://codereview.chromium.org/2172363002/diff/740001/components/constrained... File components/constrained_window/show_modal_dialog_cocoa.cc (right): https://codereview.chromium.org/2172363002/diff/740001/components/constrained... components/constrained_window/show_modal_dialog_cocoa.cc:36: // Mac uses sheets, so just call the normal show modal dialog, and the dialog On 2016/08/19 14:57:29, Mike Wittman wrote: > nit: indentation Done. https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/s... File components/web_modal/single_web_contents_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/s... components/web_modal/single_web_contents_dialog_manager.h:28: // Notify the delegate that the dialog is closing. The native On 2016/08/19 03:28:47, Lei Zhang wrote: > This was "Notifies" before... Done. https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:59: void AddNonClippedDialog(gfx::NativeWindow dialog) override; On 2016/08/19 14:57:29, Mike Wittman wrote: > why do we need this function? I added it in since ShowModalDialog moved to show_modal_dialog_views.cc in constrained window (it was previously defined here as a public function in the web contents modal dialog manager). I wanted to keep ShowNonClippedModalDialog with ShowModalDialog so I moved it to constrained_window/show_modal_dialog_views.cc. From this new location it can't access the nonclipped_child_dialogs_. The dialogs still need to be added to that set though, so I added this function to allow the new ShowNonClippedModalDialog function to add the dialogs to the manager like it did before. Should I just make the nonclipped_child_dialogs_ public instead? https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... ui/aura/window_targeter.cc:70: child->GetNativeWindowProperty(aura::kHasIndependentBoundsKey) != nullptr; On 2016/08/19 03:28:47, Lei Zhang wrote: > Or !!foo is you want it to fit on one line again. Done. https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... ui/aura/window_targeter.cc:103: } On 2016/08/19 03:28:47, Lei Zhang wrote: > bad indent Done. https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targeter.h File ui/aura/window_targeter.h (right): https://codereview.chromium.org/2172363002/diff/740001/ui/aura/window_targete... ui/aura/window_targeter.h:72: static bool ChildHasBoundsOutsideParent(Window* parent, Window *child); On 2016/08/19 03:28:47, Lei Zhang wrote: > '*' goes with the type, not the name. Done.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chrome/ lgtm
https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:59: void AddNonClippedDialog(gfx::NativeWindow dialog) override; On 2016/08/19 16:59:42, rbpotter wrote: > On 2016/08/19 14:57:29, Mike Wittman wrote: > > why do we need this function? > > I added it in since ShowModalDialog moved to show_modal_dialog_views.cc in > constrained window (it was previously defined here as a public function in the > web contents modal dialog manager). I wanted to keep ShowNonClippedModalDialog > with ShowModalDialog so I moved it to > constrained_window/show_modal_dialog_views.cc. From this new location it can't > access the nonclipped_child_dialogs_. The dialogs still need to be added to that > set though, so I added this function to allow the new ShowNonClippedModalDialog > function to add the dialogs to the manager like it did before. Should I just > make the nonclipped_child_dialogs_ public instead? I see. This approach is kind of odd because at the call site it's telling the manager how to deal with the dialog before it has started managing it. Given the new state of the code, I think a better approach would be to pass an is_nonclipped parameter to the NativeWebContentsModalDialogManagerViews constructor and through to ManageDialog, and revert all the changes in this file.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/19 17:51:42, Mike Wittman wrote: > https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... > File components/web_modal/web_contents_modal_dialog_manager.h (right): > > https://codereview.chromium.org/2172363002/diff/740001/components/web_modal/w... > components/web_modal/web_contents_modal_dialog_manager.h:59: void > AddNonClippedDialog(gfx::NativeWindow dialog) override; > On 2016/08/19 16:59:42, rbpotter wrote: > > On 2016/08/19 14:57:29, Mike Wittman wrote: > > > why do we need this function? > > > > I added it in since ShowModalDialog moved to show_modal_dialog_views.cc in > > constrained window (it was previously defined here as a public function in the > > web contents modal dialog manager). I wanted to keep ShowNonClippedModalDialog > > with ShowModalDialog so I moved it to > > constrained_window/show_modal_dialog_views.cc. From this new location it can't > > access the nonclipped_child_dialogs_. The dialogs still need to be added to > that > > set though, so I added this function to allow the new > ShowNonClippedModalDialog > > function to add the dialogs to the manager like it did before. Should I just > > make the nonclipped_child_dialogs_ public instead? > > I see. This approach is kind of odd because at the call site it's telling the > manager how to deal with the dialog before it has started managing it. > > Given the new state of the code, I think a better approach would be to pass an > is_nonclipped parameter to the NativeWebContentsModalDialogManagerViews > constructor and through to ManageDialog, and revert all the changes in this > file. Okay. Changed it to work this way. I was thinking that having the add nonclipped function separate from the create/show functions wasn't great as it could allow extra dialogs to be added without actually being managed, but couldn't think of another way without going back to threading the is_nonclipped state through to the different functions again. However with the new code structure this affects fewer functions than it did previously.
components/{constrained_window,web_modal} re-lgtm, with nits
https://codereview.chromium.org/2172363002/diff/780001/components/constrained...
File
components/constrained_window/native_web_contents_modal_dialog_manager_views.cc
(right):
https://codereview.chromium.org/2172363002/diff/780001/components/constrained...
components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:60:
ManageDialog(bool is_nonclipped) {
nit: line break after open paren rather than before function name
https://codereview.chromium.org/2172363002/diff/780001/components/constrained...
File components/constrained_window/show_modal_dialog_views.cc (right):
https://codereview.chromium.org/2172363002/diff/780001/components/constrained...
components/constrained_window/show_modal_dialog_views.cc:28:
content::WebContents* web_contents) {
nit: indentation
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2172363002/diff/780001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/2172363002/diff/780001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:60: ManageDialog(bool is_nonclipped) { On 2016/08/19 19:46:57, Mike Wittman wrote: > nit: line break after open paren rather than before function name Done. https://codereview.chromium.org/2172363002/diff/780001/components/constrained... File components/constrained_window/show_modal_dialog_views.cc (right): https://codereview.chromium.org/2172363002/diff/780001/components/constrained... components/constrained_window/show_modal_dialog_views.cc:28: content::WebContents* web_contents) { On 2016/08/19 19:46:57, Mike Wittman wrote: > nit: indentation Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/19 21:43:38, rbpotter wrote: > https://codereview.chromium.org/2172363002/diff/780001/components/constrained... > File > components/constrained_window/native_web_contents_modal_dialog_manager_views.cc > (right): > > https://codereview.chromium.org/2172363002/diff/780001/components/constrained... > components/constrained_window/native_web_contents_modal_dialog_manager_views.cc:60: > ManageDialog(bool is_nonclipped) { > On 2016/08/19 19:46:57, Mike Wittman wrote: > > nit: line break after open paren rather than before function name > > Done. > > https://codereview.chromium.org/2172363002/diff/780001/components/constrained... > File components/constrained_window/show_modal_dialog_views.cc (right): > > https://codereview.chromium.org/2172363002/diff/780001/components/constrained... > components/constrained_window/show_modal_dialog_views.cc:28: > content::WebContents* web_contents) { > On 2016/08/19 19:46:57, Mike Wittman wrote: > > nit: indentation > > Done. Not sure if this was already clear or not (apologies if it is and this is an unnecessary e-mail), but this should be ready for final review on the window_targeter code. Thanks!
It seems like there is a misunderstanding. My suggestion for how to do this is that if the dialog is allowed to extend outside the bounds of the parent, then it should be a top level window and not a child window. AFAICT from a quick look at the patch you are creating a child window. That is why you're having to change WindowTargeter. I think a top level window is a better option for this and likely less problems. I have no doubt you'll need to change event targeting though to ensure the webcontents doesn't get clicks when this dialog is shown.
Patchset #22 (id:860001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #23 (id:900001) has been deleted
Patchset #23 (id:920001) has been deleted
Patchset #23 (id:940001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... components/constrained_window/constrained_window_views.h:52: // dialog (peer of the |web_contents| native window dialog host rather than a It isn't a peer, is it? It's a top level, right? https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... components/constrained_window/constrained_window_views.h:57: void ShowTopLevelModalDialog(gfx::NativeWindow dialog, Can we move this logic into ShowWebModalDialogViews? In other words, do we really need to support both variants? Or said differently, can we make all web modal dialogs get the new behavior? https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... components/constrained_window/constrained_window_views.h:80: views::Widget* ShowTopLevelWebModalDialogViews( I'm confused by the need for this and ShowTopLevelModalDialog. They have roughly the same description. Maybe I'm missing something?
Patchset #29 (id:1080001) has been deleted
Patchset #29 (id:1100001) has been deleted
Patchset #30 (id:1140001) has been deleted
Patchset #30 (id:1160001) has been deleted
Patchset #30 (id:1180001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #31 (id:1220001) has been deleted
Patchset #32 (id:1260001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Patchset #33 (id:1300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #34 (id:1340001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Patchset #35 (id:1380001) has been deleted
Patchset #35 (id:1400001) has been deleted
Patchset #35 (id:1420001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Apologies for dropping this for a while. I think this is ready for another initial look. On 2016/09/16 16:40:25, sky wrote: > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... > File components/constrained_window/constrained_window_views.h (right): > > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... > components/constrained_window/constrained_window_views.h:52: // dialog (peer of > the |web_contents| native window dialog host rather than a > It isn't a peer, is it? It's a top level, right? It is a top level, fixed the comment. > > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... > components/constrained_window/constrained_window_views.h:57: void > ShowTopLevelModalDialog(gfx::NativeWindow dialog, > Can we move this logic into ShowWebModalDialogViews? In other words, do we > really need to support both variants? Or said differently, can we make all web > modal dialogs get the new behavior? I didn't want to change the existing behavior for the other dialogs since I don't think they have the same problem with elements shrinking arbitrarily small or disappearing like the preview does. However, if you think that is the better approach, I can experiment with making all dialogs have the new behavior. > > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... > components/constrained_window/constrained_window_views.h:80: views::Widget* > ShowTopLevelWebModalDialogViews( > I'm confused by the need for this and ShowTopLevelModalDialog. They have roughly > the same description. Maybe I'm missing something? I fixed these descriptions. One creates the widget while the other assumes it already exists.
On Mon, Nov 28, 2016 at 6:37 PM, <rbpotter@chromium.org> wrote: > Apologies for dropping this for a while. I think this is ready for another > initial look. > > On 2016/09/16 16:40:25, sky wrote: >> > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... >> File components/constrained_window/constrained_window_views.h (right): >> >> > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... >> components/constrained_window/constrained_window_views.h:52: // dialog >> (peer > of >> the |web_contents| native window dialog host rather than a >> It isn't a peer, is it? It's a top level, right? > It is a top level, fixed the comment. >> >> > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... >> components/constrained_window/constrained_window_views.h:57: void >> ShowTopLevelModalDialog(gfx::NativeWindow dialog, >> Can we move this logic into ShowWebModalDialogViews? In other words, do we >> really need to support both variants? Or said differently, can we make all >> web >> modal dialogs get the new behavior? > I didn't want to change the existing behavior for the other dialogs since I > don't think they have the same problem with elements shrinking arbitrarily > small > or disappearing like the preview does. However, if you think that is the > better > approach, I can experiment with making all dialogs have the new behavior. Wouldn't the problem occur if you shrink the browser down to a small enough size for all existing dialogs? From a complexity perspective it would be better to support only one vs two. Is there a reason not to convert everyone to the new behavior? -Scott >> >> > https://codereview.chromium.org/2172363002/diff/1060001/components/constraine... >> components/constrained_window/constrained_window_views.h:80: >> views::Widget* >> ShowTopLevelWebModalDialogViews( >> I'm confused by the need for this and ShowTopLevelModalDialog. They have > roughly >> the same description. Maybe I'm missing something? > I fixed these descriptions. One creates the widget while the other assumes > it > already exists. > > > https://codereview.chromium.org/2172363002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) |
