Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808223002:
Add test for ContentSettingBubbleContents
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 8 months ago Reviewers:
msw CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd test for ContentSettingBubbleContents
This CL adds a regression test for the issue fixed in [1]. Namely, that
a missed layout causes the updated "done" button not to resize properly
when its text changes.
[1]: https://codereview.chromium.org/2804113003/
BUG=689992
Patch Set 1 #
Total comments: 38
Patch Set 2 : msw review #Patch Set 3 : include native_widget_types #Patch Set 4 : GetNativeWindow(), avoid implicit type #
Total comments: 5
Patch Set 5 : Add test for ContentSettingBubbleContents #Patch Set 6 : s/MoveMouseTo/MoveMouseToInHost/ #Patch Set 7 : set correct layout provider #
Messages
Total messages: 37 (29 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + msw@chromium.org
msw: Here is the unit test as promised. The portion I am most unsure about is the simulated click which entails iterating through the child views until I find the checkbox via it's class name... I confirmed that this test fails without the dependent CL because the actual width of the button does not update (only it's preferred width).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lots of minor comments suggesting possible ways to simplify, but generally this seems fine and you can ignore any suggestions that just complicate things... https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc (right): https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:10: #include "base/message_loop/message_loop.h" nit: probably not needed (transitive include should suffice) https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:14: #include "base/time/time.h" nit: remove this if you can use event generator https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:17: #include "chrome/browser/ui/content_settings/content_setting_image_model.h" nit: needed? https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:18: #include "chrome/browser/ui/test/test_confirm_bubble_model.h" nit: needed? https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:30: using views::Widget; nit: remove this, add one more explicit views:: namespace qualifier use. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:32: class TestContentSettingBubbleModel : public ContentSettingBubbleModel { nit: add a comment, describe the behavior of applying |done_button_text| when the checkbox is toggled. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:38: : ContentSettingBubbleModel(delegate, web_contents, profile), Can you pass null a web_contents and profile? https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:64: public ContentSettingBubbleModelDelegate { nit: make a separate test delegate class instead https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:67: : ui_thread_(content::BrowserThread::UI, message_loop()), q: Is this and |scoped_task_scheduler_| actually needed? https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:90: TestingProfile profile_; optional nit: if it's simpler, move the profile and web-contents members to TestContentSettingBubbleModel and reduce this test subclass to just a using statement: using ContentSettingBubbleContentsTest = views::ViewsTestBase; https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:98: // TODO(csharrison): Consider moving some functionality to the harness if more nit: remove this todo, it's understood. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:104: // Set up the root widget. nit: "Set up the root widget, needed to show the bubble." https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:107: views::View* view = new views::View(); nit: pass widget->GetContentsView() to the ctor instead. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:121: bubble_widget->Show(); optional nit: you can inline the widget like this: views::BubbleDialogDelegateView::CreateBubble(bubble_view)->Show(); https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:125: gfx::Size before_size = button->GetPreferredSize(); nit: use the actual width instead of the preferred. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:127: // Check the checkbox. This entails finding it in the view hierarchy via some You can optionally add a views::Checkbox* ContentSettingBubbleContents::manage_checkbox_for_testing() { return manage_checkbox_; } instead https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:131: if (base::StringPiece(child->GetClassName()) == "Checkbox") { q: is stringpiece actually needed? https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:133: ui::MouseEvent e(ui::ET_MOUSE_RELEASED, gfx::Point(0, 0), gfx::Point(), nit: use an EventGenerator instead https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:143: // Text should change, and the preferred width should change to match it. The optional nit: just say "The done button's text and size should change." and remove the preferred size check.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@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...
Thanks for the detailed review. It looks like the ContentSettingBubbleContents also doesn't really need a WebContents (at least, the behavior I am interested in testing). This simplifies the harness a lot because we don't need the ui_thread / task scheduler / web_contents_factory. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc (right): https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:10: #include "base/message_loop/message_loop.h" On 2017/04/11 01:19:52, msw wrote: > nit: probably not needed (transitive include should suffice) Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:14: #include "base/time/time.h" On 2017/04/11 01:19:52, msw wrote: > nit: remove this if you can use event generator Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:17: #include "chrome/browser/ui/content_settings/content_setting_image_model.h" On 2017/04/11 01:19:52, msw wrote: > nit: needed? Nope, removed. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:18: #include "chrome/browser/ui/test/test_confirm_bubble_model.h" On 2017/04/11 01:19:52, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:30: using views::Widget; On 2017/04/11 01:19:52, msw wrote: > nit: remove this, add one more explicit views:: namespace qualifier use. Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:32: class TestContentSettingBubbleModel : public ContentSettingBubbleModel { On 2017/04/11 01:19:52, msw wrote: > nit: add a comment, describe the behavior of applying |done_button_text| when > the checkbox is toggled. Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:38: : ContentSettingBubbleModel(delegate, web_contents, profile), On 2017/04/11 01:19:52, msw wrote: > Can you pass null a web_contents and profile? Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:64: public ContentSettingBubbleModelDelegate { On 2017/04/11 01:19:52, msw wrote: > nit: make a separate test delegate class instead Done. Now the test class is empty! https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:67: : ui_thread_(content::BrowserThread::UI, message_loop()), On 2017/04/11 01:19:52, msw wrote: > q: Is this and |scoped_task_scheduler_| actually needed? It's needed for the web contents factory, but it's removed now. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:90: TestingProfile profile_; On 2017/04/11 01:19:52, msw wrote: > optional nit: if it's simpler, move the profile and web-contents members to > TestContentSettingBubbleModel and reduce this test subclass to just a using > statement: using ContentSettingBubbleContentsTest = views::ViewsTestBase; Not needed, I was able to remove all the web contents stuff :P https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:98: // TODO(csharrison): Consider moving some functionality to the harness if more On 2017/04/11 01:19:52, msw wrote: > nit: remove this todo, it's understood. Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:104: // Set up the root widget. On 2017/04/11 01:19:53, msw wrote: > nit: "Set up the root widget, needed to show the bubble." Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:107: views::View* view = new views::View(); On 2017/04/11 01:19:52, msw wrote: > nit: pass widget->GetContentsView() to the ctor instead. Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:121: bubble_widget->Show(); On 2017/04/11 01:19:52, msw wrote: > optional nit: you can inline the widget like this: > views::BubbleDialogDelegateView::CreateBubble(bubble_view)->Show(); Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:125: gfx::Size before_size = button->GetPreferredSize(); On 2017/04/11 01:19:52, msw wrote: > nit: use the actual width instead of the preferred. Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:127: // Check the checkbox. This entails finding it in the view hierarchy via some On 2017/04/11 01:19:52, msw wrote: > You can optionally add a views::Checkbox* > ContentSettingBubbleContents::manage_checkbox_for_testing() { return > manage_checkbox_; } instead I think I prefer this way. Adding getters for any view we might want to pull out of the contents seems not sustainable. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:131: if (base::StringPiece(child->GetClassName()) == "Checkbox") { On 2017/04/11 01:19:52, msw wrote: > q: is stringpiece actually needed? IMO it is nicer style than using strncmp or something like that. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:133: ui::MouseEvent e(ui::ET_MOUSE_RELEASED, gfx::Point(0, 0), gfx::Point(), On 2017/04/11 01:19:52, msw wrote: > nit: use an EventGenerator instead Done. https://codereview.chromium.org/2808223002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:143: // Text should change, and the preferred width should change to match it. The On 2017/04/11 01:19:53, msw wrote: > optional nit: just say "The done button's text and size should change." and > remove the preferred size check. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
nice, lgtm with a nit and a q https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc (right): https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:71: typedef views::ViewsTestBase ContentSettingBubbleContentsTest; nit: using ContentSettingBubbleContentsTest = views::ViewsTestBase; https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:105: ui::test::EventGenerator event_generator(widget->GetNativeWindow()); q: Hmm, should this be bubble_view->GetWidget()->GetNativeView()?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc (right): https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:71: typedef views::ViewsTestBase ContentSettingBubbleContentsTest; On 2017/04/11 17:19:45, msw wrote: > nit: using ContentSettingBubbleContentsTest = views::ViewsTestBase; Done. https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:105: ui::test::EventGenerator event_generator(widget->GetNativeWindow()); On 2017/04/11 17:19:45, msw wrote: > q: Hmm, should this be bubble_view->GetWidget()->GetNativeView()? I think it needs to be bubble_view->GetWidget()->GetNativeWindow(), no? The constructor takes a native window, not a native view.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc (right): https://codereview.chromium.org/2808223002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents_unittest.cc:105: ui::test::EventGenerator event_generator(widget->GetNativeWindow()); On 2017/04/11 19:20:57, Charlie Harrison wrote: > On 2017/04/11 17:19:45, msw wrote: > > q: Hmm, should this be bubble_view->GetWidget()->GetNativeView()? > > I think it needs to be bubble_view->GetWidget()->GetNativeWindow(), no? The > constructor takes a native window, not a native view. Yup, typo on my part; just meant s/widget/bubble_view->GetWidget()/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hmm, it looks like the checkbox is not getting checked on Mac. Is there anything I'm missing? I don't normally do development on Mac so I'll have some additional latency for me to manually debug.
On 2017/04/11 20:28:03, Charlie Harrison wrote: > Hmm, it looks like the checkbox is not getting checked on Mac. Is there anything > I'm missing? > > I don't normally do development on Mac so I'll have some additional latency for > me to manually debug. I don't work on Mac much either, sorry. It looks like it does find the checkbox, but the event doesn't toggle the checked state? Hmm... Maybe ping mblsha@ for EventGenerator issues on Mac, or ping tapted@ for more general Mac Views questions.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
