Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2679533002:
Added unit test for WebShareTargetPickerView. (Closed)
|
Created:
3 years, 10 months ago by Matt Giuca Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded unit test for WebShareTargetPickerView.
Also minor fix to avoid a DCHECK if double-clicking on an empty table
(which I don't think was possible to trigger anyway).
BUG=668389
Review-Url: https://codereview.chromium.org/2679533002
Cr-Commit-Position: refs/heads/master@{#449210}
Committed: https://chromium.googlesource.com/chromium/src/+/c5166d7cb235178146eee3431f7f64f0afc8aa4f
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address sky review. #Patch Set 3 : Rebase (conflict with r448512; totally broken). #Patch Set 4 : Fix conflicts (change tests) and use a proper ViewsTestBase. #Patch Set 5 : Rebase on CL 2682753004. Fixes Windows crash. #Patch Set 6 : Rebase. #Patch Set 7 : Small behaviour change: If nothing is selected, don't accept. #Patch Set 8 : Test selection and double-click logic. #
Total comments: 3
Patch Set 9 : Remove double-click on empty list logic + test. #
Depends on Patchset: Messages
Total messages: 31 (19 generated)
The CQ bit was checked by mgiuca@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...
mgiuca@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, Tests as you requested in https://codereview.chromium.org/2667803002/ (for the current version of the picker which just takes a list of names and doesn't return the chosen target). Connie will rebase on top of this and change the tests for the updated interface.
mgiuca@chromium.org changed reviewers: + constantina@google.com
+constantina
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:50: table_ = new views::TableView(this, table_columns, views::TEXT_ONLY, true); To avoid errors please initialize table_ to null in the member initializer list. https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc (right): https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:12: #include "chrome/browser/ui/views/webshare/webshare_target_picker_view.h" This should be your first include (just like web_share_target_picker_view.cc). https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:22: class WebShareTargetPickerViewTest : public ChromeRenderViewHostTestHarness { Based on capitalization all of the files in this directory should be web_share, not webshare_. https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:57: void callback(SharePickerResult result) { OnCallback? https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:68: }; DISALLOW...
Patchset #3 (id:40001) has been deleted
Thanks for the review. https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:50: table_ = new views::TableView(this, table_columns, views::TEXT_ONLY, true); On 2017/02/06 17:11:24, sky wrote: > To avoid errors please initialize table_ to null in the member initializer list. Done (in the .h file). https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc (right): https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:12: #include "chrome/browser/ui/views/webshare/webshare_target_picker_view.h" On 2017/02/06 17:11:24, sky wrote: > This should be your first include (just like web_share_target_picker_view.cc). Done. https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:22: class WebShareTargetPickerViewTest : public ChromeRenderViewHostTestHarness { On 2017/02/06 17:11:24, sky wrote: > Based on capitalization all of the files in this directory should be web_share, > not webshare_. Hmm, that's a good point, but we have directories and files called "webshare" in a LOT of places (c/b/webshare, c/b/ui/views/webshare, WebKit/public/platform/modules/webshare, android/java/src/org/chromium/chrome/browser/webshare/, etc, plus all the files within). I think for consistency that ship has sailed, though perhaps at some point we should rename all the classes to Webshare instead of WebShare. Can we not do this now? Connie and myself both have a few CLs out that will conflict if we make major changes. https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:57: void callback(SharePickerResult result) { On 2017/02/06 17:11:24, sky wrote: > OnCallback? Done. https://codereview.chromium.org/2679533002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc:68: }; On 2017/02/06 17:11:24, sky wrote: > DISALLOW... Done.
LGTM
The CQ bit was checked by mgiuca@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by mgiuca@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...
Description was changed from ========== Added unit test for WebShareTargetPickerView. BUG=668389 ========== to ========== Added unit test for WebShareTargetPickerView. Also minor fix to avoid a DCHECK if double-clicking on an empty table (which I don't think was possible to trigger anyway). BUG=668389 ==========
Hi Scott, FYI this test implementation has changed quite a bit since you looked at it (it's now a proper ViewsTestBase and actually instantiates the widget). Let me know if you have further comments, otherwise I'll just land it after the parent CL https://codereview.chromium.org/2682753004/. ty!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2679533002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2679533002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:135: if (table_->selection_model().empty()) How do we end up here with an empty selection? Doesn't the IsDialogButtonEnabled() override below make it so accept isn't called when there isn't a selection?
https://codereview.chromium.org/2679533002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2679533002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:135: if (table_->selection_model().empty()) On 2017/02/08 17:03:44, sky wrote: > How do we end up here with an empty selection? Doesn't the > IsDialogButtonEnabled() override below make it so accept isn't called when there > isn't a selection? I suspect this is because your test is directly calling OnDoubleClick. TableView only calls OnDoubleClick if there is a selection. I think the DCHECK should remain and the test should be updated.
https://codereview.chromium.org/2679533002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right): https://codereview.chromium.org/2679533002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:135: if (table_->selection_model().empty()) On 2017/02/08 17:06:51, sky wrote: > On 2017/02/08 17:03:44, sky wrote: > > How do we end up here with an empty selection? Doesn't the > > IsDialogButtonEnabled() override below make it so accept isn't called when > there > > isn't a selection? > > I suspect this is because your test is directly calling OnDoubleClick. TableView > only calls OnDoubleClick if there is a selection. I think the DCHECK should > remain and the test should be updated. Oh OK, I wasn't sure if that was guaranteed or if there was a way for OnDoubleClick to be triggered when there wasn't a selection. So I'll simply remove the test of double-clicking while there is no selection (under the assumption that that can never happen).
LGTM
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486606706031560,
"parent_rev": "88019c0dd11547f36d8a3f80c280652dfd58c1d9", "commit_rev":
"c5166d7cb235178146eee3431f7f64f0afc8aa4f"}
Message was sent while issue was closed.
Description was changed from ========== Added unit test for WebShareTargetPickerView. Also minor fix to avoid a DCHECK if double-clicking on an empty table (which I don't think was possible to trigger anyway). BUG=668389 ========== to ========== Added unit test for WebShareTargetPickerView. Also minor fix to avoid a DCHECK if double-clicking on an empty table (which I don't think was possible to trigger anyway). BUG=668389 Review-Url: https://codereview.chromium.org/2679533002 Cr-Commit-Position: refs/heads/master@{#449210} Committed: https://chromium.googlesource.com/chromium/src/+/c5166d7cb235178146eee3431f7f... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/c5166d7cb235178146eee3431f7f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
