Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168953002:
Disables the context menu in User Manager's reauth dialog (Closed)
DescriptionDisables the context menu in User Manager's reauth dialog.
This prevents user from inspecting the contents of the dialog which causes a crash.
This CL also removes the logic where the WebContentsObserver closes the dialog automatically when the final URL is reached. The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). That logic is not needed.
BUG=608313
Committed: https://crrev.com/181eb213602d7e36865b6cc9fcc3835c721ab994
Cr-Commit-Position: refs/heads/master@{#409311}
Patch Set 1 : Initial #
Total comments: 2
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : Addressed comment #
Messages
Total messages: 37 (21 generated)
The CQ bit was checked by mahmadi@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 mahmadi@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 #1 (id:1) has been deleted
Description was changed from ========== Refactors User Manager's reauth dialog code and disallows context menu BUG=608313 ========== to ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. BUG=608313 ==========
mahmadi@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, Please review this CL. https://codereview.chromium.org/2168953002/diff/20001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (left): https://codereview.chromium.org/2168953002/diff/20001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:39: CloseReauthDialog(); The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). This is not needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2168953002/diff/20001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2168953002/diff/20001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.h:76: class BaseReauthDialogDelegate : virtual public content::WebContentsDelegate { multiple inheritance is frowned upon, and diamond inheritance more so. Please restructure things so you aren't inheriting from a bunch of non-virtual classes.
The CQ bit was checked by mahmadi@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...
Hi, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (left): https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:37: if (url.ReplaceComponents(replacements) == How come you're changing this code? The description mentions menu, not this. https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (right): https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:23: content::WebContents* web_contents) : web_contents_(web_contents) {} Why do you set the delegate on line 53 instead of here? https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:41: if (url.GetOrigin() != GURL(std::string(chrome::kChromeUIChromeSigninURL))) Do you really need the std::string here? https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.h:81: bool HandleContextMenu(const content::ContextMenuParams& params) override; Have you considered leaving this class as is and making views have a WebContentsDelegate that overrides this function? That way both platform implementations are the same.
Description was changed from ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. BUG=608313 ========== to ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. This CL also removes the logic where the WebContentsObserver closes the dialog automatically when the final URL is reached. The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). That logic is not needed. BUG=608313 ==========
https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (left): https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:37: if (url.ReplaceComponents(replacements) == On 2016/07/22 22:29:01, sky wrote: > How come you're changing this code? The description mentions menu, not this. I mentioned in a comment in the initial patch that: "The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). This is not needed." My bad. should've mentioned it in the desc. https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (right): https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:23: content::WebContents* web_contents) : web_contents_(web_contents) {} On 2016/07/22 22:29:01, sky wrote: > Why do you set the delegate on line 53 instead of here? The delegate is set similarly in the cocoa logic. My thinking was that setting the webContents' delegate feels more natural if done explicitly and setting it here would be more appropriate for an observer. But no good reason really. https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:41: if (url.GetOrigin() != GURL(std::string(chrome::kChromeUIChromeSigninURL))) On 2016/07/22 22:29:01, sky wrote: > Do you really need the std::string here? Not really. Removed. https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2168953002/diff/40001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.h:81: bool HandleContextMenu(const content::ContextMenuParams& params) override; On 2016/07/22 22:29:01, sky wrote: > Have you considered leaving this class as is and making views have a > WebContentsDelegate that overrides this function? That way both platform > implementations are the same. Unfortunately that alone won't work. The logic that I really want to share between the platforms is LoadingStateChanged(...), where we find the embedded webview and manage that instead. For that this class has to inherit content::WebContentsDelegate etc etc.
https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (right): https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:51: web_contents_ = *content_set.begin(); How come web_contents_ is reset here? This class then becomes the delegate for multiple WebContents. How do you guarantee this class outlives all the webcontents?
https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (right): https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:51: web_contents_ = *content_set.begin(); On 2016/07/25 14:53:22, sky wrote: > How come web_contents_ is reset here? This class then becomes the delegate for > multiple WebContents. How do you guarantee this class outlives all the > webcontents? Sorry for the late reply... Because we would need to be delegate for WebContents of its guest webview (see new_inline_login.html) in order to handle its context menu action. The host WebView is owned by ReauthDelegate which implements BaseReauthDialogDelegate (cocoa's impl is similar) and outlives it. I believe the guest WebView's life is also dependent on its host. So we shouldn't have any problems here.
https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (right): https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:36: // If still observing the top level web contents, try to find the embedded I think this would be less confusing if you cached the WebContents* for the top and checked again it. Less error prone.
The CQ bit was checked by mahmadi@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/2168953002/diff/60001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.cc (right): https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.cc:36: // If still observing the top level web contents, try to find the embedded On 2016/08/02 03:14:08, sky wrote: > I think this would be less confusing if you cached the WebContents* for the top > and checked again it. Less error prone. Done. In fact since the |source| WebContents is passed to the function we don't need to cache it at all. The logic can be simplified even further by looking for the guest WebContents only if guest_web_contents_ hasn't already been set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/01 21:44:59, moe wrote: > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > File chrome/browser/ui/user_manager.cc (right): > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > chrome/browser/ui/user_manager.cc:51: web_contents_ = *content_set.begin(); > On 2016/07/25 14:53:22, sky wrote: > > How come web_contents_ is reset here? This class then becomes the delegate for > > multiple WebContents. How do you guarantee this class outlives all the > > webcontents? > > Sorry for the late reply... > Because we would need to be delegate for WebContents of its guest webview (see > new_inline_login.html) in order to handle its context menu action. > The host WebView is owned by ReauthDelegate which implements > BaseReauthDialogDelegate (cocoa's impl is similar) and outlives it. I believe > the guest WebView's life is also dependent on its host. So we shouldn't have any > problems here. Sorry, one more question about this. How come you only set the delegate of the first guest and not all?
On 2016/08/02 19:37:49, sky wrote: > On 2016/08/01 21:44:59, moe wrote: > > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > > File chrome/browser/ui/user_manager.cc (right): > > > > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > > chrome/browser/ui/user_manager.cc:51: web_contents_ = *content_set.begin(); > > On 2016/07/25 14:53:22, sky wrote: > > > How come web_contents_ is reset here? This class then becomes the delegate > for > > > multiple WebContents. How do you guarantee this class outlives all the > > > webcontents? > > > > Sorry for the late reply... > > Because we would need to be delegate for WebContents of its guest webview (see > > new_inline_login.html) in order to handle its context menu action. > > The host WebView is owned by ReauthDelegate which implements > > BaseReauthDialogDelegate (cocoa's impl is similar) and outlives it. I believe > > the guest WebView's life is also dependent on its host. So we shouldn't have > any > > problems here. > > Sorry, one more question about this. How come you only set the delegate of the > first guest and not all? np. Sorry I don't have much experience with WebView and WebContents so I may be sounding a bit off. AFAIK, there is only one embedded webview tag in new_inline_login.html. so there should be only one guest WebContents. that seems to be what DCHECK_LE(content_set.size(), 1U asserts here. Or am I missing something?
On 2016/08/02 19:52:01, moe wrote: > On 2016/08/02 19:37:49, sky wrote: > > On 2016/08/01 21:44:59, moe wrote: > > > > > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > > > File chrome/browser/ui/user_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > > > chrome/browser/ui/user_manager.cc:51: web_contents_ = *content_set.begin(); > > > On 2016/07/25 14:53:22, sky wrote: > > > > How come web_contents_ is reset here? This class then becomes the delegate > > for > > > > multiple WebContents. How do you guarantee this class outlives all the > > > > webcontents? > > > > > > Sorry for the late reply... > > > Because we would need to be delegate for WebContents of its guest webview > (see > > > new_inline_login.html) in order to handle its context menu action. > > > The host WebView is owned by ReauthDelegate which implements > > > BaseReauthDialogDelegate (cocoa's impl is similar) and outlives it. I > believe > > > the guest WebView's life is also dependent on its host. So we shouldn't have > > any > > > problems here. > > > > Sorry, one more question about this. How come you only set the delegate of the > > first guest and not all? > > np. Sorry I don't have much experience with WebView and WebContents so I may be > sounding a bit off. AFAIK, there is only one embedded webview tag in > new_inline_login.html. so there should be only one guest WebContents. that seems > to be what DCHECK_LE(content_set.size(), 1U asserts here. Or am I missing > something? ok, LGTM
On 2016/08/02 20:32:08, sky wrote: > On 2016/08/02 19:52:01, moe wrote: > > On 2016/08/02 19:37:49, sky wrote: > > > On 2016/08/01 21:44:59, moe wrote: > > > > > > > > > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > > > > File chrome/browser/ui/user_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2168953002/diff/60001/chrome/browser/ui/user_... > > > > chrome/browser/ui/user_manager.cc:51: web_contents_ = > *content_set.begin(); > > > > On 2016/07/25 14:53:22, sky wrote: > > > > > How come web_contents_ is reset here? This class then becomes the > delegate > > > for > > > > > multiple WebContents. How do you guarantee this class outlives all the > > > > > webcontents? > > > > > > > > Sorry for the late reply... > > > > Because we would need to be delegate for WebContents of its guest webview > > (see > > > > new_inline_login.html) in order to handle its context menu action. > > > > The host WebView is owned by ReauthDelegate which implements > > > > BaseReauthDialogDelegate (cocoa's impl is similar) and outlives it. I > > believe > > > > the guest WebView's life is also dependent on its host. So we shouldn't > have > > > any > > > > problems here. > > > > > > Sorry, one more question about this. How come you only set the delegate of > the > > > first guest and not all? > > > > np. Sorry I don't have much experience with WebView and WebContents so I may > be > > sounding a bit off. AFAIK, there is only one embedded webview tag in > > new_inline_login.html. so there should be only one guest WebContents. that > seems > > to be what DCHECK_LE(content_set.size(), 1U asserts here. Or am I missing > > something? > > ok, LGTM Thank you!
The CQ bit was checked by mahmadi@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.
Description was changed from ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. This CL also removes the logic where the WebContentsObserver closes the dialog automatically when the final URL is reached. The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). That logic is not needed. BUG=608313 ========== to ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. This CL also removes the logic where the WebContentsObserver closes the dialog automatically when the final URL is reached. The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). That logic is not needed. BUG=608313 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. This CL also removes the logic where the WebContentsObserver closes the dialog automatically when the final URL is reached. The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). That logic is not needed. BUG=608313 ========== to ========== Disables the context menu in User Manager's reauth dialog. This prevents user from inspecting the contents of the dialog which causes a crash. This CL also removes the logic where the WebContentsObserver closes the dialog automatically when the final URL is reached. The reauth dialog is closed explicitly in InlineLoginHandler::HandleDialogClose(). That logic is not needed. BUG=608313 Committed: https://crrev.com/181eb213602d7e36865b6cc9fcc3835c721ab994 Cr-Commit-Position: refs/heads/master@{#409311} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/181eb213602d7e36865b6cc9fcc3835c721ab994 Cr-Commit-Position: refs/heads/master@{#409311} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
