Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
754953002:
Enable AutoResize for Constrained Web Dialogs (Closed)
DescriptionCurrently, constrained web dialogs cannot autoresize. This change gives an option to pass in a minimum and maximum size, which enables the autoresizing functionality.
OSX has not yet been implemented.
BUG=217034
Committed: https://crrev.com/bdcf05d84a94245a8e6dd0e7bcd08796a78a5a86
Cr-Commit-Position: refs/heads/master@{#308300}
Patch Set 1 : Initial #
Total comments: 42
Patch Set 2 : per miu's comments #
Total comments: 6
Patch Set 3 : per miu's comments #
Total comments: 12
Patch Set 4 : per bernhard bauer's comments #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 16
Patch Set 7 : Add browser test #
Total comments: 16
Patch Set 8 : Clean up test #
Total comments: 22
Patch Set 9 : Per msw's comments #Patch Set 10 : Move dialog border to constant #
Total comments: 12
Patch Set 11 : #Patch Set 12 : #
Total comments: 5
Patch Set 13 : #Patch Set 14 : Rename CreateConstrainedWebDialogWithAutoResize, update comment #
Total comments: 2
Patch Set 15 : #Patch Set 16 : #Messages
Total messages: 68 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
apacible@chromium.org changed reviewers: + miu@chromium.org
Patchset #1 (id:40001) has been deleted
Yuri, please take a look. Thanks!
Nothing too complex to fix here. My comments are all about unwinding things to simplify (write less code that does more): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:12: #include "components/web_modal/popup_manager.h" Is this used anywhere? https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:69: web_view_->SetBoundsRect(web_view_->bounds()); Is this call to SetBoundsRect() necessary? Seems like it should be a no-op (see implementation of SetBoundsRect in src/ui/views/view.cc). https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:72: gfx::Size GetPreferredSize() const { I don't think this method is needed. See comments for |delegate_| member (in the ConstrainedWebDialogDelegateViewViews class) below. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:79: gfx::Size preferred_size_; Don't need this if you don't need GetPreferredSize() (see prior comment). https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:143: min_size_ = gfx::Size(); In C++, both |min_size_| and |max_size_| are automatically initialized with their zero-arg constructor. So, you don't need to explicitly assign them here. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:147: ConstrainedWebDialogDelegateViewViews( nit: Add newline above this line. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:153: : views::WebView(browser_context), Chromium now allows C++11 delegated constructors, which allows you to greatly simplify things. In other words, I think you want the 2nd constructor to be: ConstrainedWebDialogDelegateViewViews( content::BrowserContext* browser_context, ui::WebDialogDelegate* delegate, content::WebContents* web_contents, gfx::Size min_size, gfx::Size max_size) : ConstrainedWebDialogDelegateViewViews(browser_context, delegate, web_contents), min_size_(min_size), max_size_(max_size) { DCHECK(!min_size.IsEmpty()); DCHECK(!max_size.IsEmpty()); } https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:189: void EnableAutoResize(const gfx::Size& min_size, const gfx::Size& max_size) This should be a private method. Also, you don't need the two arguments, since you can reference |min_size_| and |max_size_|. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:207: nit: Remove this newline. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:231: size = delegate_->GetPreferredSize(); If you unwind the code, this looks identical to: size = WebView::GetPreferredSize(); So, you don't need |delegate_|. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:239: return gfx::Size(); Should this method return min_size_ instead? Also, should you override View::GetMaximumSize() to return max_size_ if !max_size_.IsEmpty()? https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:260: content::WebContents* web_contents_; Don't need this. The code can always call the web_contents() method, which is inherited from WebView (which inherits from WebContentsObserver). https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:262: gfx::Size min_size_; Use 'const gfx::Size', since these are not modified after construction. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:265: bool auto_resize_enabled_; Don't need this extra bool member. The code could simply check whether !max_size_.IsEmpty() instead. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:267: WebDialogWebContentsDelegateViews* delegate_; Don't need this one, either. ;-) https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:269: void SetBoundsForAutoResize(const gfx::Size& min_size, Only the ctor calls this method. Assuming you agree with my suggestion above (about how to define the 2nd constructor), you can get rid of this method. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:304: constrained_window::CreateWebModalDialogViews(dialog, web_contents); Did you mean to call ShowWebModalDialogViews() instead? Or, does the showing happen later? If you did mean to call CreateWebModalDialogViews(), it seems like the other CreateConstrainedWebDialog() method should be renamed to ShowConstrainedWebDialog() for readability. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h (right): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h:47: void EnableAutoResize(const gfx::Size& min_size, This method should be removed in this class. See comments in constrained_web_dialog_ui.h. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_ui.h:12: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) Instead of including the header, just forward-declare the class (this keeps the compiling efficient for everyone): namespace gfx { class Size; } https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_ui.h:47: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) This shouldn't be a part of the interface since outside callers should not control when auto resize is enabled. In the impl class, as mentioned above, I think the method should be private. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_ui.h:98: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) Again, don't conditionally declare this function, either. It's fine if the function isn't implemented on Mac because there are no callers of it from any code path that runs on Mac. The linker will report an error should anyone try to do this in new code.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:12: #include "components/web_modal/popup_manager.h" On 2014/11/26 00:47:43, miu wrote: > Is this used anywhere? Nope, fixed. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:69: web_view_->SetBoundsRect(web_view_->bounds()); On 2014/11/26 00:47:43, miu wrote: > Is this call to SetBoundsRect() necessary? Seems like it should be a no-op (see > implementation of SetBoundsRect in src/ui/views/view.cc). No, and fixed. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:72: gfx::Size GetPreferredSize() const { On 2014/11/26 00:47:43, miu wrote: > I don't think this method is needed. See comments for |delegate_| member (in > the ConstrainedWebDialogDelegateViewViews class) below. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:79: gfx::Size preferred_size_; On 2014/11/26 00:47:43, miu wrote: > Don't need this if you don't need GetPreferredSize() (see prior comment). Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:143: min_size_ = gfx::Size(); On 2014/11/26 00:47:43, miu wrote: > In C++, both |min_size_| and |max_size_| are automatically initialized with > their zero-arg constructor. So, you don't need to explicitly assign them here. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:147: ConstrainedWebDialogDelegateViewViews( On 2014/11/26 00:47:43, miu wrote: > nit: Add newline above this line. Removed one constructor. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:153: : views::WebView(browser_context), On 2014/11/26 00:47:43, miu wrote: > Chromium now allows C++11 delegated constructors, which allows you to greatly > simplify things. In other words, I think you want the 2nd constructor to be: > > ConstrainedWebDialogDelegateViewViews( > content::BrowserContext* browser_context, > ui::WebDialogDelegate* delegate, > content::WebContents* web_contents, > gfx::Size min_size, > gfx::Size max_size) > : ConstrainedWebDialogDelegateViewViews(browser_context, delegate, > web_contents), > min_size_(min_size), > max_size_(max_size) { > DCHECK(!min_size.IsEmpty()); > DCHECK(!max_size.IsEmpty()); > } Combined both constructors into one. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:189: void EnableAutoResize(const gfx::Size& min_size, const gfx::Size& max_size) On 2014/11/26 00:47:43, miu wrote: > This should be a private method. Also, you don't need the two arguments, since > you can reference |min_size_| and |max_size_|. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:207: On 2014/11/26 00:47:43, miu wrote: > nit: Remove this newline. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:231: size = delegate_->GetPreferredSize(); On 2014/11/26 00:47:43, miu wrote: > If you unwind the code, this looks identical to: > > size = WebView::GetPreferredSize(); > > So, you don't need |delegate_|. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:239: return gfx::Size(); On 2014/11/26 00:47:43, miu wrote: > Should this method return min_size_ instead? > > Also, should you override View::GetMaximumSize() to return max_size_ if > !max_size_.IsEmpty()? Done. Talked offline about functionality of GetMaximumSize(). https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:260: content::WebContents* web_contents_; On 2014/11/26 00:47:43, miu wrote: > Don't need this. The code can always call the web_contents() method, which is > inherited from WebView (which inherits from WebContentsObserver). Talked offline -- keeping web_contents_ but renaming as initiator_web_contents_. GetWebContents() and web_contents() refer to WebContents from ConstrainedWebDialogDelegateBase, but this is referring to WebDialogWebContentsDelegateView's initiator WebContents. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:262: gfx::Size min_size_; On 2014/11/26 00:47:43, miu wrote: > Use 'const gfx::Size', since these are not modified after construction. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:265: bool auto_resize_enabled_; On 2014/11/26 00:47:44, miu wrote: > Don't need this extra bool member. The code could simply check whether > !max_size_.IsEmpty() instead. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:267: WebDialogWebContentsDelegateViews* delegate_; On 2014/11/26 00:47:43, miu wrote: > Don't need this one, either. ;-) Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:269: void SetBoundsForAutoResize(const gfx::Size& min_size, On 2014/11/26 00:47:43, miu wrote: > Only the ctor calls this method. Assuming you agree with my suggestion above > (about how to define the 2nd constructor), you can get rid of this method. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:304: constrained_window::CreateWebModalDialogViews(dialog, web_contents); On 2014/11/26 00:47:43, miu wrote: > Did you mean to call ShowWebModalDialogViews() instead? Or, does the showing > happen later? > > If you did mean to call CreateWebModalDialogViews(), it seems like the other > CreateConstrainedWebDialog() method should be renamed to > ShowConstrainedWebDialog() for readability. The showing happens later. I will rename the former method. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h (right): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h:47: void EnableAutoResize(const gfx::Size& min_size, On 2014/11/26 00:47:44, miu wrote: > This method should be removed in this class. See comments in > constrained_web_dialog_ui.h. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_ui.h:12: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) On 2014/11/26 00:47:44, miu wrote: > Instead of including the header, just forward-declare the class (this keeps the > compiling efficient for everyone): > > namespace gfx { > class Size; > } Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_ui.h:47: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) On 2014/11/26 00:47:44, miu wrote: > This shouldn't be a part of the interface since outside callers should not > control when auto resize is enabled. In the impl class, as mentioned above, I > think the method should be private. Done. https://codereview.chromium.org/754953002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/constrained_web_dialog_ui.h:98: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) On 2014/11/26 00:47:44, miu wrote: > Again, don't conditionally declare this function, either. It's fine if the > function isn't implemented on Mac because there are no callers of it from any > code path that runs on Mac. The linker will report an error should anyone try > to do this in new code. Done.
Changes per miu's comments.
Looks great! Just a few tweaks left: https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:134: initiator_web_contents_ = web_contents; Please put this in the initializer list above. https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:223: content::WebContents* initiator_web_contents_; Should be: content::WebContents* const initiator_web_contents_; Since this pointer is not changed after construction. https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:94: ConstrainedWebDialogDelegate* CreateConstrainedWebDialogWithAutoResize( Needs a short comment. Mention that this only creates the dialog, with the intention that the caller should show the dialog by calling constrained_window::ShowWebModalDialogViews() after the document load is completed, to avoid UI jank.
https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:134: initiator_web_contents_ = web_contents; On 2014/11/26 22:17:36, miu wrote: > Please put this in the initializer list above. Done. https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:223: content::WebContents* initiator_web_contents_; On 2014/11/26 22:17:36, miu wrote: > Should be: > > content::WebContents* const initiator_web_contents_; > > Since this pointer is not changed after construction. Done. https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:94: ConstrainedWebDialogDelegate* CreateConstrainedWebDialogWithAutoResize( On 2014/11/26 22:17:36, miu wrote: > Needs a short comment. Mention that this only creates the dialog, with the > intention that the caller should show the dialog by calling > constrained_window::ShowWebModalDialogViews() after the document load is > completed, to avoid UI jank. Done.
apacible@chromium.org changed reviewers: + alekseys@chromium.org, avi@chromium.org, bauerb@chromium.org, bcwhite@chromium.org, msw@chromium.org
Adding OWNERS for review: +alekseys for print_preview_dialog_controller.cc +avi for constrained_web_dialog_delegate_mac.mm +bauerb for certificate_viewer_webui.cc, constrained_web_dialog_ui.h, constrained_web_dialog_ui_browsertest.cc +bcwhite for profile_signin_confirmation_dialog.cc +msw for constrained_web_dialog_delegate_views.cc
On 2014/11/26 22:53:03, apacible wrote: > +avi for constrained_web_dialog_delegate_mac.mm This LGTM
> +bcwhite for profile_signin_confirmation_dialog.cc LGTM
https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:124: gfx::Size min_size, Pass by const reference? https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:222: content::WebContents* const initiator_web_contents_; You want the pointer value to be const? Usually we don't bother with that. https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:230: DCHECK(render_view_host); This is somewhat unnecessary, as you will crash on the next line anyway if the |render_view_host| is nullptr. https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:14: class Size; Don't indent inside of namespaces. https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:94: // Creates a constrained HTML dialog with auto-resize enabled. The dialog Nit: Either use imperative style ("create") like above, or change both to be descriptive.
https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:222: content::WebContents* const initiator_web_contents_; On 2014/11/27 12:41:34, Bernhard Bauer wrote: > You want the pointer value to be const? Usually we don't bother with that. I asked for this change on a prior round of review. It's both good practice and in the C++ style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const Using const here lets the compiler catch erroneous attempts to change the value. Also, using const improves readability, in that anyone who modifies this code in the future won't have to look through the entire implementation to determine whether |initiator_web_contents_| is set anywhere but the ctor. http://gamesfromwithin.com/the-const-nazi
https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:124: gfx::Size min_size, On 2014/11/27 12:41:34, Bernhard Bauer wrote: > Pass by const reference? Done. https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:230: DCHECK(render_view_host); On 2014/11/27 12:41:33, Bernhard Bauer wrote: > This is somewhat unnecessary, as you will crash on the next line anyway if the > |render_view_host| is nullptr. Done. https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:14: class Size; On 2014/11/27 12:41:34, Bernhard Bauer wrote: > Don't indent inside of namespaces. Done. https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:94: // Creates a constrained HTML dialog with auto-resize enabled. The dialog On 2014/11/27 12:41:34, Bernhard Bauer wrote: > Nit: Either use imperative style ("create") like above, or change both to be > descriptive. Done.
https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:124: gfx::Size min_size, On 2014/12/01 17:35:57, apacible wrote: > On 2014/11/27 12:41:34, Bernhard Bauer wrote: > > Pass by const reference? > > Done. Const *reference* please.
https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:124: gfx::Size min_size, On 2014/12/01 17:46:04, Bernhard Bauer wrote: > On 2014/12/01 17:35:57, apacible wrote: > > On 2014/11/27 12:41:34, Bernhard Bauer wrote: > > > Pass by const reference? > > > > Done. > > Const *reference* please. Oops, fixed.
LGTM!
miu@chromium.org changed reviewers: + sky@chromium.org - msw@chromium.org
sky: msw@'s out. Can you PTAL at constrained_web_dialog_delegate_views.cc?
lgtm
I don't see test coverage of the autoresize. Can you write test coverage? https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:129: initiator_web_contents_(web_contents), What is the lifetime of this WebContents? https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:227: void EnableAutoResize() { functions before members. https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:254: gfx::Size min_size, const gfx::Size&
On 2014/11/26 22:53:03, apacible wrote: > +alekseys for print_preview_dialog_controller.cc lgtm
Patchset #6 (id:220001) has been deleted
msw@chromium.org changed reviewers: + msw@chromium.org
Please add some tests for this new functionality. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/754953002/diff/240001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:374: new PrintPreviewDialogDelegate(initiator), nit: fix indent... https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:26: class InitiatorWebContentsObserver Why do you need to subclass content::WebContentsObserver? https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:32: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:84: InitiatorWebContentsObserver* const observer_; nit: Use a more descriptive name, like |initiator_observer_|. (ditto for all the argument plumbing for this member) https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:149: } Remove this incorrect indentation change. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:205: size = WebView::GetPreferredSize(); I'm worried that this might be conflating the sizes of the web content and the dialog hosting the web content, it'd be nice if you added some comments to make the meaning of all the sizes used in this CL more explicit. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:242: InitiatorWebContentsObserver observer_; ditto nit: Use a more descriptive name, like |initiator_observer_|. (ditto for all the argument plumbing for this member) https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/webui... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/certificate_viewer_webui.cc:232: web_contents); nit: fix indent...
On 2014/12/04 19:27:19, msw wrote: > Please add some tests for this new functionality. > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/printing... > File chrome/browser/printing/print_preview_dialog_controller.cc (right): > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/printing... > chrome/browser/printing/print_preview_dialog_controller.cc:374: new > PrintPreviewDialogDelegate(initiator), > nit: fix indent... > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:26: class > InitiatorWebContentsObserver > Why do you need to subclass content::WebContentsObserver? > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:32: }; > nit: DISALLOW_COPY_AND_ASSIGN > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:84: > InitiatorWebContentsObserver* const observer_; > nit: Use a more descriptive name, like |initiator_observer_|. > (ditto for all the argument plumbing for this member) > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:149: } > Remove this incorrect indentation change. > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:205: size = > WebView::GetPreferredSize(); > I'm worried that this might be conflating the sizes of the web content and the > dialog hosting the web content, it'd be nice if you added some comments to make > the meaning of all the sizes used in this CL more explicit. > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... > chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:242: > InitiatorWebContentsObserver observer_; > ditto nit: Use a more descriptive name, like |initiator_observer_|. > (ditto for all the argument plumbing for this member) > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): > > https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/webui... > chrome/browser/ui/webui/certificate_viewer_webui.cc:232: web_contents); > nit: fix indent... Oops, didn't mean to publish the last patch or round of comment responses yet. I'm working on browser tests right now. Are there current unit tests for the constrained web dialog? I couldn't find them but want to make sure I'm not duplicating a unit test file...
On 2014/12/04 19:36:50, apacible wrote: > Oops, didn't mean to publish the last patch or round of comment responses yet. > I'm working on browser tests right now. Are there current unit tests for the > constrained web dialog? I couldn't find them but want to make sure I'm not > duplicating a unit test file... I don't think there are unit tests, it's too tightly bound to actually showing dialogs, etc.
https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:129: initiator_web_contents_(web_contents), On 2014/12/02 15:55:40, sky wrote: > What is the lifetime of this WebContents? The WebContents are destroyed before ConstrainedWebDialogDelegateViewViews teardown. I made a modification to use a WebContentsObserver to keep track of whether the WebContents still exist. https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:227: void EnableAutoResize() { On 2014/12/02 15:55:40, sky wrote: > functions before members. Done. https://codereview.chromium.org/754953002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:254: gfx::Size min_size, On 2014/12/02 15:55:40, sky wrote: > const gfx::Size& Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/754953002/diff/240001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller.cc:374: new PrintPreviewDialogDelegate(initiator), On 2014/12/04 19:27:19, msw wrote: > nit: fix indent... Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:26: class InitiatorWebContentsObserver On 2014/12/04 19:27:19, msw wrote: > Why do you need to subclass content::WebContentsObserver? The WebContentsObserver constructor is protected. To create a new WebContentsObserver, I subclassed it so I can call a public constructor. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:32: }; On 2014/12/04 19:27:19, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:84: InitiatorWebContentsObserver* const observer_; On 2014/12/04 19:27:19, msw wrote: > nit: Use a more descriptive name, like |initiator_observer_|. > (ditto for all the argument plumbing for this member) Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:149: } On 2014/12/04 19:27:19, msw wrote: > Remove this incorrect indentation change. Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:205: size = WebView::GetPreferredSize(); On 2014/12/04 19:27:19, msw wrote: > I'm worried that this might be conflating the sizes of the web content and the > dialog hosting the web content, it'd be nice if you added some comments to make > the meaning of all the sizes used in this CL more explicit. Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:242: InitiatorWebContentsObserver observer_; On 2014/12/04 19:27:19, msw wrote: > ditto nit: Use a more descriptive name, like |initiator_observer_|. > (ditto for all the argument plumbing for this member) Done. https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/webui... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/754953002/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/certificate_viewer_webui.cc:232: web_contents); On 2014/12/04 19:27:19, msw wrote: > nit: fix indent... Done.
sky, msw: Added browser tests. PTAL. Thanks!
https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:27: class InitiatorWebContentsObserver Sorry, I must be crazy, but what code actually employs observation of the web contents? As far as I can tell, holding a pointer to the initiator_web_contents would be sufficient and no actual WebContentsObserver is required. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:75: if (!initiator_observer_->web_contents()) Should this check that the source matches the initiator? https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:211: // Size is set here if the dialog has been auto-resized in nit: "The size" https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:215: // Size set here if dialog has not been auto-resized or nit: "The size is set" and "if the dialog" https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:261: // Minimum and maximum sizes to determine dialog bounds if nit: "bounds for auto-resizing." for a one-liner. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:105: // |browser_context| is used to construct the constrained HTML dialog's nit: remove "constrained HTML", it's implicit.
Some test comments. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:49: bool IsEqualSizes(gfx::Size expected, Why does this need to be a ConstrainedWebDialogBrowserTest member? Can this just be an anonymous namespace function in this test file and then the Bind calls can avoid using base::Unretained(this) on the test class instance. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:55: bool RunLoopUntil(const base::Callback<bool()>& condition) { This seems like a bad practice that may yield flaky failures and poor test performance. Can these tests employ a web contents observer or similar instead? https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:143: WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate( nit: break line after assignment operator https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:175: base::string16 ready_title(base::ASCIIToUTF16("ready")); Is there no better pattern to observe webcontents loading in tests? In the same browser_test_utils.h I see WaitForLoadStop*, etc. (avoiding a wait would be nice, but not necessary, I suppose...) https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:182: // Dialog size to WebContents size + 16 for both width and height. This seems fragile, can this value be determined within the test? https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:225: std::string url = "data:text/html,<!doctype html>" nit: make this a shared local/class static const char kTestDataURL[] or similar. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:267: ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize()); Shouldn't there theoretically be the same wait/callback mechanism to ensure that the initiator web contents has resized and run its callback to offer the dialog the chance to resize (and then test that it was not, in fact, resized?). Otherwise, I think you could make this dialog resizable and this test would still pass.
https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:27: class InitiatorWebContentsObserver On 2014/12/09 19:51:50, msw wrote: > Sorry, I must be crazy, but what code actually employs observation of the web > contents? As far as I can tell, holding a pointer to the initiator_web_contents > would be sufficient and no actual WebContentsObserver is required. I'm currently using the InitiatorWebContentsObserver in 3 checks to see if the web contents still exists. Originally, I just held a pointer to the initiator_web_contents_. From digging into the lifetime of the initiator_web_contents_ (per sky's earlier comment), I found that the web contents can potentially be destroyed before a call to use the web contents is made (e.g. during teardown with pending resizing call). That's why I changed the pointer to the use of an observer. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:75: if (!initiator_observer_->web_contents()) On 2014/12/09 19:51:50, msw wrote: > Should this check that the source matches the initiator? The source and initiator WebContents are not the same WebContents. The source WebContents is the same thing as the ConstrainedWebDialogDelegateViewView's GetWebContents() and set in ConstrainedWebDialogDelegateBase as a new web contents. The initiator web contents is the web contents passed into the ConstrainedWebDialogDelegateViewViews constructor. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:211: // Size is set here if the dialog has been auto-resized in On 2014/12/09 19:51:50, msw wrote: > nit: "The size" Done. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:215: // Size set here if dialog has not been auto-resized or On 2014/12/09 19:51:50, msw wrote: > nit: "The size is set" and "if the dialog" Done. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:261: // Minimum and maximum sizes to determine dialog bounds if On 2014/12/09 19:51:50, msw wrote: > nit: "bounds for auto-resizing." for a one-liner. Done. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:105: // |browser_context| is used to construct the constrained HTML dialog's On 2014/12/09 19:51:50, msw wrote: > nit: remove "constrained HTML", it's implicit. Done. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:49: bool IsEqualSizes(gfx::Size expected, On 2014/12/09 20:34:41, msw wrote: > Why does this need to be a ConstrainedWebDialogBrowserTest member? Can this just > be an anonymous namespace function in this test file and then the Bind calls can > avoid using base::Unretained(this) on the test class instance. Done. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:55: bool RunLoopUntil(const base::Callback<bool()>& condition) { On 2014/12/09 20:34:41, msw wrote: > This seems like a bad practice that may yield flaky failures and poor test > performance. Can these tests employ a web contents observer or similar instead? The dialog resizing is asynchronous, so we want to block any assert calls until the resizing is done. RunLoopUntil will prevent flakiness since the sizing will definitely be done before any asserts are called. /cc miu, who suggested this. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:143: WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate( On 2014/12/09 20:34:41, msw wrote: > nit: break line after assignment operator Done. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:175: base::string16 ready_title(base::ASCIIToUTF16("ready")); On 2014/12/09 20:34:41, msw wrote: > Is there no better pattern to observe webcontents loading in tests? > In the same browser_test_utils.h I see WaitForLoadStop*, etc. > (avoiding a wait would be nice, but not necessary, I suppose...) I tried using WaitForLoadStop*, but it doesn't always wait until all of the WebContents has actually loaded. I consistently got "Uncaught ReferenceError: changeDimensions is not defined" since the rest of the test was trying to run before the entire WebContents has loaded. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:182: // Dialog size to WebContents size + 16 for both width and height. On 2014/12/09 20:34:41, msw wrote: > This seems fragile, can this value be determined within the test? I haven't found a way to determine the value yet, but still looking into this... https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:225: std::string url = "data:text/html,<!doctype html>" On 2014/12/09 20:34:41, msw wrote: > nit: make this a shared local/class static const char kTestDataURL[] or similar. Done. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:267: ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize()); On 2014/12/09 20:34:41, msw wrote: > Shouldn't there theoretically be the same wait/callback mechanism to ensure that > the initiator web contents has resized and run its callback to offer the dialog > the chance to resize (and then test that it was not, in fact, resized?). > Otherwise, I think you could make this dialog resizable and this test would > still pass. Done.
https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:55: bool RunLoopUntil(const base::Callback<bool()>& condition) { On 2014/12/10 18:39:29, apacible wrote: > On 2014/12/09 20:34:41, msw wrote: > > This seems like a bad practice that may yield flaky failures and poor test > > performance. Can these tests employ a web contents observer or similar > instead? > > The dialog resizing is asynchronous, so we want to block any assert calls until > the resizing is done. RunLoopUntil will prevent flakiness since the sizing will > definitely be done before any asserts are called. > > /cc miu, who suggested this. To be clear, the resizing is initiated by the render process. So, there's no way to know when that resize message will reach the browser process and then hop from the IO thread to a UI thread task. The code here simply blocks the caller, running the UI MessageLoop until that expected task has posted and been run.
https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:182: // Dialog size to WebContents size + 16 for both width and height. On 2014/12/10 18:39:29, apacible wrote: > On 2014/12/09 20:34:41, msw wrote: > > This seems fragile, can this value be determined within the test? > > I haven't found a way to determine the value yet, but still looking into this... Per offline conversation with msw -- made the dialog border (16) into a constant in patch 10.
Took another look after all the changes. Still lgtm. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:27: class InitiatorWebContentsObserver nit: Maybe add a comment that this WCO is just tracking the lifetime of the WebContents to avoid use-after-destroy possibilites.
https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:27: class InitiatorWebContentsObserver On 2014/12/10 22:31:15, miu wrote: > nit: Maybe add a comment that this WCO is just tracking the lifetime of the > WebContents to avoid use-after-destroy possibilites. Done.
https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:27: class InitiatorWebContentsObserver On 2014/12/10 18:39:28, apacible wrote: > On 2014/12/09 19:51:50, msw wrote: > > Sorry, I must be crazy, but what code actually employs observation of the web > > contents? As far as I can tell, holding a pointer to the > initiator_web_contents > > would be sufficient and no actual WebContentsObserver is required. > > I'm currently using the InitiatorWebContentsObserver in 3 checks to see if the > web contents still exists. > > Originally, I just held a pointer to the initiator_web_contents_. From digging > into the lifetime of the initiator_web_contents_ (per sky's earlier comment), I > found that the web contents can potentially be destroyed before a call to use > the web contents is made (e.g. during teardown with pending resizing call). > That's why I changed the pointer to the use of an observer. Please add an explanatory comment, otherwise it seems like an oversight. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:75: if (!initiator_observer_->web_contents()) On 2014/12/10 18:39:28, apacible wrote: > On 2014/12/09 19:51:50, msw wrote: > > Should this check that the source matches the initiator? > > The source and initiator WebContents are not the same WebContents. The source > WebContents is the same thing as the ConstrainedWebDialogDelegateViewView's > GetWebContents() and set in ConstrainedWebDialogDelegateBase as a new web > contents. > > The initiator web contents is the web contents passed into the > ConstrainedWebDialogDelegateViewViews constructor. Sorry, should this check that the |source| WebContents matches the dialog's WebContents? https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:55: bool RunLoopUntil(const base::Callback<bool()>& condition) { On 2014/12/10 19:28:22, miu wrote: > On 2014/12/10 18:39:29, apacible wrote: > > On 2014/12/09 20:34:41, msw wrote: > > > This seems like a bad practice that may yield flaky failures and poor test > > > performance. Can these tests employ a web contents observer or similar > > instead? > > > > The dialog resizing is asynchronous, so we want to block any assert calls > until > > the resizing is done. RunLoopUntil will prevent flakiness since the sizing > will > > definitely be done before any asserts are called. > > > > /cc miu, who suggested this. > > To be clear, the resizing is initiated by the render process. So, there's no > way to know when that resize message will reach the browser process and then hop > from the IO thread to a UI thread task. The code here simply blocks the caller, > running the UI MessageLoop until that expected task has posted and been run. I was hoping this could use something like WaitForResizeComplete but I'm not an expert here, so I'll defer to you and others. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:143: WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate( On 2014/12/10 18:39:29, apacible wrote: > On 2014/12/09 20:34:41, msw wrote: > > nit: break line after assignment operator > > Done. Ping (not actually done). https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:175: base::string16 ready_title(base::ASCIIToUTF16("ready")); On 2014/12/10 18:39:29, apacible wrote: > On 2014/12/09 20:34:41, msw wrote: > > Is there no better pattern to observe webcontents loading in tests? > > In the same browser_test_utils.h I see WaitForLoadStop*, etc. > > (avoiding a wait would be nice, but not necessary, I suppose...) > > I tried using WaitForLoadStop*, but it doesn't always wait until all of the > WebContents has actually loaded. I consistently got "Uncaught ReferenceError: > changeDimensions is not defined" since the rest of the test was trying to run > before the entire WebContents has loaded. There's probably some mechanism (maybe a test util?) to wait until scripts are loaded, but I don't know where to look for that offhand. Why don't you just pass the inlined script in your call to ExecuteScript, rather than including the script in the data URL's HTML? Then you can probably use WaitForLoadStop instead of the more roundabout TitleWatcher. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:146: WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate( nit: break line after assignment operator. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:188: gfx::Size(150 + kDialogBorderSpace, 150 + kDialogBorderSpace), optional nit: define "const int initial_size = 150 + kDialogBorderSpace;" above. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:198: gfx::Size(175 + kDialogBorderSpace, 175 + kDialogBorderSpace), optional nit: define "const int new_size = 175 + kDialogBorderSpace;" above. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:258: ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize()); Is this still needed? Doesn't the above statement ensure the same thing? https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:267: ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize()); Ditto: Is this still needed? Doesn't the above statement ensure the same thing?
https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right): https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:27: class InitiatorWebContentsObserver On 2014/12/11 01:13:57, msw wrote: > On 2014/12/10 18:39:28, apacible wrote: > > On 2014/12/09 19:51:50, msw wrote: > > > Sorry, I must be crazy, but what code actually employs observation of the > web > > > contents? As far as I can tell, holding a pointer to the > > initiator_web_contents > > > would be sufficient and no actual WebContentsObserver is required. > > > > I'm currently using the InitiatorWebContentsObserver in 3 checks to see if the > > web contents still exists. > > > > Originally, I just held a pointer to the initiator_web_contents_. From digging > > into the lifetime of the initiator_web_contents_ (per sky's earlier comment), > I > > found that the web contents can potentially be destroyed before a call to use > > the web contents is made (e.g. during teardown with pending resizing call). > > That's why I changed the pointer to the use of an observer. > > Please add an explanatory comment, otherwise it seems like an oversight. Added in patch 11. https://codereview.chromium.org/754953002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:75: if (!initiator_observer_->web_contents()) On 2014/12/11 01:13:57, msw wrote: > On 2014/12/10 18:39:28, apacible wrote: > > On 2014/12/09 19:51:50, msw wrote: > > > Should this check that the source matches the initiator? > > > > The source and initiator WebContents are not the same WebContents. The source > > WebContents is the same thing as the ConstrainedWebDialogDelegateViewView's > > GetWebContents() and set in ConstrainedWebDialogDelegateBase as a new web > > contents. > > > > The initiator web contents is the web contents passed into the > > ConstrainedWebDialogDelegateViewViews constructor. > > Sorry, should this check that the |source| WebContents matches the dialog's > WebContents? Ah, yes. Added. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:55: bool RunLoopUntil(const base::Callback<bool()>& condition) { On 2014/12/11 01:13:57, msw wrote: > On 2014/12/10 19:28:22, miu wrote: > > On 2014/12/10 18:39:29, apacible wrote: > > > On 2014/12/09 20:34:41, msw wrote: > > > > This seems like a bad practice that may yield flaky failures and poor test > > > > performance. Can these tests employ a web contents observer or similar > > > instead? > > > > > > The dialog resizing is asynchronous, so we want to block any assert calls > > until > > > the resizing is done. RunLoopUntil will prevent flakiness since the sizing > > will > > > definitely be done before any asserts are called. > > > > > > /cc miu, who suggested this. > > > > To be clear, the resizing is initiated by the render process. So, there's no > > way to know when that resize message will reach the browser process and then > hop > > from the IO thread to a UI thread task. The code here simply blocks the > caller, > > running the UI MessageLoop until that expected task has posted and been run. > > I was hoping this could use something like WaitForResizeComplete but I'm not an > expert here, so I'll defer to you and others. I looked into WaitForResizeComplete, but it's aura only. I didn't find anything else similar that I could use here. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:143: WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate( On 2014/12/11 01:13:57, msw wrote: > On 2014/12/10 18:39:29, apacible wrote: > > On 2014/12/09 20:34:41, msw wrote: > > > nit: break line after assignment operator > > > > Done. > > Ping (not actually done). Oops, sorry. Done. https://codereview.chromium.org/754953002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:175: base::string16 ready_title(base::ASCIIToUTF16("ready")); On 2014/12/11 01:13:57, msw wrote: > On 2014/12/10 18:39:29, apacible wrote: > > On 2014/12/09 20:34:41, msw wrote: > > > Is there no better pattern to observe webcontents loading in tests? > > > In the same browser_test_utils.h I see WaitForLoadStop*, etc. > > > (avoiding a wait would be nice, but not necessary, I suppose...) > > > > I tried using WaitForLoadStop*, but it doesn't always wait until all of the > > WebContents has actually loaded. I consistently got "Uncaught ReferenceError: > > changeDimensions is not defined" since the rest of the test was trying to run > > before the entire WebContents has loaded. > > There's probably some mechanism (maybe a test util?) to wait until scripts are > loaded, but I don't know where to look for that offhand. Why don't you just pass > the inlined script in your call to ExecuteScript, rather than including the > script in the data URL's HTML? Then you can probably use WaitForLoadStop instead > of the more roundabout TitleWatcher. Done. I moved the JS to a call in ExecuteScript and removed it from the URL's HTML. I also changed TitleWatcher to WaitForLoadStop. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:146: WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate( On 2014/12/11 01:13:57, msw wrote: > nit: break line after assignment operator. Done. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:188: gfx::Size(150 + kDialogBorderSpace, 150 + kDialogBorderSpace), On 2014/12/11 01:13:57, msw wrote: > optional nit: define "const int initial_size = 150 + kDialogBorderSpace;" above. Done. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:198: gfx::Size(175 + kDialogBorderSpace, 175 + kDialogBorderSpace), On 2014/12/11 01:13:57, msw wrote: > optional nit: define "const int new_size = 175 + kDialogBorderSpace;" above. Done. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:258: ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize()); On 2014/12/11 01:13:57, msw wrote: > Is this still needed? Doesn't the above statement ensure the same thing? Done. Also changed for the other test. https://codereview.chromium.org/754953002/diff/320001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:267: ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize()); On 2014/12/11 01:13:57, msw wrote: > Ditto: Is this still needed? Doesn't the above statement ensure the same thing? Done.
lgtm with a nit and a q. https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:37: static const int initial_size = 150 + kDialogBorderSpace; nit: move these into the scope of ContentResizeInAutoResizingDialog. (drop |static|s; ditto for kDialogBorderSpace, renamed to dialog_border_space). https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:183: ASSERT_TRUE(IsShowingWebContentsModalDialog(web_contents)); q: Where is the call to actually show the dialog?
Patchset #13 (id:380001) has been deleted
Thanks! https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:37: static const int initial_size = 150 + kDialogBorderSpace; On 2014/12/11 23:30:59, msw wrote: > nit: move these into the scope of ContentResizeInAutoResizingDialog. > (drop |static|s; ditto for kDialogBorderSpace, renamed to dialog_border_space). Done. https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:183: ASSERT_TRUE(IsShowingWebContentsModalDialog(web_contents)); On 2014/12/11 23:30:59, msw wrote: > q: Where is the call to actually show the dialog? The dialog is shown when the WebContents load (L181) from this call: ConstrainedWebDialogDelegateViewViews->DocumentOnLoadCompletedInMainFrame.
https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc (right): https://codereview.chromium.org/754953002/diff/360001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc:183: ASSERT_TRUE(IsShowingWebContentsModalDialog(web_contents)); On 2014/12/12 23:02:09, apacible wrote: > On 2014/12/11 23:30:59, msw wrote: > > q: Where is the call to actually show the dialog? > > The dialog is shown when the WebContents load (L181) from this call: > ConstrainedWebDialogDelegateViewViews->DocumentOnLoadCompletedInMainFrame. Hmm, it's odd that this code only ever calls create, not show, and the dialog is shown automatically on load... Should CreateConstrainedWebDialogWithAutoResize be renamed to Show* if the dialog is always automatically shown?
> > > q: Where is the call to actually show the dialog? > > > > The dialog is shown when the WebContents load (L181) from this call: > > ConstrainedWebDialogDelegateViewViews->DocumentOnLoadCompletedInMainFrame. > > Hmm, it's odd that this code only ever calls create, not show, and the dialog is > shown automatically on load... Should CreateConstrainedWebDialogWithAutoResize > be renamed to Show* if the dialog is always automatically shown? Good question. I originally named it Create* since the function calls CreateWebModalDialogViews instead of ShowWebModalDialogViews. However, the end result should be a creation of the dialog after the WebContents has loaded. I could either rename it to Show* or add a comment that the dialog will be shown after onload. What would be easier for maintenance?
On 2014/12/12 23:30:01, apacible wrote: > > > > q: Where is the call to actually show the dialog? > > > > > > The dialog is shown when the WebContents load (L181) from this call: > > > ConstrainedWebDialogDelegateViewViews->DocumentOnLoadCompletedInMainFrame. > > > > Hmm, it's odd that this code only ever calls create, not show, and the dialog > is > > shown automatically on load... Should CreateConstrainedWebDialogWithAutoResize > > be renamed to Show* if the dialog is always automatically shown? > > Good question. I originally named it Create* since the function calls > CreateWebModalDialogViews instead of ShowWebModalDialogViews. However, the end > result should be a creation of the dialog after the WebContents has loaded. I > could either rename it to Show* or add a comment that the dialog will be shown > after onload. What would be easier for maintenance? I vote for a rename and a comment to that effect with the function declaration.
On 2014/12/13 00:04:31, msw wrote: > On 2014/12/12 23:30:01, apacible wrote: > > > > > q: Where is the call to actually show the dialog? > > > > > > > > The dialog is shown when the WebContents load (L181) from this call: > > > > ConstrainedWebDialogDelegateViewViews->DocumentOnLoadCompletedInMainFrame. > > > > > > Hmm, it's odd that this code only ever calls create, not show, and the > dialog > > is > > > shown automatically on load... Should > CreateConstrainedWebDialogWithAutoResize > > > be renamed to Show* if the dialog is always automatically shown? > > > > Good question. I originally named it Create* since the function calls > > CreateWebModalDialogViews instead of ShowWebModalDialogViews. However, the end > > result should be a creation of the dialog after the WebContents has loaded. I > > could either rename it to Show* or add a comment that the dialog will be shown > > after onload. What would be easier for maintenance? > > I vote for a rename and a comment to that effect with the function declaration. Renamed and modified a comment in the function declaration. The changes in chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc were from rebasing. (Should have uploaded another diff for that.)
lgtm with a nit, thanks! https://codereview.chromium.org/754953002/diff/420001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/420001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:102: // Create a constrained HTML dialog with auto-resize enabled. The dialog nit: Create and show
https://codereview.chromium.org/754953002/diff/420001/chrome/browser/ui/webui... File chrome/browser/ui/webui/constrained_web_dialog_ui.h (right): https://codereview.chromium.org/754953002/diff/420001/chrome/browser/ui/webui... chrome/browser/ui/webui/constrained_web_dialog_ui.h:102: // Create a constrained HTML dialog with auto-resize enabled. The dialog On 2014/12/13 01:23:48, msw wrote: > nit: Create and show Done.
The CQ bit was checked by apacible@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/754953002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2014/12/13 03:43:29, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_compile_dbg_ng on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) Since OSX hasn't been implemented yet, is it preferable to: a) Put ifdefs around the unimplemented virtual methods (GetMinimumSize/GetMaximumSize/GetPreferredSize), similar to how the browser test is currently set up, or b) Add the methods to ConstrainedWebDialogDelegateViewMac as NOTREACHED(), similar to how ConstrainedWebDialogDelegateBase is written?
On 2014/12/13 05:11:00, apacible wrote: > On 2014/12/13 03:43:29, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > mac_chromium_compile_dbg_ng on tryserver.chromium.mac > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > > Since OSX hasn't been implemented yet, is it preferable to: > a) Put ifdefs around the unimplemented virtual methods > (GetMinimumSize/GetMaximumSize/GetPreferredSize), similar to how the browser > test is currently set up, or > b) Add the methods to ConstrainedWebDialogDelegateViewMac as NOTREACHED(), > similar to how ConstrainedWebDialogDelegateBase is written? Option B, but use NOTIMPLEMENTED().
Patchset #16 (id:460001) has been deleted
Patchset #16 (id:480001) has been deleted
On 2014/12/14 04:32:32, miu wrote: > On 2014/12/13 05:11:00, apacible wrote: > > On 2014/12/13 03:43:29, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > mac_chromium_compile_dbg_ng on tryserver.chromium.mac > > > > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > > > > Since OSX hasn't been implemented yet, is it preferable to: > > a) Put ifdefs around the unimplemented virtual methods > > (GetMinimumSize/GetMaximumSize/GetPreferredSize), similar to how the browser > > test is currently set up, or > > b) Add the methods to ConstrainedWebDialogDelegateViewMac as NOTREACHED(), > > similar to how ConstrainedWebDialogDelegateBase is written? > > Option B, but use NOTIMPLEMENTED(). Updated and ran try bots.
The CQ bit was checked by miu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/754953002/500001
Message was sent while issue was closed.
Committed patchset #16 (id:500001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/bdcf05d84a94245a8e6dd0e7bcd08796a78a5a86 Cr-Commit-Position: refs/heads/master@{#308300} |
