Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2357243003:
[UI] Make auto-resizable constrained web dialog work even when initiated (Closed)
Description[UI] Make auto-resizable constrained web dialog work even when initiated
from a non-top-level WebContents (<webview>).
CreateWebModalDialogViews and ShowModalDialog should only be called with
a top-level WebContents since they assume the existence of
WebContentsModalDialogManager. For non-autoresizable dialogs, this is
already taken care of by ShowWebModalDialogViews(). For resizable
dialogs, these 2 functions have to be called separately. Introduce a
helper function GetTopLevelWebContents that can be used by the
autoresize logic as well as ShowWebModalDialogViews().
BUG=649191
Committed: https://crrev.com/c5eae4a12a589376be4c5d06fe4c356ee6d495a5
Cr-Commit-Position: refs/heads/master@{#420636}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed comments #
Messages
Total messages: 18 (7 generated)
imcheng@chromium.org changed reviewers: + msw@chromium.org, wjmaclean@chromium.org
msw / wjmaclean: PTAL at everything Let me know if you prefer another way to structure this.
https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:83: content::WebContents* top_level_web_contents = It seems odd to present a dialog from an embedded webview over the top-level embedder's web contents. Please post a screenshot of this behavior, and please seek a security review. Your cl description seems to indicate that this is the current behavior for other (non-resizable) dialogs; if so, please also point out some examples of that existing behavior? What do other platforms do?
https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:83: content::WebContents* top_level_web_contents = On 2016/09/22 03:27:35, msw wrote: > It seems odd to present a dialog from an embedded webview over the top-level > embedder's web contents. Please post a screenshot of this behavior, and please > seek a security review. Your cl description seems to indicate that this is the > current behavior for other (non-resizable) dialogs; if so, please also point out > some examples of that existing behavior? What do other platforms do? Indeed, we currently allow embedded <webview> to create non-autoresizable constrained web dialogs over the top-level WebContents. It is currently used by the auth login flow, but lazyboy@ (CC'ed) may be able to provide additional background about this. The dialog has the same behavior in terms of appearance / modality, but I can provide a screenshot tomorrow. Also, the autoresizable constrained web dialog already works on Mac today. The Mac implementation of the auto-resizable dialog creates a window using the embedder's top-level WebContents: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/constrained_wind... BTW, who do you suggest looping in for security review?
https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:83: content::WebContents* top_level_web_contents = On 2016/09/22 04:32:23, imcheng wrote: > On 2016/09/22 03:27:35, msw wrote: > > It seems odd to present a dialog from an embedded webview over the top-level > > embedder's web contents. Please post a screenshot of this behavior, and please > > seek a security review. Your cl description seems to indicate that this is the > > current behavior for other (non-resizable) dialogs; if so, please also point > out > > some examples of that existing behavior? What do other platforms do? > > Indeed, we currently allow embedded <webview> to create non-autoresizable > constrained web dialogs over the top-level WebContents. It is currently used by > the auth login flow, but lazyboy@ (CC'ed) may be able to provide additional > background about this. The dialog has the same behavior in terms of appearance / > modality, but I can provide a screenshot tomorrow. > > Also, the autoresizable constrained web dialog already works on Mac today. The > Mac implementation of the auto-resizable dialog creates a window using the > embedder's top-level WebContents: > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/constrained_wind... > > BTW, who do you suggest looping in for security review? Thanks for the info, this probably doesn't need security review if it's already done for Mac and for Views non-autoresizable constrained web dialogs. https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:89: top_level_web_contents) Check for null before using |top_level_web_contents|? https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:247: constrained_window::GetTopLevelWebContents( Ditto, check for null first? https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:300: constrained_window::CreateWebModalDialogViews(dialog, top_level_web_contents); nit: maybe DCHECK that |top_level_web_contents| isn't null here? https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... components/constrained_window/constrained_window_views.cc:148: content::WebContents* GetTopLevelWebContents( nit: maybe just skip the helper and inline calls to guest_view::GuestViewBase::GetTopLevelWebContents?
https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:89: top_level_web_contents) On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > Check for null before using |top_level_web_contents|? Definitely check for null ... there's no guarantee this will be non-null. https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... components/constrained_window/constrained_window_views.cc:148: content::WebContents* GetTopLevelWebContents( On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > nit: maybe just skip the helper and inline calls to > guest_view::GuestViewBase::GetTopLevelWebContents? Wouldn't that mean that the guestview interface would need to be visible in the .h file?
Thanks for the review! https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:83: content::WebContents* top_level_web_contents = On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > On 2016/09/22 04:32:23, imcheng wrote: > > On 2016/09/22 03:27:35, msw wrote: > > > It seems odd to present a dialog from an embedded webview over the top-level > > > embedder's web contents. Please post a screenshot of this behavior, and > please > > > seek a security review. Your cl description seems to indicate that this is > the > > > current behavior for other (non-resizable) dialogs; if so, please also point > > out > > > some examples of that existing behavior? What do other platforms do? > > > > Indeed, we currently allow embedded <webview> to create non-autoresizable > > constrained web dialogs over the top-level WebContents. It is currently used > by > > the auth login flow, but lazyboy@ (CC'ed) may be able to provide additional > > background about this. The dialog has the same behavior in terms of appearance > / > > modality, but I can provide a screenshot tomorrow. > > > > Also, the autoresizable constrained web dialog already works on Mac today. The > > Mac implementation of the auto-resizable dialog creates a window using the > > embedder's top-level WebContents: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/constrained_wind... > > > > BTW, who do you suggest looping in for security review? > > Thanks for the info, this probably doesn't need security review if it's already > done for Mac and for Views non-autoresizable constrained web dialogs. Acknowledged. https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:89: top_level_web_contents) On 2016/09/22 15:36:48, wjmaclean wrote: > On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > > Check for null before using |top_level_web_contents|? > > Definitely check for null ... there's no guarantee this will be non-null. Done. Although I did notice that constrained_window_views.cc was calling this and using the result without checking for nullptr? https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:247: constrained_window::GetTopLevelWebContents( On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > Ditto, check for null first? Done. https://codereview.chromium.org/2357243003/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:300: constrained_window::CreateWebModalDialogViews(dialog, top_level_web_contents); On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > nit: maybe DCHECK that |top_level_web_contents| isn't null here? Done. https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... components/constrained_window/constrained_window_views.cc:148: content::WebContents* GetTopLevelWebContents( On 2016/09/22 15:36:48, wjmaclean wrote: > On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > > nit: maybe just skip the helper and inline calls to > > guest_view::GuestViewBase::GetTopLevelWebContents? > > Wouldn't that mean that the guestview interface would need to be visible in the > .h file? Yeah, I did it this way so I don't have to include GuestView stuff in constrained_web_dialog_delegate_views.cc, and to provide a helper function along side the other functions in constrained_window.
The CQ bit was checked by imcheng@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.
lgtm https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/2357243003/diff/1/components/constrained_wind... components/constrained_window/constrained_window_views.cc:148: content::WebContents* GetTopLevelWebContents( On 2016/09/22 17:20:58, imcheng wrote: > On 2016/09/22 15:36:48, wjmaclean wrote: > > On 2016/09/22 13:36:19, msw on vacation 9-22 wrote: > > > nit: maybe just skip the helper and inline calls to > > > guest_view::GuestViewBase::GetTopLevelWebContents? > > > > Wouldn't that mean that the guestview interface would need to be visible in > the > > .h file? > > Yeah, I did it this way so I don't have to include GuestView stuff in > constrained_web_dialog_delegate_views.cc, and to provide a helper function along > side the other functions in constrained_window. Ah, then this is fine, thanks.
lgtm
The CQ bit was checked by imcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [UI] Make auto-resizable constrained web dialog work even when initiated from a non-top-level WebContents (<webview>). CreateWebModalDialogViews and ShowModalDialog should only be called with a top-level WebContents since they assume the existence of WebContentsModalDialogManager. For non-autoresizable dialogs, this is already taken care of by ShowWebModalDialogViews(). For resizable dialogs, these 2 functions have to be called separately. Introduce a helper function GetTopLevelWebContents that can be used by the autoresize logic as well as ShowWebModalDialogViews(). BUG=649191 ========== to ========== [UI] Make auto-resizable constrained web dialog work even when initiated from a non-top-level WebContents (<webview>). CreateWebModalDialogViews and ShowModalDialog should only be called with a top-level WebContents since they assume the existence of WebContentsModalDialogManager. For non-autoresizable dialogs, this is already taken care of by ShowWebModalDialogViews(). For resizable dialogs, these 2 functions have to be called separately. Introduce a helper function GetTopLevelWebContents that can be used by the autoresize logic as well as ShowWebModalDialogViews(). BUG=649191 Committed: https://crrev.com/c5eae4a12a589376be4c5d06fe4c356ee6d495a5 Cr-Commit-Position: refs/heads/master@{#420636} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c5eae4a12a589376be4c5d06fe4c356ee6d495a5 Cr-Commit-Position: refs/heads/master@{#420636} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
