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

Issue 1386403003: Resize only the virtual viewport when the OSK triggers a resize. (Closed)

Created:
5 years, 2 months ago by ymalik
Modified:
4 years, 9 months ago
CC:
AKV, Changwan Ryu, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, jdduke (slow), nasko+codewatch_chromium.org, nona+watch_chromium.org, Rick Byers, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. This CL adds a new View (InsetObserverView) to the View hierarchy that will store the value of insets (OSK, status bar). When there is a resize due to OSK show, we keep the view bounds the same and change the visible viewport size. Design doc: http://go/osk-unification BUG=404315 Committed: https://crrev.com/3edbd8d05df275432f79649ac740070d329b26ea Cr-Commit-Position: refs/heads/master@{#381590}

Patch Set 1 #

Total comments: 16

Patch Set 2 : Worked on review comments. #

Total comments: 2

Patch Set 3 : rebase master Inset handling logic #

Total comments: 30

Patch Set 4 : address review comments #

Total comments: 2

Patch Set 5 : moved insets to InsetConsumerView #

Patch Set 6 : added test #

Patch Set 7 : fix test #

Patch Set 8 : fix select bar bug #

Total comments: 3

Patch Set 9 : add inset consumer view one off the root view #

Total comments: 28

Patch Set 10 : Added InsetConsumerView as root view's first child + other review comments #

Patch Set 11 : #

Total comments: 10

Patch Set 12 : addressed more review comments #

Patch Set 13 : s/InsetConsumerView/InsetObserverView #

Unified diffs Side-by-side diffs Delta from patch set Stats (+435 lines, -1 line) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +21 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/InsetObserverView.java View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +103 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +36 lines, -0 lines 0 comments Download
A chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +154 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/android/content_view_core_impl.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 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 1 chunk +11 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +40 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 8 9 2 chunks +24 lines, -0 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 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 1 chunk +3 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 72 (14 generated)
ymalik
Hey guys, can you please take a look and let me know what you think ...
5 years, 2 months ago (2015-10-08 19:30:20 UTC) #2
ymalik
https://codereview.chromium.org/1386403003/diff/1/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/1386403003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc#newcode621 content/browser/renderer_host/render_widget_host_view_android.cc:621: return gfx::Rect(content_view_core_->GetViewSizeWithoutOSK()); @jdduke GetViewBounds has a bunch of callers. ...
5 years, 2 months ago (2015-10-08 19:39:02 UTC) #3
bokan
Some less consequential comments below. Overall, the approach looks fine to me but Jared has ...
5 years, 2 months ago (2015-10-08 22:38:21 UTC) #4
ymalik
https://codereview.chromium.org/1386403003/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1386403003/diff/1/chrome/app/generated_resources.grd#newcode5572 chrome/app/generated_resources.grd:5572: + Enable OSK overscroll support. With this flag on, ...
5 years, 2 months ago (2015-10-09 01:05:25 UTC) #5
ymalik
On 2015/10/08 22:38:21, bokan wrote: > Some less consequential comments below. Overall, the approach looks ...
5 years, 2 months ago (2015-10-09 01:18:17 UTC) #6
bokan
On 2015/10/09 01:18:17, ymalik1 wrote: > On 2015/10/08 22:38:21, bokan wrote: > > Some less ...
5 years, 2 months ago (2015-10-09 13:16:01 UTC) #7
ymalik
On 2015/10/09 13:16:01, bokan wrote: > On 2015/10/09 01:18:17, ymalik1 wrote: > > On 2015/10/08 ...
5 years, 2 months ago (2015-10-09 14:21:41 UTC) #8
ymalik
On 2015/10/09 13:16:01, bokan wrote: > On 2015/10/09 01:18:17, ymalik1 wrote: > > On 2015/10/08 ...
5 years, 2 months ago (2015-10-09 14:21:42 UTC) #9
jdduke (slow)
Hard to comment further without seeing this wired up. +aelias,changwan who have been working more ...
5 years, 2 months ago (2015-10-09 15:17:28 UTC) #11
aelias_OOO_until_Jul13
Let's not introduce a flag for this. Please write an end-to-end change you believe should ...
5 years, 2 months ago (2015-10-09 21:56:30 UTC) #12
bokan
On 2015/10/09 21:56:30, aelias wrote: > Let's not introduce a flag for this. Please write ...
5 years, 2 months ago (2015-10-13 17:54:09 UTC) #13
ymalik
On 2015/10/09 21:56:30, aelias wrote: > Let's not introduce a flag for this. Please write ...
5 years, 2 months ago (2015-10-13 18:06:35 UTC) #15
ymalik
On 2015/10/13 17:54:09, bokan wrote: > On 2015/10/09 21:56:30, aelias wrote: > > Let's not ...
5 years, 2 months ago (2015-10-13 18:12:22 UTC) #16
aelias_OOO_until_Jul13
On 2015/10/13 at 17:54:09, bokan wrote: > I'm not sure we can just go straight ...
5 years, 2 months ago (2015-10-13 18:35:16 UTC) #17
Rick Byers
On 2015/10/13 18:35:16, aelias wrote: > On 2015/10/13 at 17:54:09, bokan wrote: > > I'm ...
5 years, 2 months ago (2015-10-15 00:05:53 UTC) #18
aelias_OOO_until_Jul13
On 2015/10/15 at 00:05:53, rbyers wrote: > New/changed web platform features are often launched first ...
5 years, 2 months ago (2015-10-15 01:16:41 UTC) #19
jdduke (slow)
On 2015/10/15 01:16:41, aelias wrote: > Beyond that, I'm starting to have a bad feeling ...
5 years, 2 months ago (2015-10-15 01:29:11 UTC) #20
Rick Byers
On 2015/10/15 01:16:41, aelias wrote: > On 2015/10/15 at 00:05:53, rbyers wrote: > > New/changed ...
5 years, 2 months ago (2015-10-15 01:57:29 UTC) #21
aelias_OOO_until_Jul13
On 2015/10/15 at 01:57:29, rbyers wrote: > Interesting. I agree we don't want to rely ...
5 years, 2 months ago (2015-10-15 02:58:12 UTC) #22
Rick Byers
On 2015/10/15 02:58:12, aelias wrote: > On 2015/10/15 at 01:57:29, rbyers wrote: > > Interesting. ...
5 years, 2 months ago (2015-10-15 03:46:07 UTC) #23
aelias_OOO_until_Jul13
Sure, feel free to dig in, read the Android source code etc to learn about ...
5 years, 2 months ago (2015-10-15 18:50:45 UTC) #24
bokan
The adjustPan mode is problematic because it only guarantees that it'll scroll the cursor into ...
5 years ago (2015-12-04 21:23:09 UTC) #25
ymalik
On 2015/12/04 21:23:09, bokan wrote: > The adjustPan mode is problematic because it only guarantees ...
4 years, 11 months ago (2016-01-07 17:39:59 UTC) #26
aelias_OOO_until_Jul13
On 2016/01/07 at 17:39:59, ymalik wrote: > I agree that crux here is that we ...
4 years, 11 months ago (2016-01-07 22:27:31 UTC) #27
ymalik
On 2016/01/07 22:27:31, aelias wrote: > On 2016/01/07 at 17:39:59, ymalik wrote: > > I ...
4 years, 11 months ago (2016-01-28 02:09:21 UTC) #28
ymalik
https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/res/layout/main.xml File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/res/layout/main.xml#newcode1 chrome/android/java/res/layout/main.xml:1: <?xml version="1.0" encoding="utf-8"?> FYI, The differ doesn't see this, ...
4 years, 11 months ago (2016-01-28 02:09:36 UTC) #29
aelias_OOO_until_Jul13
Overall approach looks reasonable to me, thanks. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode353 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:353: while (parent.getParent() ...
4 years, 11 months ago (2016-01-28 05:04:56 UTC) #30
aelias_OOO_until_Jul13
On furthet thought, I suggest storing the rect state in the InsetConsumerView and making the ...
4 years, 11 months ago (2016-01-28 05:16:45 UTC) #31
bokan
https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode361 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: mInsetConsumerView.setFitsSystemWindows(true); On 2016/01/28 05:04:55, aelias wrote: > The Android ...
4 years, 10 months ago (2016-01-28 15:53:42 UTC) #32
AKV
Please check in line comments. https://codereview.chromium.org/1386403003/diff/40001/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/1386403003/diff/40001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode634 content/browser/renderer_host/render_widget_host_view_android.cc:634: gfx::Size RenderWidgetHostViewAndroid::GetVisibleViewportSize() const { ...
4 years, 10 months ago (2016-02-10 11:35:28 UTC) #34
ymalik
On 2016/01/28 05:16:45, aelias wrote: > On furthet thought, I suggest storing the rect state ...
4 years, 10 months ago (2016-02-11 00:58:34 UTC) #35
ymalik
@aelias, I addressed your review feedback. My apologies I saw your suggestion regarding moving the ...
4 years, 10 months ago (2016-02-11 01:06:23 UTC) #36
aelias_OOO_until_Jul13
Testing-wise, you might look at AutofillPopupWithKeyboardTest.java which involves waiting for the on-screen-keyboard to appear. Also, ...
4 years, 10 months ago (2016-02-24 02:43:59 UTC) #37
ymalik
@aelias, PTAL :) I moved the insets to InsetConsumerView and added tests. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java ...
4 years, 9 months ago (2016-03-03 02:10:18 UTC) #38
aelias_OOO_until_Jul13
lgtm, thanks. Adding tedchoc@ for OWNERS. (As some context, after some research ymalik@ found a ...
4 years, 9 months ago (2016-03-04 07:31:42 UTC) #40
bokan
Could you expand on the changes made by this CL in the description. The design ...
4 years, 9 months ago (2016-03-07 20:20:06 UTC) #42
ymalik
On 2016/03/04 07:31:42, aelias wrote: > lgtm, thanks. Adding tedchoc@ for OWNERS. > > (As ...
4 years, 9 months ago (2016-03-07 20:27:36 UTC) #43
ymalik
On 2016/03/04 07:31:42, aelias wrote: > lgtm, thanks. Adding tedchoc@ for OWNERS. > > (As ...
4 years, 9 months ago (2016-03-08 17:09:08 UTC) #46
aelias_OOO_until_Jul13
https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode378 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: addAfter.setFitsSystemWindows(false); Hmm, this algorithm is more complicated than I'd ...
4 years, 9 months ago (2016-03-09 01:34:43 UTC) #47
ymalik
https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode378 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: addAfter.setFitsSystemWindows(false); On 2016/03/09 01:34:43, aelias wrote: > Hmm, this ...
4 years, 9 months ago (2016-03-09 16:47:14 UTC) #48
aelias_OOO_until_Jul13
https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode378 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: addAfter.setFitsSystemWindows(false); I don't think the worry the root View ...
4 years, 9 months ago (2016-03-09 22:18:59 UTC) #49
ymalik
On 2016/03/09 22:18:59, aelias wrote: > https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode378 ...
4 years, 9 months ago (2016-03-10 01:54:48 UTC) #50
aelias_OOO_until_Jul13
lgtm, over to tedchoc@ for OWNERS.
4 years, 9 months ago (2016-03-10 02:08:17 UTC) #51
ymalik
On 2016/03/10 02:08:17, aelias wrote: > lgtm, over to tedchoc@ for OWNERS. tedchoc@ Currently, hierarchy ...
4 years, 9 months ago (2016-03-10 15:25:06 UTC) #52
Ted C
This will be really useful. I'm hopeful we can use this logic (or something very ...
4 years, 9 months ago (2016-03-12 00:01:44 UTC) #53
ymalik
@tedchoc PTAL :) I will rename InsetConsumerView to InsetObserverView now that it doesn't actually consume ...
4 years, 9 months ago (2016-03-14 22:19:02 UTC) #54
Ted C
It would be good to test this in multi-window if you have access to the ...
4 years, 9 months ago (2016-03-14 22:53:07 UTC) #55
ymalik
@tedchoc, PTAL :) - Addressed review comments. - Tested on Android M Multiwindow and Samsung ...
4 years, 9 months ago (2016-03-15 16:23:26 UTC) #56
Ted C
lgtm w/ the proposed rename On 2016/03/15 16:23:26, ymalik1 wrote: > @tedchoc, PTAL :) > ...
4 years, 9 months ago (2016-03-15 19:59:51 UTC) #57
ymalik
On 2016/03/07 20:20:06, bokan wrote: > Could you expand on the changes made by this ...
4 years, 9 months ago (2016-03-16 20:46:40 UTC) #59
ymalik
+isherman@ for histograms.xml +avi for content/public/common/content_switches
4 years, 9 months ago (2016-03-16 20:49:49 UTC) #61
ymalik
+isherman@ for histograms.xml +avi for content/public/common/content_switches
4 years, 9 months ago (2016-03-16 20:49:56 UTC) #62
Avi (use Gerrit)
On 2016/03/16 20:49:56, ymalik1 wrote: > +isherman@ for histograms.xml > +avi for content/public/common/content_switches LGTM; we ...
4 years, 9 months ago (2016-03-16 21:01:41 UTC) #63
ymalik
On 2016/03/16 21:01:41, Avi wrote: > On 2016/03/16 20:49:56, ymalik1 wrote: > > +isherman@ for ...
4 years, 9 months ago (2016-03-16 21:03:55 UTC) #64
Ilya Sherman
histograms.xml lgtm
4 years, 9 months ago (2016-03-16 22:23:44 UTC) #65
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1386403003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1386403003/240001
4 years, 9 months ago (2016-03-16 22:33:04 UTC) #68
commit-bot: I haz the power
Committed patchset #13 (id:240001)
4 years, 9 months ago (2016-03-16 23:37:14 UTC) #70
commit-bot: I haz the power
4 years, 9 months ago (2016-03-16 23:38:37 UTC) #72
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/3edbd8d05df275432f79649ac740070d329b26ea
Cr-Commit-Position: refs/heads/master@{#381590}

Powered by Google App Engine
This is Rietveld 408576698