|
|
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. |
Descriptionshelf: 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. #
Messages
Total messages: 41 (22 generated)
Description was changed from ========== 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 ========== to ========== 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 ==========
sammiequon@chromium.org changed reviewers: + msw@chromium.org
msw@ - Please take a look. Thanks!
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#new... ash/shelf/shelf_view.cc:1085: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) q: shouldn't this definitely be true if we are dragging between shelves, and |is_overflow_mode()| is false? (should this be a DCHECK instead?) https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1110: // Item is dragged from overflow shelf to shelf. nit: "The item was dragged from the overflow shelf to the main shelf." https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1123: // Item is dragged from shelf to overflow shelf. The item was dragged from the main shelf to the overflow shelf. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1139: } else if (overflow_bubble_ && overflow_bubble_->IsShowing()) { ditto q: shouldn't this definitely be true if we are dragging between shelves, and |is_overflow_mode()| is false? (should this be a DCHECK instead?) https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1141: // After entering the overflow shelf the last item becomes visible I don't understand what is happening here (I also don't know this dragging code terribly well), you should ping skuhne@ or someone else more familiar with this code. Why does hiding the last item in the overflow shelf help? Are you saying that item always matches the dragged item here (what happens when you're dragging a non-last-item?)? Where do we reset the last_visible_index_ to the correct value? This doesn't make sense to me... https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1144: overflow_bubble_->shelf_view()->last_visible_index_ = nit: overflow_bubble_->shelf_view()->last_visible_index_--; https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1174: // If the overflow bubble is showing, the current item will swap to the Ditto, I'm a bit confused by this... current/last/dragged, can you try to explain this a bit more/differently? https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1178: overflow_bubble_->shelf_view()->last_visible_index_ = nit: overflow_bubble_->shelf_view()->last_visible_index_--; (and remove {}) https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1219: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) ditto q: shouldn't this definitely be true if we are dragging between shelves, and |is_overflow_mode()| is false? (should this be a DCHECK instead?) https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1241: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) ditto q: shouldn't this definitely be true if we are dragging between shelves, and |is_overflow_mode()| is false? (should this be a DCHECK instead?) https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.h File ash/shelf/shelf_view.h (right): https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.h#newc... ash/shelf/shelf_view.h:458: // True when ripped item from overflow bubble is entered into Shelf or vice nit: // True when an item is dragged from one shelf to another (eg. overflow). https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.h#newc... ash/shelf/shelf_view.h:460: bool dragged_off_from_shelf_to_other_shelf_; This name is pretty bad; how about moving this just below |dragged_off_shelf_| and using the name |dragged_to_another_shelf_| or |dragged_from_another_shelf_| or similar? https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view_unitte... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view_unitte... ash/shelf/shelf_view_unittest.cc:651: // To insert at |drop_index|, more smaller x-axis value of |drop_point| nit: s/more/a/ here and below "a smaller" and "a larger". https://codereview.chromium.org/2820693004/diff/1/ash/test/shelf_view_test_api.h File ash/test/shelf_view_test_api.h (right): https://codereview.chromium.org/2820693004/diff/1/ash/test/shelf_view_test_ap... ash/test/shelf_view_test_api.h:119: // Returns true if an item is ripped off one shelf and entered into other nit: // True when an item is dragged from one shelf to another (eg. overflow). https://codereview.chromium.org/2820693004/diff/1/ash/test/shelf_view_test_ap... ash/test/shelf_view_test_api.h:121: bool DraggedItemFromShelfToOtherShelf(); nit: rename to match member name
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#new... ash/shelf/shelf_view.cc:1085: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) On 2017/04/17 19:29:09, msw wrote: > q: shouldn't this definitely be true if we are dragging between shelves, and > |is_overflow_mode()| is false? (should this be a DCHECK instead?) Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1110: // Item is dragged from overflow shelf to shelf. On 2017/04/17 19:29:09, msw wrote: > nit: "The item was dragged from the overflow shelf to the main shelf." Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1123: // Item is dragged from shelf to overflow shelf. On 2017/04/17 19:29:09, msw wrote: > The item was dragged from the main shelf to the overflow shelf. Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1141: // After entering the overflow shelf the last item becomes visible On 2017/04/17 19:29:09, msw wrote: > I don't understand what is happening here (I also don't know this dragging code > terribly well), you should ping skuhne@ or someone else more familiar with this > code. Why does hiding the last item in the overflow shelf help? Are you saying > that item always matches the dragged item here (what happens when you're > dragging a non-last-item?)? Where do we reset the last_visible_index_ to the > correct value? This doesn't make sense to me... If we do not hide the last item, when an item is ripped off, you'll see a copy of it under the cursor, as well as the same item in the last spot of the overflow shelf (because the overflow shelf does not disappear when the main shelf is clicked anymore). The last item should also match the dragged item because we swap it to the back before starting our drag phase. last_visible_index_ gets updated when shelf items get rearranged again (ie when the dragged item enters a shelf etc.). So if our model has 13 items (and lets say 6 should be overflow), both shelfs will have all 13 items except the main shelf will have first_visible_index=0 and last_visible_index=6, while the overflow shelf will have first_visible_index=7 and last_visible_index=12. So when rip an item off the main shelf below (lets say item at index 3), we swap it will the last index in the model. The items on the shelf then get redraw according to the model. The chosen item is now index 12 and will not be displayed on the main shelf, but it will be displayed on the overflow shelf as the last item. This is actually a bug introduced in my last CL I believe because previously the overflow shelf would disappear when an item on the main shelf was chosen so it didn't matter). To compensate for this we set the last_visible_index of the overflow to 11, so that the shelf will get redrawn without the duplicate (because the ripped of item is also shown where the cursor is). Let me know if that clears things up :-), before I attempt to put it in comment form. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1144: overflow_bubble_->shelf_view()->last_visible_index_ = On 2017/04/17 19:29:09, msw wrote: > nit: overflow_bubble_->shelf_view()->last_visible_index_--; Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1178: overflow_bubble_->shelf_view()->last_visible_index_ = On 2017/04/17 19:29:09, msw wrote: > nit: overflow_bubble_->shelf_view()->last_visible_index_--; (and remove {}) Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1219: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) On 2017/04/17 19:29:09, msw wrote: > ditto q: shouldn't this definitely be true if we are dragging between shelves, > and |is_overflow_mode()| is false? (should this be a DCHECK instead?) Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.cc#new... ash/shelf/shelf_view.cc:1241: else if (overflow_bubble_ && overflow_bubble_->IsShowing()) On 2017/04/17 19:29:09, msw wrote: > ditto q: shouldn't this definitely be true if we are dragging between shelves, > and |is_overflow_mode()| is false? (should this be a DCHECK instead?) Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.h File ash/shelf/shelf_view.h (right): https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.h#newc... ash/shelf/shelf_view.h:458: // True when ripped item from overflow bubble is entered into Shelf or vice On 2017/04/17 19:29:09, msw wrote: > nit: // True when an item is dragged from one shelf to another (eg. overflow). Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view.h#newc... ash/shelf/shelf_view.h:460: bool dragged_off_from_shelf_to_other_shelf_; On 2017/04/17 19:29:09, msw wrote: > This name is pretty bad; how about moving this just below |dragged_off_shelf_| > and using the name |dragged_to_another_shelf_| or |dragged_from_another_shelf_| > or similar? Done. https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view_unitte... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2820693004/diff/1/ash/shelf/shelf_view_unitte... ash/shelf/shelf_view_unittest.cc:651: // To insert at |drop_index|, more smaller x-axis value of |drop_point| On 2017/04/17 19:29:10, msw wrote: > nit: s/more/a/ here and below "a smaller" and "a larger". Done. https://codereview.chromium.org/2820693004/diff/1/ash/test/shelf_view_test_api.h File ash/test/shelf_view_test_api.h (right): https://codereview.chromium.org/2820693004/diff/1/ash/test/shelf_view_test_ap... ash/test/shelf_view_test_api.h:119: // Returns true if an item is ripped off one shelf and entered into other On 2017/04/17 19:29:10, msw wrote: > nit: // True when an item is dragged from one shelf to another (eg. overflow). Done. https://codereview.chromium.org/2820693004/diff/1/ash/test/shelf_view_test_ap... ash/test/shelf_view_test_api.h:121: bool DraggedItemFromShelfToOtherShelf(); On 2017/04/17 19:29:10, msw wrote: > nit: rename to match member name Done.
lgtm with clarified comments and an optional DCHECK nit. Please seek review from a shelf dragging expert (skuhne@?). 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#new... ash/shelf/shelf_view.cc:1141: // After entering the overflow shelf the last item becomes visible On 2017/04/17 23:42:29, sammiequon wrote: > On 2017/04/17 19:29:09, msw wrote: > > I don't understand what is happening here (I also don't know this dragging > code > > terribly well), you should ping skuhne@ or someone else more familiar with > this > > code. Why does hiding the last item in the overflow shelf help? Are you saying > > that item always matches the dragged item here (what happens when you're > > dragging a non-last-item?)? Where do we reset the last_visible_index_ to the > > correct value? This doesn't make sense to me... > > If we do not hide the last item, when an item is ripped off, you'll see a copy > of it under the cursor, as well as the same item in the last spot of the > overflow shelf (because the overflow shelf does not disappear when the main > shelf is clicked anymore). > The last item should also match the dragged item because we swap it to the back > before starting our drag phase. > last_visible_index_ gets updated when shelf items get rearranged again (ie when > the dragged item enters a shelf etc.). > So if our model has 13 items (and lets say 6 should be overflow), both shelfs > will have all 13 items except the main shelf will have first_visible_index=0 and > last_visible_index=6, while the overflow shelf will have first_visible_index=7 > and last_visible_index=12. So when rip an item off the main shelf below (lets > say item at index 3), we swap it will the last index in the model. The items on > the shelf then get redraw according to the model. The chosen item is now index > 12 and will not be displayed on the main shelf, but it will be displayed on the > overflow shelf as the last item. This is actually a bug introduced in my last CL > I believe because previously the overflow shelf would disappear when an item on > the main shelf was chosen so it didn't matter). To compensate for this we set > the last_visible_index of the overflow to 11, so that the shelf will get redrawn > without the duplicate (because the ripped of item is also shown where the cursor > is). > > Let me know if that clears things up :-), before I attempt to put it in comment > form. Ah, I didn't realize that when we started dragging an item, it's moved to the end of the list; that clears up most of my confusion. I don't see exactly where the |last_visible_index_| members are update after the drag completes, but there are plenty of places that seem to recalculate it, so I'll trust you on that... Please do try to clarify the comments here and on line 1175, and it would still be good to loop in a more knowledgeable reviewer. https://codereview.chromium.org/2820693004/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1085: DCHECK(overflow_bubble_ && overflow_bubble_->IsShowing()); nit: just DCHECK(overflow_bubble_->IsShowing()); that'll disambiguate crashes when |overflow_bubble_| is null from DCHECK failures when it's not showing. (ditto for the other similar DCHECKS below)
sammiequon@chromium.org changed reviewers: + skuhne@chromium.org
Thanks! +skuhne@ skuhne@ - Are you familiar with the dragging shelf items around code? Please take a look. Thanks! https://codereview.chromium.org/2820693004/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2820693004/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1085: DCHECK(overflow_bubble_ && overflow_bubble_->IsShowing()); On 2017/04/18 00:11:04, msw wrote: > nit: just DCHECK(overflow_bubble_->IsShowing()); that'll disambiguate crashes > when |overflow_bubble_| is null from DCHECK failures when it's not showing. > (ditto for the other similar DCHECKS below) Done.
On 2017/04/18 18:57:45, sammiequon wrote: > Thanks! > > +skuhne@ > > skuhne@ - Are you familiar with the dragging shelf items around code? Please > take a look. Thanks! > > https://codereview.chromium.org/2820693004/diff/20001/ash/shelf/shelf_view.cc > File ash/shelf/shelf_view.cc (right): > > https://codereview.chromium.org/2820693004/diff/20001/ash/shelf/shelf_view.cc... > ash/shelf/shelf_view.cc:1085: DCHECK(overflow_bubble_ && > overflow_bubble_->IsShowing()); > On 2017/04/18 00:11:04, msw wrote: > > nit: just DCHECK(overflow_bubble_->IsShowing()); that'll disambiguate crashes > > when |overflow_bubble_| is null from DCHECK failures when it's not showing. > > (ditto for the other similar DCHECKS below) > > Done. skuhne@ - Friendly ping.
skuhne@google.com changed reviewers: + skuhne@google.com
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... ash/shelf/shelf_view.cc:1083: main_shelf_->EndDrag(true); could you please add here true /* cancel */ ? https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1086: overflow_bubble_->shelf_view()->EndDrag(true); same here (and in the other places) https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1248: } Line 1241 - 1248 appears to be quite repetitive. (e.g. 1217 - 1224, 1138 - 1149, 1082-1087, ..) You might create a function for it - putting in the cancel value and getting in return if it was in overflow mode - in which case you could do the special casing like the increment from line 1148).
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... ash/shelf/shelf_view.cc:1083: main_shelf_->EndDrag(true); On 2017/05/03 00:56:18, skuhne wrote: > could you please add here true /* cancel */ ? Done. https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1086: overflow_bubble_->shelf_view()->EndDrag(true); On 2017/05/03 00:56:18, skuhne wrote: > same here (and in the other places) Done. https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1248: } On 2017/05/03 00:56:18, skuhne wrote: > Line 1241 - 1248 appears to be quite repetitive. > (e.g. 1217 - 1224, 1138 - 1149, 1082-1087, ..) > > You might create a function for it - putting in the cancel value and getting in > return if it was in overflow mode - in which case you could do the special > casing like the increment from line 1148). I did this but I think its better to keep this as void and put a check right after for the special case in line 1148 because it is a little confusing to me. What do you think?
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... ash/shelf/shelf_view.cc:1248: } Super. Thanks!
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 (right): > > https://codereview.chromium.org/2820693004/diff/40001/ash/shelf/shelf_view.cc... > ash/shelf/shelf_view.cc:1248: } > Super. Thanks! Thanks!
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2820693004/#ps100001 (title: "Fixed patch set 3 errors.")
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
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_presub...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
sammiequon@chromium.org changed reviewers: + derat@chromium.org
On 2017/05/03 18:01:41, commit-bot: I haz the power wrote: > 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_presub...) derat@ - Please take a look. Thanks!
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: This issue passed the CQ dry run.
lgtm for //ash/test (which i assume is all you needed me to look at)
On 2017/05/03 20:41:28, Daniel Erat wrote: > lgtm for //ash/test (which i assume is all you needed me to look at) Yup, thanks!
The CQ bit was checked by sammiequon@chromium.org
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
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_presub...)
The CQ bit was checked by sammiequon@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: This issue passed the CQ dry run.
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, derat@chromium.org, skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/2820693004/#ps120001 (title: "Rebased.")
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": 120001, "attempt_start_ts": 1493917713027570, "parent_rev": "d9c705db92238916a6c88e4dca9acb005b2657da", "commit_rev": "31438b99f6f743976583ce02a4a362c26b9e144e"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/31438b99f6f743976583ce02a4a3... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/31438b99f6f743976583ce02a4a3... |