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

Issue 2820693004: shelf: Allow dragging items from main shelf to overflow shelf. (Closed)

Created:
3 years, 8 months ago by sammiequon
Modified:
3 years, 7 months ago
CC:
chromium-reviews, kalyank, sadrul, Mr4D (OOO till 08-26)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

shelf: Allow dragging items from main shelf to overflow shelf. Previously only overflow shelf to main shelf, now bidirectional. TEST=ash_unittests --gtest_filter="ShelfViewTest.*" BUG=710228 Review-Url: https://codereview.chromium.org/2820693004 Cr-Commit-Position: refs/heads/master@{#469368} Committed: https://chromium.googlesource.com/chromium/src/+/31438b99f6f743976583ce02a4a362c26b9e144e

Patch Set 1 #

Total comments: 29

Patch Set 2 : Fixed patch set 1 errors. #

Total comments: 2

Patch Set 3 : Fix patch set 2 errors. #

Total comments: 7

Patch Set 4 : Rebased. #

Patch Set 5 : Rebased. #

Patch Set 6 : Fixed patch set 3 errors. #

Patch Set 7 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+172 lines, -66 lines) Patch
M ash/shelf/shelf_view.h View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M ash/shelf/shelf_view.cc View 1 2 3 4 5 6 8 chunks +62 lines, -23 lines 0 comments Download
M ash/shelf/shelf_view_unittest.cc View 1 2 3 4 5 6 5 chunks +97 lines, -39 lines 0 comments Download
M ash/test/shelf_view_test_api.h View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M ash/test/shelf_view_test_api.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 41 (22 generated)
sammiequon
msw@ - Please take a look. Thanks!
3 years, 8 months ago (2017-04-17 17:18:57 UTC) #3
msw
https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#newcode1085 ash/shelf/shelf_view.cc:1085: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) q: shouldn't this definitely ...
3 years, 8 months ago (2017-04-17 19:29:10 UTC) #4
sammiequon
https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#newcode1085 ash/shelf/shelf_view.cc:1085: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) On 2017/04/17 19:29:09, msw ...
3 years, 8 months ago (2017-04-17 23:42:29 UTC) #5
msw
lgtm with clarified comments and an optional DCHECK nit. Please seek review from a shelf ...
3 years, 8 months ago (2017-04-18 00:11:04 UTC) #6
sammiequon
Thanks! +skuhne@ skuhne@ - Are you familiar with the dragging shelf items around code? Please ...
3 years, 8 months ago (2017-04-18 18:57:45 UTC) #8
sammiequon
On 2017/04/18 18:57:45, sammiequon wrote: > Thanks! > > +skuhne@ > > skuhne@ - Are ...
3 years, 7 months ago (2017-04-27 16:51:26 UTC) #9
skuhne
A few comments. Mainly nits. https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc#newcode1083 ash/shelf/shelf_view.cc:1083: main_shelf_->EndDrag(true); could you please ...
3 years, 7 months ago (2017-05-03 00:56:18 UTC) #11
sammiequon
https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc#newcode1083 ash/shelf/shelf_view.cc:1083: main_shelf_->EndDrag(true); On 2017/05/03 00:56:18, skuhne wrote: > could you ...
3 years, 7 months ago (2017-05-03 16:19:19 UTC) #12
Mr4D (OOO till 08-26)
lgtm https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc#newcode1248 ash/shelf/shelf_view.cc:1248: } Super. Thanks!
3 years, 7 months ago (2017-05-03 17:30:00 UTC) #13
sammiequon
On 2017/05/03 17:30:00, Mr4D (IO delays) wrote: > lgtm > > https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc > File ash/shelf/shelf_view.cc ...
3 years, 7 months ago (2017-05-03 17:49:31 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820693004/100001
3 years, 7 months ago (2017-05-03 17:50:23 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/426492)
3 years, 7 months ago (2017-05-03 18:01:41 UTC) #19
sammiequon
On 2017/05/03 18:01:41, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 7 months ago (2017-05-03 18:07:12 UTC) #22
Daniel Erat
lgtm for //ash/test (which i assume is all you needed me to look at)
3 years, 7 months ago (2017-05-03 20:41:28 UTC) #26
sammiequon
On 2017/05/03 20:41:28, Daniel Erat wrote: > lgtm for //ash/test (which i assume is all ...
3 years, 7 months ago (2017-05-03 21:14:49 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820693004/100001
3 years, 7 months ago (2017-05-03 21:16:10 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/426772)
3 years, 7 months ago (2017-05-03 21:25:25 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820693004/120001
3 years, 7 months ago (2017-05-04 17:08:51 UTC) #38
commit-bot: I haz the power
3 years, 7 months ago (2017-05-04 17:15:13 UTC) #41
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/31438b99f6f743976583ce02a4a3...

Powered by Google App Engine
This is Rietveld 408576698