Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421943002:
Make JavaScript dialogs auto-dismiss on tab switch. (Closed)
|
Created:
4 years, 2 months ago by Avi (use Gerrit) Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake JavaScript dialogs auto-dismiss on tab switch.
This is a Views-only implementation for now. This works on the Mac too, but is ugly; a real Mac version is to come.
Behind the feature flag "AutoDismissingDialogs".
BUG=629964
Committed: https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29
Cr-Commit-Position: refs/heads/master@{#426133}
Patch Set 1 #Patch Set 2 : android #Patch Set 3 : close dialogs on window switch #Patch Set 4 : android #Patch Set 5 : fix #
Total comments: 38
Patch Set 6 : pkasting #
Total comments: 15
Patch Set 7 : more commentary, nits #
Total comments: 4
Patch Set 8 : last fixes #
Total comments: 2
Messages
Total messages: 52 (35 generated)
The CQ bit was checked by avi@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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by avi@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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by avi@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...)
The CQ bit was checked by avi@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.
avi@chromium.org changed reviewers: + pkasting@chromium.org
Peter: You're a UI-type peep, so I was wondering if I could run some reviews by you. Did you see my "Project OldSpice" doc? This is the first bit of the implementation.
The CQ bit was checked by avi@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.
Mostly nits https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:38: DCHECK(false); Do not handle DCHECK failure (banned by style guide). Just DCHECK(browser); here and keep going. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:56: content::WebContents* alerting_web_contents, Nit: Param name in .cc and .h should match https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:112: } else { Nit: No else after return https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:128: // was doing, but now the user can just close the page. Nit: I sort of feel like this comment is really a TODO on the message-suppression logic in general, like "rip all this out when auto-dismissal is on all the time". https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:192: // Handle any app-modal dialogs that may be going. Nit: "going"? You mean "showing"? (Sorry, but my brain is having a hard time parsing "going" here.) (2 places) https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:19: class JavaScriptDialogTabHelper Nit: While here: Would be nice to have an overview comment about the purpose of this class. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:46: // WebContentsObserver: Nit: Keep blocks of overrides and superclass list in same order https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:63: // itself calls it, but this class will do in some cases. Nit: "will do it" or "will do so". Would be nice to refer to some comment elsewhere that would make it clear what these kinds of cases might be (e.g. the class overview comment mentioned above?). https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:31: // JavaScriptDialogViews, constructor & destructor: Nit: Personally I'm not a fan of these ///// comment blocks on individual pieces of a single class' definition, because they tend to get out-of-date later as we add and move around functions, and they don't seem that helpful in the meantime. Also, they can hinder addressing my next comment. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:33: JavaScriptDialogViews::JavaScriptDialogViews( Nit: Function definition order should match .cc declaration order. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:64: dialog_callback_.Reset(); Why would we not call Close() instead? It's not obvious to me what the difference in intent of these two functions is, in part because the declaration for this method in the header has no comment on it. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:79: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; Nit: Shorter: const bool is_alert = message_type_ == content::JAVASCRIPT_MESSAGE_TYPE_ALERT; return ui::DIALOG_BUTTON_OK | (is_alert ? 0 : ui::DIALOG_BUTTON_CANCEL); https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:118: return views::DialogDelegate::GetInitiallyFocusedView(); Nit: Shorter: auto* text_box = message_box_view_->text_box(); return text_box ? text_box : views::DialogDelegate::GetInitiallyFocusedView(); https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:19: class JavaScriptDialogViews : public views::DialogDelegate { Nit: Add description of what this class is and why we have it. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:21: static base::WeakPtr<JavaScriptDialogViews> Create( Nit: Per https://google.github.io/styleguide/cppguide.html#Declaration_Order , destructor goes above static method https://codereview.chromium.org/2421943002/diff/80001/components/app_modal/ja... File components/app_modal/javascript_dialog_manager.h (right): https://codereview.chromium.org/2421943002/diff/80001/components/app_modal/ja... components/app_modal/javascript_dialog_manager.h:42: const GURL& origin_url); Nit: Move definition order in .cc to match
The CQ bit was checked by avi@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.
https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:38: DCHECK(false); On 2016/10/17 20:16:04, Peter Kasting wrote: > Do not handle DCHECK failure (banned by style guide). > > Just DCHECK(browser); here and keep going. Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:56: content::WebContents* alerting_web_contents, On 2016/10/17 20:16:04, Peter Kasting wrote: > Nit: Param name in .cc and .h should match Hm. The problem is that in this function, there are two (possibly different) WebContentses in play: the alerting one, which is passed in, and the parent one, which we get a pointer to later. If we match the .cc and .h names, then we have |parent_web_contents| and |web_contents| which is awkward. OTOH, if we explicitly call the alerting WebContents |alerting_web_contents|, like I do in this CL, then that significantly clarifies things inside this function, but we pay the price of having the .cc and .h param names not match. I think the clarity of the variable naming is worth the price of the param names not matching. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:112: } else { On 2016/10/17 20:16:04, Peter Kasting wrote: > Nit: No else after return Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:128: // was doing, but now the user can just close the page. On 2016/10/17 20:16:04, Peter Kasting wrote: > Nit: I sort of feel like this comment is really a TODO on the > message-suppression logic in general, like "rip all this out when auto-dismissal > is on all the time". Unfortunately, the old path without auto-dismissal will be with us forever, or until JavaScript dialogs are entirely removed. We still need that old path, complete with "suppress further dialogs", for onbeforeunload dialogs, for extension dialogs, and for WebContentses that aren't browser tabs. Given that the boolean |did_suppress_message| is going to stay forever in the dialog API in which we're called here, I wanted to note why we return a constant value. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:192: // Handle any app-modal dialogs that may be going. On 2016/10/17 20:16:04, Peter Kasting wrote: > Nit: "going"? You mean "showing"? (Sorry, but my brain is having a hard time > parsing "going" here.) (2 places) Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:19: class JavaScriptDialogTabHelper On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: While here: Would be nice to have an overview comment about the purpose of > this class. Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:46: // WebContentsObserver: On 2016/10/17 20:16:04, Peter Kasting wrote: > Nit: Keep blocks of overrides and superclass list in same order Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:63: // itself calls it, but this class will do in some cases. On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: "will do it" or "will do so". Would be nice to refer to some comment > elsewhere that would make it clear what these kinds of cases might be (e.g. the > class overview comment mentioned above?). Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:31: // JavaScriptDialogViews, constructor & destructor: On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Personally I'm not a fan of these ///// comment blocks on individual pieces > of a single class' definition, because they tend to get out-of-date later as we > add and move around functions, and they don't seem that helpful in the meantime. > Also, they can hinder addressing my next comment. Stolen from other Views code. Removed. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:33: JavaScriptDialogViews::JavaScriptDialogViews( On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Function definition order should match .cc declaration order. Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:64: dialog_callback_.Reset(); On 2016/10/17 20:16:05, Peter Kasting wrote: > Why would we not call Close() instead? It's not obvious to me what the > difference in intent of these two functions is, in part because the declaration > for this method in the header has no comment on it. Close() is a Views method, and is an implementation detail due to JavaScriptDialogViews being derived from views::DialogDelegate. When I write the Mac UI version, the callbacks are going to be different, so I want to have a single "hey, dialog, go away" method on both. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:79: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Shorter: > > const bool is_alert = message_type_ == content::JAVASCRIPT_MESSAGE_TYPE_ALERT; > return ui::DIALOG_BUTTON_OK | (is_alert ? 0 : ui::DIALOG_BUTTON_CANCEL); OK. (I don't know what Views idiom is, so I stole it from another file.) https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:118: return views::DialogDelegate::GetInitiallyFocusedView(); On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Shorter: > > auto* text_box = message_box_view_->text_box(); > return text_box ? text_box : views::DialogDelegate::GetInitiallyFocusedView(); ditto re stolen https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:19: class JavaScriptDialogViews : public views::DialogDelegate { On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Add description of what this class is and why we have it. Done. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:21: static base::WeakPtr<JavaScriptDialogViews> Create( On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Per https://google.github.io/styleguide/cppguide.html#Declaration_Order , > destructor goes above static method Done. https://codereview.chromium.org/2421943002/diff/80001/components/app_modal/ja... File components/app_modal/javascript_dialog_manager.h (right): https://codereview.chromium.org/2421943002/diff/80001/components/app_modal/ja... components/app_modal/javascript_dialog_manager.h:42: const GURL& origin_url); On 2016/10/17 20:16:05, Peter Kasting wrote: > Nit: Move definition order in .cc to match Done.
LGTM if you resolve the issue about Close() below. The other stuff is all minor commenting/style stuff. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:56: content::WebContents* alerting_web_contents, On 2016/10/18 02:46:41, Avi wrote: > On 2016/10/17 20:16:04, Peter Kasting wrote: > > Nit: Param name in .cc and .h should match > > Hm. > > The problem is that in this function, there are two (possibly different) > WebContentses in play: the alerting one, which is passed in, and the parent one, > which we get a pointer to later. > > If we match the .cc and .h names, then we have |parent_web_contents| and > |web_contents| which is awkward. OTOH, if we explicitly call the alerting > WebContents |alerting_web_contents|, like I do in this CL, then that > significantly clarifies things inside this function, but we pay the price of > having the .cc and .h param names not match. > > I think the clarity of the variable naming is worth the price of the param names > not matching. I'm willing to be overridden on this. Another option would simply be to declare a local: // Clearer variable name to avoid ambiguity below. content::WebContents* alerting_web_contents = web_contents; Another option would be to try to rename some of the other variables; maybe call |parent_web_contents| just |parent| (to me |parent| and |web_contents| are pretty distinct when reading, so hard to mentally mix up). https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:128: // was doing, but now the user can just close the page. On 2016/10/18 02:46:41, Avi wrote: > On 2016/10/17 20:16:04, Peter Kasting wrote: > > Nit: I sort of feel like this comment is really a TODO on the > > message-suppression logic in general, like "rip all this out when > auto-dismissal > > is on all the time". > > Unfortunately, the old path without auto-dismissal will be with us forever, or > until JavaScript dialogs are entirely removed. We still need that old path, > complete with "suppress further dialogs", for onbeforeunload dialogs, for > extension dialogs, and for WebContentses that aren't browser tabs. > > Given that the boolean |did_suppress_message| is going to stay forever in the > dialog API in which we're called here, I wanted to note why we return a constant > value. Ah. Maybe just update this comment to clarify this a little, e.g. "While we still need to provide message suppression for onbeforeunload dialogs, extension dialogs, and the like, we no longer need them for [phrase for the kind of dialogs we're handling here] because ..." https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:64: dialog_callback_.Reset(); On 2016/10/18 02:46:41, Avi wrote: > On 2016/10/17 20:16:05, Peter Kasting wrote: > > Why would we not call Close() instead? It's not obvious to me what the > > difference in intent of these two functions is, in part because the > declaration > > for this method in the header has no comment on it. > > Close() is a Views method, and is an implementation detail due to > JavaScriptDialogViews being derived from views::DialogDelegate. When I write the > Mac UI version, the callbacks are going to be different, so I want to have a > single "hey, dialog, go away" method on both. What I was more asking was, Close() runs the callback and passes false, but this function doesn't run the callback. I don't understand why this functional difference exists. Is it intentional? If so, add comments about it, which would presumably allude to the differing underlying purposes of each of these functions, which would be explained ( :D ) in comments on their declarations. If it's not intentional, then I'd call Close() here (and then GetWidget()->Close()? Or does that call Close() anyway?) directly to keep the two in sync. https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:79: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; On 2016/10/18 02:46:42, Avi wrote: > On 2016/10/17 20:16:05, Peter Kasting wrote: > > Nit: Shorter: > > > > const bool is_alert = message_type_ == > content::JAVASCRIPT_MESSAGE_TYPE_ALERT; > > return ui::DIALOG_BUTTON_OK | (is_alert ? 0 : ui::DIALOG_BUTTON_CANCEL); > > OK. > > (I don't know what Views idiom is, so I stole it from another file.) Dunno that we have a consistent idiom for this. Pretty much every way you could phrase this is in use in some places. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:38: Nit: Maybe remove blank line? https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:20: // JavaScriptDialogManager for them and handles displaying their dialogs. Nit: Might be nice to expand this some to talk about, or reference other comments/links about, the policies on auto-dismissing dialogs, not showing dialogs in non-foremost tabs, etc., as well as the high-level picture of what parts of this class implement what pieces of that. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:66: // user's input but by a call to |CloseDialog()|, this class will call it. Nit: No || on function names (the () is the marker that this is an identifier) https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:14: JavaScriptDialogViews::~JavaScriptDialogViews() {} Nit: Feel free to use "= default" in header to avoid this https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:20: // that are browser tabs. Nit: Maybe describe/link to what an "auto-dismissing dialog" is. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:35: // Close the dialog without sending a callback. Nit: Close -> Closes (see the "imperative" bit in http://google.github.io/styleguide/cppguide.html#Function_Comments ) https://codereview.chromium.org/2421943002/diff/100001/components/app_modal/j... File components/app_modal/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2421943002/diff/100001/components/app_modal/j... components/app_modal/javascript_dialog_manager.cc:110: #if !defined(OS_ANDROID) Nit: I know you're just moving this code... but could you reverse the arms of this #if? "#if !X...else" makes my head hurt.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:64: dialog_callback_.Reset(); On 2016/10/18 04:31:03, Peter Kasting wrote: > What I was more asking was, Close() runs the callback and passes false, but this > function doesn't run the callback. I don't understand why this functional > difference exists. Is it intentional? Yes; JavaScriptDialogManager's call CancelDialogs has a "suppress_callbacks" parameter. We need the ability to not do the callbacks. The other problem is that JavaScriptDialogManager has HandleJavaScriptDialog which requires the callback to be fired with custom values. If we want to have the view _always_ be the one to fire callbacks, then we have to plumb three values (suppress callback, custom result, custom user input) into the "close" method, and then we end up polluting the view class with lots of code that is cross-platform. This is the best solution I could find. Have the view class have an externally-visible "make this go away silently with no callbacks" call, and have the core tab helper do the work of figuring out whether or not to do callbacks and with what values depending on what call it gets on the JavaScriptDialogManager interface. > If so, add comments about it, which > would presumably allude to the differing underlying purposes of each of these > functions, which would be explained ( :D ) in comments on their declarations. But this is a simple design. The view has the ability to go away silently, and the tab helper handles the weirdness of the JavaScriptDialogManager interface. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:38: On 2016/10/18 04:31:03, Peter Kasting wrote: > Nit: Maybe remove blank line? Done. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:20: // JavaScriptDialogManager for them and handles displaying their dialogs. On 2016/10/18 04:31:03, Peter Kasting wrote: > Nit: Might be nice to expand this some to talk about, or reference other > comments/links about, the policies on auto-dismissing dialogs, not showing > dialogs in non-foremost tabs, etc., as well as the high-level picture of what > parts of this class implement what pieces of that. Done. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:66: // user's input but by a call to |CloseDialog()|, this class will call it. On 2016/10/18 04:31:03, Peter Kasting wrote: > Nit: No || on function names (the () is the marker that this is an identifier) Done. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:14: JavaScriptDialogViews::~JavaScriptDialogViews() {} On 2016/10/18 04:31:03, Peter Kasting wrote: > Nit: Feel free to use "= default" in header to avoid this Done. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:20: // that are browser tabs. On 2016/10/18 04:31:03, Peter Kasting wrote: > Nit: Maybe describe/link to what an "auto-dismissing dialog" is. Done. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:35: // Close the dialog without sending a callback. On 2016/10/18 04:31:03, Peter Kasting wrote: > Nit: Close -> Closes (see the "imperative" bit in > http://google.github.io/styleguide/cppguide.html#Function_Comments ) Done. https://codereview.chromium.org/2421943002/diff/100001/components/app_modal/j... File components/app_modal/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2421943002/diff/100001/components/app_modal/j... components/app_modal/javascript_dialog_manager.cc:110: #if !defined(OS_ANDROID) On 2016/10/18 04:31:04, Peter Kasting wrote: > Nit: I know you're just moving this code... but could you reverse the arms of > this #if? "#if !X...else" makes my head hurt. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Quick, submit now before I have a chance to meaninglessly wordsmith more things!! (Still LGTM) https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc (right): https://codereview.chromium.org/2421943002/diff/80001/chrome/browser/ui/javas... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc:64: dialog_callback_.Reset(); On 2016/10/18 16:31:49, Avi wrote: > On 2016/10/18 04:31:03, Peter Kasting wrote: > > What I was more asking was, Close() runs the callback and passes false, but > this > > function doesn't run the callback. I don't understand why this functional > > difference exists. Is it intentional? > > Yes; JavaScriptDialogManager's call CancelDialogs has a "suppress_callbacks" > parameter. We need the ability to not do the callbacks. The other problem is > that JavaScriptDialogManager has HandleJavaScriptDialog which requires the > callback to be fired with custom values. If we want to have the view _always_ be > the one to fire callbacks, then we have to plumb three values (suppress > callback, custom result, custom user input) into the "close" method, and then we > end up polluting the view class with lots of code that is cross-platform. > > This is the best solution I could find. Have the view class have an > externally-visible "make this go away silently with no callbacks" call, and have > the core tab helper do the work of figuring out whether or not to do callbacks > and with what values depending on what call it gets on the > JavaScriptDialogManager interface. Seems fine. > > If so, add comments about it, which > > would presumably allude to the differing underlying purposes of each of these > > functions, which would be explained ( :D ) in comments on their declarations. > > But this is a simple design. The view has the ability to go away silently, and > the tab helper handles the weirdness of the JavaScriptDialogManager interface. Right, I don't object to the design. I just suggest clarifying the understanding above for readers like me who do not necessarily know all this in their heads. For example, rename this function CloseDialogWithoutCallback() and write a comment in the header giving some of the explanation you just gave me about what this ends up being useful for, how it differs from the Close() method, and why that method should, in fact, call the callback. https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/100001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:66: // user's input but by a call to |CloseDialog()|, this class will call it. On 2016/10/18 16:31:49, Avi wrote: > On 2016/10/18 04:31:03, Peter Kasting wrote: > > Nit: No || on function names (the () is the marker that this is an identifier) > > Done. Oh, I was suggesting removing the || rather than removing the (). Sorry to be pedantic... I dunno quite how we got to the point where we like to put || around so many things. https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:23: // dialogs that close when the user switches away. Because JavaScript dialogs Nit: away -> to another tab? https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h (right): https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:20: // when the user switches away from interacting with the tab, used for Nit: switches away from interacting with the tab -> switches tabs? (I'm just worried that the "from interacting with" part sounds like maybe if I change to interacting with the omnibox or something, I've stopped interacting with the tab, so the dialog should dismiss.)
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2421943002/#ps140001 (title: "last fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Committing. BTW, I do expect someone modifying the JavaScriptDialogTabHelper code to know the basic JavaScriptDialogManager interface. https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h (right): https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h:23: // dialogs that close when the user switches away. Because JavaScript dialogs On 2016/10/18 16:59:55, Peter Kasting wrote: > Nit: away -> to another tab? Done. https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h (right): https://codereview.chromium.org/2421943002/diff/120001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h:20: // when the user switches away from interacting with the tab, used for On 2016/10/18 16:59:55, Peter Kasting wrote: > Nit: switches away from interacting with the tab -> switches tabs? (I'm just > worried that the "from interacting with" part sounds like maybe if I change to > interacting with the omnibox or something, I've stopped interacting with the > tab, so the dialog should dismiss.) Done.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by avi@chromium.org
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
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...)
The CQ bit was checked by avi@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 #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Make JavaScript dialogs auto-dismiss on tab switch. This is a Views-only implementation for now. This works on the Mac too, but is ugly; a real Mac version is to come. Behind the feature flag "AutoDismissingDialogs". BUG=629964 ========== to ========== Make JavaScript dialogs auto-dismiss on tab switch. This is a Views-only implementation for now. This works on the Mac too, but is ugly; a real Mac version is to come. Behind the feature flag "AutoDismissingDialogs". BUG=629964 Committed: https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29 Cr-Commit-Position: refs/heads/master@{#426133} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29 Cr-Commit-Position: refs/heads/master@{#426133}
Message was sent while issue was closed.
jochen@chromium.org changed reviewers: + jochen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2421943002/diff/140001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/140001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:110: parent_web_contents->GetDelegate()->ActivateContents(parent_web_contents); will this defeat the pop-under blocker? In the old code path, we've created an AppModalDialogHelper before activating the contents, so if there was an popup before the web contents got raised, it will be restored after the dialog closes.
Message was sent while issue was closed.
https://codereview.chromium.org/2421943002/diff/140001/chrome/browser/ui/java... File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): https://codereview.chromium.org/2421943002/diff/140001/chrome/browser/ui/java... chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:110: parent_web_contents->GetDelegate()->ActivateContents(parent_web_contents); On 2016/10/27 15:16:03, jochen wrote: > will this defeat the pop-under blocker? In the old code path, we've created an > AppModalDialogHelper before activating the contents, so if there was an popup > before the web contents got raised, it will be restored after the dialog closes. I was not aware of the interaction with the popup blocker and this code; thank you for bringing it to my attention. So that I understand, let me summarize what I think your concern is. The attack is to open a popup window and then show a JS alert, using the alert to raise the alerting window, in the hopes that the alerting window stays z-ordered above the popup window. The dialog helper captures the active window before activating the alerting window, and restores it when the alert is complete. I think we should talk, because that paragraph is the extent of my knowledge about the dialog helper, and I'm having trouble working out the interaction between the auto-dismissing behavior of these new JS alerts and the dialog helper. In particular, the new JS alerts auto-dismiss when you switch away from the alerting tab. If you switch away to a different tab in the same window, I think we're OK switching back to the previously-active window, but if you switch away to a different window, we can't lose the user's focus intent.
Message was sent while issue was closed.
On 2016/10/28 at 14:18:07, avi wrote: > https://codereview.chromium.org/2421943002/diff/140001/chrome/browser/ui/java... > File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc (right): > > https://codereview.chromium.org/2421943002/diff/140001/chrome/browser/ui/java... > chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:110: parent_web_contents->GetDelegate()->ActivateContents(parent_web_contents); > On 2016/10/27 15:16:03, jochen wrote: > > will this defeat the pop-under blocker? In the old code path, we've created an > > AppModalDialogHelper before activating the contents, so if there was an popup > > before the web contents got raised, it will be restored after the dialog closes. > > I was not aware of the interaction with the popup blocker and this code; thank you for bringing it to my attention. > > So that I understand, let me summarize what I think your concern is. > > The attack is to open a popup window and then show a JS alert, using the alert to raise the alerting window, in the hopes that the alerting window stays z-ordered above the popup window. The dialog helper captures the active window before activating the alerting window, and restores it when the alert is complete. > that's correct. However, there's one more thing: sites dismiss the prompt themselves, e.g., by triggering it via pdf and destroying the plugin element, so there's no user interaction required. > I think we should talk, because that paragraph is the extent of my knowledge about the dialog helper, and I'm having trouble working out the interaction between the auto-dismissing behavior of these new JS alerts and the dialog helper. In particular, the new JS alerts auto-dismiss when you switch away from the alerting tab. If you switch away to a different tab in the same window, I think we're OK switching back to the previously-active window, but if you switch away to a different window, we can't lose the user's focus intent. |
