Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(209)

Issue 1414203005: Add flag to TableView to control if selection is automatically updated (Closed)

Created:
5 years, 1 month ago by juncai
Modified:
5 years, 1 month ago
Reviewers:
sky
CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, Reilly Grant (use Gerrit)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add flag to TableView to control if selection is automatically updated when the selected item is removed. This patch added a member variable select_on_remove_ to TableView to control if selection is automatically updated when the selected item is removed. This is useful for permission chooser which uses TableView, since when user chooses an item to grant permission, if that selected item is then removed from the list, there should be no item automatically selected to grant permission before user makes another choice. BUG=492204 Committed: https://crrev.com/9d1653ef4155ed241f01c5aa7ff2203a561dc4c7 Cr-Commit-Position: refs/heads/master@{#357918}

Patch Set 1 : added a member variable move_selection_on_removal_ to TableView #

Total comments: 2

Patch Set 2 : use set_select_on_remove function to set the variable #

Patch Set 3 : updated test function names #

Total comments: 2

Patch Set 4 : removed some test code #

Total comments: 2

Patch Set 5 : use nullptr instead of NULL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -1 line) Patch
M ui/views/controls/table/table_view.h View 1 2 chunks +11 lines, -0 lines 0 comments Download
M ui/views/controls/table/table_view.cc View 1 2 chunks +2 lines, -1 line 0 comments Download
M ui/views/controls/table/table_view_unittest.cc View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (6 generated)
juncai
sky@chromium.org: Please review this patch.
5 years, 1 month ago (2015-11-03 20:32:28 UTC) #3
tfarina
Is there a crbug associated to this? This is seem the kind of work based ...
5 years, 1 month ago (2015-11-03 20:38:07 UTC) #4
juncai
On 2015/11/03 20:38:07, tfarina wrote: > Is there a crbug associated to this? This is ...
5 years, 1 month ago (2015-11-03 20:45:38 UTC) #6
sky
https://codereview.chromium.org/1414203005/diff/1/ui/views/controls/table/table_view.h File ui/views/controls/table/table_view.h (right): https://codereview.chromium.org/1414203005/diff/1/ui/views/controls/table/table_view.h#newcode96 ui/views/controls/table/table_view.h:96: bool move_selection_on_removal); No need for another obscure boolean. Add ...
5 years, 1 month ago (2015-11-03 23:33:10 UTC) #7
juncai
https://codereview.chromium.org/1414203005/diff/1/ui/views/controls/table/table_view.h File ui/views/controls/table/table_view.h (right): https://codereview.chromium.org/1414203005/diff/1/ui/views/controls/table/table_view.h#newcode96 ui/views/controls/table/table_view.h:96: bool move_selection_on_removal); On 2015/11/03 23:33:09, sky wrote: > No ...
5 years, 1 month ago (2015-11-04 01:48:19 UTC) #9
sky
The TableView change is fine, I question where we need the extend of tests you're ...
5 years, 1 month ago (2015-11-04 16:50:34 UTC) #10
juncai
On 2015/11/04 16:50:34, sky wrote: > The TableView change is fine, I question where we ...
5 years, 1 month ago (2015-11-04 17:15:49 UTC) #11
sky
Exactly. On Wed, Nov 4, 2015 at 9:15 AM, <juncai@chromium.org> wrote: > On 2015/11/04 16:50:34, ...
5 years, 1 month ago (2015-11-04 17:16:39 UTC) #12
juncai
Sure, I will update the code. Thanks.
5 years, 1 month ago (2015-11-04 17:18:01 UTC) #13
juncai
https://codereview.chromium.org/1414203005/diff/40001/ui/views/controls/table/table_view_unittest.cc File ui/views/controls/table/table_view_unittest.cc (right): https://codereview.chromium.org/1414203005/diff/40001/ui/views/controls/table/table_view_unittest.cc#newcode575 ui/views/controls/table/table_view_unittest.cc:575: // remove 2 -> 0 1 [2] On 2015/11/04 ...
5 years, 1 month ago (2015-11-04 19:14:51 UTC) #14
sky
LGTM https://codereview.chromium.org/1414203005/diff/60001/ui/views/controls/table/table_view_unittest.cc File ui/views/controls/table/table_view_unittest.cc (right): https://codereview.chromium.org/1414203005/diff/60001/ui/views/controls/table/table_view_unittest.cc#newcode621 ui/views/controls/table/table_view_unittest.cc:621: table_->SetObserver(NULL); nullptr
5 years, 1 month ago (2015-11-04 20:52:19 UTC) #15
juncai
https://codereview.chromium.org/1414203005/diff/60001/ui/views/controls/table/table_view_unittest.cc File ui/views/controls/table/table_view_unittest.cc (right): https://codereview.chromium.org/1414203005/diff/60001/ui/views/controls/table/table_view_unittest.cc#newcode621 ui/views/controls/table/table_view_unittest.cc:621: table_->SetObserver(NULL); On 2015/11/04 20:52:19, sky wrote: > nullptr Done.
5 years, 1 month ago (2015-11-04 22:14:12 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414203005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414203005/80001
5 years, 1 month ago (2015-11-04 22:15:22 UTC) #19
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 1 month ago (2015-11-04 22:45:00 UTC) #20
commit-bot: I haz the power
5 years, 1 month ago (2015-11-04 22:45:53 UTC) #21
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/9d1653ef4155ed241f01c5aa7ff2203a561dc4c7
Cr-Commit-Position: refs/heads/master@{#357918}

Powered by Google App Engine
This is Rietveld 408576698