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

Issue 481683003: Support for Adaptive Handle Orientation (Closed)

Created:
6 years, 4 months ago by AviD
Modified:
5 years, 2 months ago
CC:
AKVT, chromium-reviews, darin-cc_chromium.org, jam, jdduke+watch_chromium.org, mohsen, raghu
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Support for Adaptive Handle Orientation It would be easier in edge screen cases to move the selection handles if it is not hidden. Adding support for Adaptive Handle Orientation. BUG=402824 Committed: https://crrev.com/348be68968d377e8a3cb55af747784936aa56185 Cr-Commit-Position: refs/heads/master@{#352319}

Patch Set 1 #

Patch Set 2 : cleaning up #

Patch Set 3 : some more cleaning up #

Total comments: 6

Patch Set 4 : Initial patchset #

Total comments: 2

Patch Set 5 : Separated viewport changed notification #

Total comments: 6

Patch Set 6 : using mirror_x and mirror_y in handles #

Total comments: 4

Patch Set 7 : #

Total comments: 8

Patch Set 8 : Review comments #

Patch Set 9 : #

Total comments: 6

Patch Set 10 : Code refactoring #

Patch Set 11 : Cleanup #

Total comments: 1

Patch Set 12 : #

Patch Set 13 : cleanup #

Total comments: 49

Patch Set 14 : Rebase and refactoring #

Patch Set 15 : Fixed nit added TODO for creation of new inverted handle. #

Patch Set 16 : Fixed few inverting issues checked on Chrome Public apk #

Total comments: 6

Patch Set 17 : Review comments #

Total comments: 12

Patch Set 18 : Added drawable whitespace offset value setter #

Patch Set 19 : unittests #

Total comments: 16

Patch Set 20 : Refactor + Unittests #

Patch Set 21 : fixed nits #

Total comments: 14

Patch Set 22 : Fixed nits #

Total comments: 8

Patch Set 23 : Fixed nits. #

Total comments: 22

Patch Set 24 : Fixed as per jdduke's comments #

Patch Set 25 : nits #

Total comments: 14

Patch Set 26 : changed as per jdduke comments #

Total comments: 4

Patch Set 27 : removed enabling flag #

Total comments: 8

Patch Set 28 : fixed nits #

Patch Set 29 : Added corresponding methods in Aura #

Total comments: 8

Patch Set 30 : Changes in aura handles #

Total comments: 4

Patch Set 31 : Update handles for explicit handle visibility calls #

Total comments: 6

Patch Set 32 : incorporated changes as per review comments #

Patch Set 33 : rebased #

Patch Set 34 : rebase fix #

Patch Set 35 : rebase fix #

Total comments: 8

Patch Set 36 : review comments #

Total comments: 2

Patch Set 37 : changed switch name #

Patch Set 38 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+673 lines, -232 lines) Patch
M content/browser/android/composited_touch_handle_drawable.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 2 chunks +7 lines, -4 lines 0 comments Download
M content/browser/android/composited_touch_handle_drawable.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 6 chunks +44 lines, -26 lines 0 comments Download
M content/browser/android/popup_touch_handle_drawable.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 2 chunks +6 lines, -2 lines 0 comments Download
M content/browser/android/popup_touch_handle_drawable.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 3 chunks +22 lines, -13 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 2 chunks +12 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +10 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 8 chunks +53 lines, -54 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +5 lines, -0 lines 0 comments Download
M ui/touch_selection/touch_handle.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 6 chunks +49 lines, -7 lines 0 comments Download
M ui/touch_selection/touch_handle.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 12 chunks +121 lines, -19 lines 0 comments Download
M ui/touch_selection/touch_handle_drawable_aura.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 2 chunks +8 lines, -6 lines 0 comments Download
M ui/touch_selection/touch_handle_drawable_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 5 chunks +22 lines, -30 lines 0 comments Download
M ui/touch_selection/touch_handle_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 18 chunks +230 lines, -61 lines 0 comments Download
M ui/touch_selection/touch_selection_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 5 chunks +12 lines, -0 lines 0 comments Download
M ui/touch_selection/touch_selection_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 9 chunks +66 lines, -8 lines 0 comments Download
M ui/touch_selection/touch_selection_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 114 (15 generated)
AviD
@jdduke: Could you please take a look? This is only a basic patch which adds ...
6 years, 4 months ago (2014-08-20 11:56:57 UTC) #1
jdduke (slow)
I'm not sure this is the best approach. If we made the drawables viewport-aware, this ...
6 years, 4 months ago (2014-08-20 15:15:02 UTC) #2
AviD
jdduke@ Thanks for your review comments. I was thinking of the below approach for having ...
6 years, 4 months ago (2014-08-21 16:15:20 UTC) #3
jdduke (slow)
So, my suggestion here would be to wait until we enable composited position updates (https://codereview.chromium.org/359033002/). ...
6 years, 4 months ago (2014-08-22 18:04:08 UTC) #4
AviD
@jdduke: This is only a general idea of the inverted handles for text selection. I ...
5 years, 9 months ago (2015-03-16 10:57:51 UTC) #5
jdduke (slow)
I may not be able to review this thoroughly until Wednesday. +mfomitchev, +mohsen for FYI. ...
5 years, 9 months ago (2015-03-16 22:01:45 UTC) #6
AviD
@mfomitchev, @mohsen, @jdduke Could you PTAL? https://codereview.chromium.org/481683003/diff/60001/ui/touch_selection/touch_selection_controller.cc File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/60001/ui/touch_selection/touch_selection_controller.cc#newcode72 ui/touch_selection/touch_selection_controller.cc:72: const gfx::SizeF& root_layer_size, ...
5 years, 9 months ago (2015-03-19 13:13:33 UTC) #7
jdduke (slow)
https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch_selection_controller.cc File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch_selection_controller.cc#newcode444 ui/touch_selection/touch_selection_controller.cc:444: start_orientation_ == TouchHandleOrientation::LEFT_INVERTED || Why does the position change ...
5 years, 9 months ago (2015-03-19 18:56:39 UTC) #8
mfomitchev
I think we should discuss this with Android ProdM. Inverting the handles sounds like a ...
5 years, 9 months ago (2015-03-19 20:01:39 UTC) #9
jdduke (slow)
On 2015/03/19 20:01:39, mfomitchev wrote: > I think we should discuss this with Android ProdM. ...
5 years, 9 months ago (2015-03-19 22:10:56 UTC) #10
mfomitchev
After some discussions, the consensus is that we want this for Chrome on both Android ...
5 years, 9 months ago (2015-03-23 21:01:06 UTC) #12
jdduke (slow)
https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch_selection_controller.cc File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch_selection_controller.cc#newcode444 ui/touch_selection/touch_selection_controller.cc:444: start_orientation_ == TouchHandleOrientation::LEFT_INVERTED || On 2015/03/19 18:56:39, jdduke wrote: ...
5 years, 9 months ago (2015-03-23 21:05:31 UTC) #13
AviD
On 2015/03/23 21:01:06, mfomitchev wrote: > After some discussions, the consensus is that we want ...
5 years, 9 months ago (2015-03-24 11:08:22 UTC) #14
jdduke (slow)
On 2015/03/24 11:08:22, AviD wrote: > On 2015/03/23 21:01:06, mfomitchev wrote: > > After some ...
5 years, 9 months ago (2015-03-24 20:19:57 UTC) #15
AviD
Added patch with mirror_x and mirror_y params. https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch_selection_controller.cc File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/481683003/diff/80001/ui/touch_selection/touch_selection_controller.cc#newcode444 ui/touch_selection/touch_selection_controller.cc:444: start_orientation_ == ...
5 years, 9 months ago (2015-03-26 14:53:11 UTC) #16
AviD
On 2015/03/24 20:19:57, jdduke wrote: > On 2015/03/24 11:08:22, AviD wrote: > > On 2015/03/23 ...
5 years, 9 months ago (2015-03-26 15:51:02 UTC) #17
jdduke (slow)
https://codereview.chromium.org/481683003/diff/100001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/100001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode159 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:159: // Flipped Inverted Case Flipped/inverted can mean the same ...
5 years, 8 months ago (2015-04-06 18:58:42 UTC) #18
AviD
https://codereview.chromium.org/481683003/diff/100001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/100001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode159 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:159: // Flipped Inverted Case On 2015/04/06 18:58:42, jdduke wrote: ...
5 years, 8 months ago (2015-04-07 14:14:11 UTC) #19
jdduke (slow)
I haven't forgotten about this! Just working through a few release blocking issues, I'm hoping ...
5 years, 8 months ago (2015-04-09 16:46:30 UTC) #20
jdduke (slow)
https://codereview.chromium.org/481683003/diff/120001/content/browser/android/composited_touch_handle_drawable.cc File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/120001/content/browser/android/composited_touch_handle_drawable.cc#newcode48 content/browser/android/composited_touch_handle_drawable.cc:48: left_inverted_bitmap_ = SkBitmapOperations::Rotate( There's no need to create new ...
5 years, 8 months ago (2015-04-14 00:00:32 UTC) #21
AviD
https://codereview.chromium.org/481683003/diff/120001/content/browser/android/composited_touch_handle_drawable.cc File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/120001/content/browser/android/composited_touch_handle_drawable.cc#newcode48 content/browser/android/composited_touch_handle_drawable.cc:48: left_inverted_bitmap_ = SkBitmapOperations::Rotate( On 2015/04/14 00:00:32, jdduke wrote: > ...
5 years, 8 months ago (2015-04-23 14:45:17 UTC) #22
jdduke (slow)
Getting closer! Eventually we'll want some tests, but let's leave them out until the implementation ...
5 years, 8 months ago (2015-04-23 15:22:39 UTC) #23
AviD
I will add tests once the approach has been finalized. https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touch_handle.h File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/160001/ui/touch_selection/touch_handle.h#newcode74 ...
5 years, 8 months ago (2015-04-24 16:19:07 UTC) #24
jdduke (slow)
https://codereview.chromium.org/481683003/diff/200001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/200001/ui/touch_selection/touch_handle.cc#newcode141 ui/touch_selection/touch_handle.cc:141: focus_bottom_ = bottom; OK, so there are 3 inputs ...
5 years, 8 months ago (2015-04-24 21:42:25 UTC) #25
jdduke (slow)
Any updates here? Do you want to keep going with this or do you need ...
5 years, 7 months ago (2015-04-30 19:41:51 UTC) #26
AviD
On 2015/04/30 19:41:51, jdduke wrote: > Any updates here? Do you want to keep going ...
5 years, 7 months ago (2015-05-04 05:29:02 UTC) #27
AviD
jdduke@ is this approach Ok? I will upload the modified test cases if you're Ok ...
5 years, 7 months ago (2015-05-10 02:34:12 UTC) #28
jdduke (slow)
A few nits, but at a high level this looks pretty good! Let's wait until ...
5 years, 7 months ago (2015-05-11 15:40:16 UTC) #29
mfomitchev
>A few nits, but at a high level this looks pretty good! Let's wait until ...
5 years, 7 months ago (2015-05-11 21:46:54 UTC) #30
mfomitchev
https://codereview.chromium.org/481683003/diff/240001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/481683003/diff/240001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1457 content/browser/renderer_host/render_widget_host_view_android.cc:1457: gfx::SizeF view_size(frame_metadata.scrollable_viewport_size); Nit: Rename this to viewport_size for consistency? ...
5 years, 7 months ago (2015-05-13 22:19:14 UTC) #31
mfomitchev
https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode214 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:214: } I think we should, since we can return ...
5 years, 7 months ago (2015-05-13 22:33:01 UTC) #32
jdduke (slow)
https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode221 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/13 22:33:01, mfomitchev wrote: ...
5 years, 7 months ago (2015-05-14 15:20:10 UTC) #33
mfomitchev
https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode221 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:20:10, jdduke wrote: ...
5 years, 7 months ago (2015-05-14 15:52:39 UTC) #34
jdduke (slow)
https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode221 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:52:39, mfomitchev wrote: ...
5 years, 7 months ago (2015-05-14 15:55:57 UTC) #35
mfomitchev
https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/240001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode221 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:221: return new LayerDrawable(drawableArray) { On 2015/05/14 15:55:57, jdduke wrote: ...
5 years, 7 months ago (2015-05-14 15:59:40 UTC) #36
AviD
I will upload the new rebased patch tomorrow https://codereview.chromium.org/481683003/diff/240001/content/browser/android/composited_touch_handle_drawable.cc File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/240001/content/browser/android/composited_touch_handle_drawable.cc#newcode120 content/browser/android/composited_touch_handle_drawable.cc:120: layer_->SetBitmap(bitmap); ...
5 years, 7 months ago (2015-05-19 16:26:28 UTC) #37
jdduke (slow)
Thanks! Once you have a chance to upload the latest patchset I'll try to play ...
5 years, 7 months ago (2015-05-19 18:59:26 UTC) #38
AviD
@jdduke, @mfomitchev : PTAL Created an inner private class for InvertedDrawable in PopupTouchHandleDrawable, so that ...
5 years, 7 months ago (2015-05-21 09:23:15 UTC) #39
AviD
jdduke@ PTAL?
5 years, 6 months ago (2015-06-02 19:03:09 UTC) #40
jdduke (slow)
https://codereview.chromium.org/481683003/diff/300001/content/browser/android/composited_touch_handle_drawable.cc File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/300001/content/browser/android/composited_touch_handle_drawable.cc#newcode116 content/browser/android/composited_touch_handle_drawable.cc:116: const SkBitmap& bitmap = g_selection_resources.Get().GetBitmap(orientation); Can we move the ...
5 years, 6 months ago (2015-06-03 18:59:00 UTC) #41
AviD
jdduke@ PTAL https://codereview.chromium.org/481683003/diff/300001/content/browser/android/composited_touch_handle_drawable.cc File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/300001/content/browser/android/composited_touch_handle_drawable.cc#newcode116 content/browser/android/composited_touch_handle_drawable.cc:116: const SkBitmap& bitmap = g_selection_resources.Get().GetBitmap(orientation); On 2015/06/03 ...
5 years, 6 months ago (2015-06-08 08:51:37 UTC) #42
jdduke (slow)
Looking pretty good, I think we're in a state where we can add some tests. ...
5 years, 6 months ago (2015-06-08 15:24:46 UTC) #43
mfomitchev
https://codereview.chromium.org/481683003/diff/320001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/320001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode167 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:167: } nit: no need for brackets https://codereview.chromium.org/481683003/diff/320001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc ...
5 years, 6 months ago (2015-06-08 18:06:09 UTC) #44
AviD
Added unittests. PTAL https://codereview.chromium.org/481683003/diff/320001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/320001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode150 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:150: boolean mirrorChanged) { On 2015/06/08 15:24:46, ...
5 years, 6 months ago (2015-06-16 14:09:14 UTC) #45
jdduke (slow)
Haven't forgotten about this, but I won't be able to get to reviewing it until ...
5 years, 6 months ago (2015-06-17 19:37:57 UTC) #46
mfomitchev
https://codereview.chromium.org/481683003/diff/360001/content/browser/android/composited_touch_handle_drawable.cc File content/browser/android/composited_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android/composited_touch_handle_drawable.cc#newcode155 content/browser/android/composited_touch_handle_drawable.cc:155: mirror_horizontal ? layer_width * 0.25f : layer_width * 0.75f; ...
5 years, 6 months ago (2015-06-18 16:16:31 UTC) #47
AviD
https://codereview.chromium.org/481683003/diff/360001/content/browser/android/composited_touch_handle_drawable.h File content/browser/android/composited_touch_handle_drawable.h (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android/composited_touch_handle_drawable.h#newcode31 content/browser/android/composited_touch_handle_drawable.h:31: gfx::SizeF GetDrawablePadding() const override; On 2015/06/18 16:16:31, mfomitchev wrote: ...
5 years, 6 months ago (2015-06-19 07:20:47 UTC) #48
mfomitchev
https://codereview.chromium.org/481683003/diff/360001/content/browser/android/composited_touch_handle_drawable.h File content/browser/android/composited_touch_handle_drawable.h (right): https://codereview.chromium.org/481683003/diff/360001/content/browser/android/composited_touch_handle_drawable.h#newcode31 content/browser/android/composited_touch_handle_drawable.h:31: gfx::SizeF GetDrawablePadding() const override; On 2015/06/19 07:20:47, AviD wrote: ...
5 years, 6 months ago (2015-06-19 13:43:29 UTC) #49
jdduke (slow)
https://codereview.chromium.org/481683003/diff/360001/content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/360001/content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java#newcode23 content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:23: public class HandleViewResources { For now, I don't think ...
5 years, 6 months ago (2015-06-19 14:33:58 UTC) #50
jdduke (slow)
Thanks Avi, I think the last major thing we want here is to add a ...
5 years, 6 months ago (2015-06-19 14:59:52 UTC) #51
AviD
Thanks jdduke. I will add the enable/disable bit in the next patch. https://codereview.chromium.org/481683003/diff/360001/content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java ...
5 years, 6 months ago (2015-06-19 15:25:29 UTC) #52
jdduke (slow)
On 2015/06/19 15:25:29, AviD wrote: > mfomitchev suggested moving this logic of setting origin to ...
5 years, 6 months ago (2015-06-19 15:27:10 UTC) #53
AviD
Moved focal offset and hotspot logic to TouchHandle. Also added a flag based updating of ...
5 years, 4 months ago (2015-07-30 15:09:55 UTC) #54
mfomitchev
Looks great! Just minor comments from me. Thanks for doing this work! https://codereview.chromium.org/481683003/diff/400001/content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java ...
5 years, 4 months ago (2015-08-06 14:33:03 UTC) #55
AviD
Thanks mfomitchev@ PTAL https://codereview.chromium.org/481683003/diff/400001/content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java File content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java (right): https://codereview.chromium.org/481683003/diff/400001/content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java#newcode24 content/public/android/java/src/org/chromium/content/browser/input/HandleViewResources.java:24: private static final float sHandleHorizontalPadding = ...
5 years, 4 months ago (2015-08-09 13:55:42 UTC) #56
mfomitchev
LGTM with nits https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touch_handle.cc#newcode112 ui/touch_selection/touch_handle.cc:112: SetUpdateLayoutRequired(); On 2015/08/09 13:55:41, AviD wrote: ...
5 years, 4 months ago (2015-08-10 13:23:46 UTC) #57
AviD
https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/400001/ui/touch_selection/touch_handle.cc#newcode112 ui/touch_selection/touch_handle.cc:112: SetUpdateLayoutRequired(); On 2015/08/10 13:23:45, mfomitchev wrote: > On 2015/08/09 ...
5 years, 4 months ago (2015-08-10 14:16:10 UTC) #58
jdduke (slow)
Sorry for the delayed review, I'd like to get to it this week but it ...
5 years, 4 months ago (2015-08-10 21:48:30 UTC) #59
jdduke (slow)
Thanks, haven't looked at the tests yet, mostly just a few more nits. https://codereview.chromium.org/481683003/diff/440001/content/browser/android/popup_touch_handle_drawable.cc File ...
5 years, 4 months ago (2015-08-17 17:20:54 UTC) #60
AviD
Thanks jdduke, PTAL. https://codereview.chromium.org/481683003/diff/440001/content/browser/android/popup_touch_handle_drawable.cc File content/browser/android/popup_touch_handle_drawable.cc (right): https://codereview.chromium.org/481683003/diff/440001/content/browser/android/popup_touch_handle_drawable.cc#newcode70 content/browser/android/popup_touch_handle_drawable.cc:70: const float PopupTouchHandleDrawable::GetPaddingFromResources() { On 2015/08/17 ...
5 years, 4 months ago (2015-08-18 13:56:15 UTC) #61
AviD
jdduke@ Are there any other changes that you would like me to do or any ...
5 years, 4 months ago (2015-08-25 12:57:30 UTC) #62
jdduke (slow)
On 2015/08/25 12:57:30, AviD wrote: > jdduke@ Are there any other changes that you would ...
5 years, 4 months ago (2015-08-25 16:12:30 UTC) #63
jdduke (slow)
https://codereview.chromium.org/481683003/diff/480001/content/browser/android/content_startup_flags.cc File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/480001/content/browser/android/content_startup_flags.cc#newcode69 content/browser/android/content_startup_flags.cc:69: parsed_command_line->AppendSwitch( I think we still need to address WebView ...
5 years, 3 months ago (2015-09-08 15:42:55 UTC) #64
AviD
@jdduke I built webview and tried replacing the SystemWebView on my Note4 device, but that ...
5 years, 3 months ago (2015-09-09 15:17:01 UTC) #65
jdduke (slow)
With the flag change I'll stamp this for landing. https://codereview.chromium.org/481683003/diff/500001/content/browser/android/content_startup_flags.cc File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/500001/content/browser/android/content_startup_flags.cc#newcode69 content/browser/android/content_startup_flags.cc:69: ...
5 years, 3 months ago (2015-09-09 22:19:51 UTC) #66
AviD
Thanks @jdduke https://codereview.chromium.org/481683003/diff/500001/content/browser/android/content_startup_flags.cc File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/481683003/diff/500001/content/browser/android/content_startup_flags.cc#newcode69 content/browser/android/content_startup_flags.cc:69: switches::kEnableAdaptiveHandleOrientation); On 2015/09/09 22:19:50, jdduke wrote: > ...
5 years, 3 months ago (2015-09-10 06:06:13 UTC) #68
jochen (gone - plz use gerrit)
lgtm
5 years, 3 months ago (2015-09-14 09:36:01 UTC) #69
jdduke (slow)
Thanks, I think you'll need to update ui/touch_selection/touch_handle_drawable_aura.cc for this to compile on Aura then ...
5 years, 3 months ago (2015-09-14 15:19:40 UTC) #70
AviD
https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touch_handle.cc#newcode128 ui/touch_selection/touch_handle.cc:128: if (focus_top_ == top && focus_bottom_ == bottom) On ...
5 years, 3 months ago (2015-09-14 15:57:40 UTC) #71
AviD
Will update touch_handle_drawable_aura.cc shortly. On 2015/09/14 15:57:40, AviD wrote: > https://codereview.chromium.org/481683003/diff/520001/ui/touch_selection/touch_handle.cc > File ui/touch_selection/touch_handle.cc (right): ...
5 years, 3 months ago (2015-09-14 15:59:56 UTC) #72
AviD
@jdduke: PTAL
5 years, 3 months ago (2015-09-14 17:38:21 UTC) #73
jdduke (slow)
mohsen@: Can you look at the touch_handle_drawable_aura.h/cc changes? I don't think we can just treat ...
5 years, 3 months ago (2015-09-14 18:38:42 UTC) #74
mohsen
https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touch_handle_drawable_aura.cc File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touch_handle_drawable_aura.cc#newcode138 ui/touch_selection/touch_handle_drawable_aura.cc:138: focal_position_ = position; On 2015/09/14 18:38:41, jdduke wrote: > ...
5 years, 3 months ago (2015-09-14 21:52:41 UTC) #76
AviD
https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touch_handle_drawable_aura.cc File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touch_handle_drawable_aura.cc#newcode102 ui/touch_selection/touch_handle_drawable_aura.cc:102: if (orientation_ == orientation) On 2015/09/14 18:38:41, jdduke wrote: ...
5 years, 3 months ago (2015-09-21 15:40:16 UTC) #77
mohsen
https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touch_handle_drawable_aura.cc File ui/touch_selection/touch_handle_drawable_aura.cc (right): https://codereview.chromium.org/481683003/diff/560001/ui/touch_selection/touch_handle_drawable_aura.cc#newcode138 ui/touch_selection/touch_handle_drawable_aura.cc:138: focal_position_ = position; On 2015/09/21 15:40:16, AviD wrote: > ...
5 years, 3 months ago (2015-09-21 20:24:22 UTC) #78
mohsen
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc#newcode113 ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); When does this layout update actually happen? For ...
5 years, 3 months ago (2015-09-22 18:20:22 UTC) #79
AviD
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc#newcode113 ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); On 2015/09/22 18:20:22, mohsen wrote: > When does ...
5 years, 3 months ago (2015-09-23 05:54:56 UTC) #80
mohsen
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc#newcode113 ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); On 2015/09/23 05:54:56, AviD wrote: > On 2015/09/22 ...
5 years, 3 months ago (2015-09-23 06:03:56 UTC) #81
AviD
https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc File ui/touch_selection/touch_handle.cc (right): https://codereview.chromium.org/481683003/diff/580001/ui/touch_selection/touch_handle.cc#newcode113 ui/touch_selection/touch_handle.cc:113: SetUpdateLayoutRequired(); On 2015/09/23 06:03:56, mohsen wrote: > On 2015/09/23 ...
5 years, 3 months ago (2015-09-23 11:02:05 UTC) #82
mohsen
Aura handles LGTM
5 years, 3 months ago (2015-09-23 15:27:19 UTC) #83
jdduke (slow)
lgtm https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touch_handle.h File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touch_handle.h#newcode61 ui/touch_selection/touch_handle.h:61: virtual void SetNeedsAnimate() = 0; I'd feel slightly ...
5 years, 3 months ago (2015-09-23 15:46:33 UTC) #84
mohsen
https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touch_handle_drawable_aura.h File ui/touch_selection/touch_handle_drawable_aura.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touch_handle_drawable_aura.h#newcode52 ui/touch_selection/touch_handle_drawable_aura.h:52: gfx::PointF focal_position_; Forgot to mention that the variable name ...
5 years, 3 months ago (2015-09-23 16:27:43 UTC) #85
AviD
Thanks @jdduke @mohsen https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touch_handle.h File ui/touch_selection/touch_handle.h (right): https://codereview.chromium.org/481683003/diff/600001/ui/touch_selection/touch_handle.h#newcode61 ui/touch_selection/touch_handle.h:61: virtual void SetNeedsAnimate() = 0; On ...
5 years, 3 months ago (2015-09-24 09:55:25 UTC) #86
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/620001
5 years, 3 months ago (2015-09-24 09:56:07 UTC) #89
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/73163) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, ...
5 years, 3 months ago (2015-09-24 09:58:44 UTC) #91
jdduke (slow)
Looks like you need a rebase, let me know if you have issues making that ...
5 years, 3 months ago (2015-09-24 15:05:50 UTC) #92
AviD
On 2015/09/24 15:05:50, jdduke wrote: > Looks like you need a rebase, let me know ...
5 years, 2 months ago (2015-09-25 16:24:11 UTC) #93
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/680001
5 years, 2 months ago (2015-09-25 16:26:04 UTC) #96
jdduke (slow)
Sorry, just a couple more nits after the rebase. https://codereview.chromium.org/481683003/diff/680001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/680001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode207 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:207: ...
5 years, 2 months ago (2015-09-25 16:44:03 UTC) #98
AviD
PTAL https://codereview.chromium.org/481683003/diff/680001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java File content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/481683003/diff/680001/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java#newcode207 content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:207: final boolean hadValidOrientation = mOrientation != TouchHandleOrientation.UNDEFINED; On ...
5 years, 2 months ago (2015-09-25 17:36:51 UTC) #100
jdduke (slow)
lgtm https://codereview.chromium.org/481683003/diff/700001/content/public/common/content_switches.cc File content/public/common/content_switches.cc (right): https://codereview.chromium.org/481683003/diff/700001/content/public/common/content_switches.cc#newcode888 content/public/common/content_switches.cc:888: const char kEnableAdaptiveHandleOrientation[] = Can we rename this ...
5 years, 2 months ago (2015-09-28 15:55:13 UTC) #101
AviD
https://codereview.chromium.org/481683003/diff/700001/content/public/common/content_switches.cc File content/public/common/content_switches.cc (right): https://codereview.chromium.org/481683003/diff/700001/content/public/common/content_switches.cc#newcode888 content/public/common/content_switches.cc:888: const char kEnableAdaptiveHandleOrientation[] = On 2015/09/28 15:55:13, jdduke wrote: ...
5 years, 2 months ago (2015-09-29 07:44:25 UTC) #102
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/720001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/720001
5 years, 2 months ago (2015-09-29 07:45:02 UTC) #105
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/126796)
5 years, 2 months ago (2015-09-29 08:01:48 UTC) #107
jdduke (slow)
Since it's so close to branch point (this Friday), I'm somewhat inclined for us to ...
5 years, 2 months ago (2015-09-29 15:08:45 UTC) #108
AviD
On 2015/09/29 15:08:45, jdduke wrote: > Since it's so close to branch point (this Friday), ...
5 years, 2 months ago (2015-09-29 17:06:30 UTC) #109
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/481683003/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/481683003/740001
5 years, 2 months ago (2015-10-05 10:46:01 UTC) #112
commit-bot: I haz the power
Committed patchset #38 (id:740001)
5 years, 2 months ago (2015-10-05 12:12:31 UTC) #113
commit-bot: I haz the power
5 years, 2 months ago (2015-10-05 12:13:17 UTC) #114
Message was sent while issue was closed.
Patchset 38 (id:??) landed as
https://crrev.com/348be68968d377e8a3cb55af747784936aa56185
Cr-Commit-Position: refs/heads/master@{#352319}

Powered by Google App Engine
This is Rietveld 408576698