Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861713005:
WebView: add support for MultiWindow when choosing interstitial (Closed)
|
Created:
3 years, 7 months ago by Nate Fischer Modified:
3 years, 7 months ago Reviewers:
Torne CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebView: add support for MultiWindow when choosing interstitial
This CL changes the logic for choosing the loud vs. quiet interstitial.
Instead of comparing against screen dimensions, we instead use window
dimensions, which makes more sense when Android Multi-Window is enabled.
BUG=718195
Review-Url: https://codereview.chromium.org/2861713005
Cr-Commit-Position: refs/heads/master@{#469521}
Committed: https://chromium.googlesource.com/chromium/src/+/d72c255b78bb35804ac82597879cc74a268ca3bf
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use getRootView() instead, fix extendsOffDeviceScreen to use window size #Patch Set 3 : Remove unused DisplayAndroid #Messages
Total messages: 26 (16 generated)
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
ntfschr@chromium.org changed reviewers: + torne@chromium.org
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.
PTAL. I tried using this for extendsOffDeviceScreen(), but didn't get good results. It appears getMetrics doesn't behave sensibly for some cases: * if an application is full-height (it has disabled the notification bar and title bar), then it will appear to exceed window height * if an application is in the right window of a split screen, it will have x > 0 and will appear that its right side exceeds past the right screen boundary (even if it doesn't). * same as above, for applications in the bottom of a split screen If you have suggestions for handling this as well, let me know. Or if you know of a more reliable way to get window dimensions, let me know.
We should probably check with Android if there's a better way to figure out the size of the containing window properly, because the inconsistencies you've found make me doubtful :) https://codereview.chromium.org/2861713005/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2861713005/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:125: private static final double MIN_SCREEN_WIDTH_PERCENTAGE_FOR_INTERSTITIAL = 0.95; The reason I proposed checking for 100% of the width in the first place was to handle UIs that are doing things like displaying a WebView inside a Material card, or similar designs where there's a narrow margin on either side and a WebView might just happen to meet the height requirement by chance. Is 95% close enough for that sort of UI not to be a false positive? I'm not sure what the normal Material horizontal spacing amounts to.
I wonder if looking at either the dimensions of the root view (from getRootView() on ourselves), or the decor view of the window (from Window.getDecorView()) would be more in line with what we want?
On 2017/05/04 at 15:27:02, torne wrote: > I wonder if looking at either the dimensions of the root view (from getRootView() on ourselves), or the decor view of the window (from Window.getDecorView()) would be more in line with what we want? I tried out Window.getDecorView() and getRootView(). These work a lot better, but still have quirks. Sometimes the bottom/right app in a split will include the nav bar as part of the window size (which totally messes things up). Sometimes it works as expected. I've noticed it's a problem if my test application (which disables title bar and notification bar) is in the bottom/right window, but not if system webview shell is in the bottom/right. Perhaps there's something weird about disabling title & notification bar? It seems like b/30226981 describes this bug, but I can actually still repro on N and O, so it doesn't help us much.
Description was changed from ========== WebView: add support for MultiWindow when choosing interstitial This CL changes the logic for choosing the loud vs. quiet interstitial. Instead of comparing against screen dimensions, we instead use window dimensions, which makes more sense when Android Multi-Window is enabled. When Multi-Window is enabled, it appears that WebView width is never quite 100% of window width (I saw that it was always off by a pixel). To work around this, instead of checking if WebView width is 100% of the window width, we check if it's between 95-100% of the window width. This is strict enough that we probably won't get too many false positives, but can still reasonably support Multi-Window. BUG=718195 ========== to ========== WebView: add support for MultiWindow when choosing interstitial This CL changes the logic for choosing the loud vs. quiet interstitial. Instead of comparing against screen dimensions, we instead use window dimensions, which makes more sense when Android Multi-Window is enabled. BUG=718195 ==========
PTAL. I decided to go with getRootView(), since this seems to work about as well as getDecoreView and it doesn't require an activity context. None of the approaches seem suitable for the remaining edge case, where an app is: - fullscreen (it requested to hide the notification bar) - in the right-hand slot of a landscape split-screen In this case, we'll just fall back to the quiet interstitial, which shouldn't be totally horrible.
The CQ bit was checked by ntfschr@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Latest version LGTM as discussed in chat :)
The CQ bit was checked by ntfschr@chromium.org
On 2017/05/04 at 21:52:40, torne wrote: > Latest version LGTM as discussed in chat :) Thanks, Torne!
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 ntfschr@chromium.org
The CQ bit was checked by ntfschr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/2861713005/#ps40001 (title: "Remove unused DisplayAndroid")
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": 40001, "attempt_start_ts": 1493938509228610,
"parent_rev": "428cd3be8169a04c94c7ac1e0cf18f3789c4935f", "commit_rev":
"d72c255b78bb35804ac82597879cc74a268ca3bf"}
Message was sent while issue was closed.
Description was changed from ========== WebView: add support for MultiWindow when choosing interstitial This CL changes the logic for choosing the loud vs. quiet interstitial. Instead of comparing against screen dimensions, we instead use window dimensions, which makes more sense when Android Multi-Window is enabled. BUG=718195 ========== to ========== WebView: add support for MultiWindow when choosing interstitial This CL changes the logic for choosing the loud vs. quiet interstitial. Instead of comparing against screen dimensions, we instead use window dimensions, which makes more sense when Android Multi-Window is enabled. BUG=718195 Review-Url: https://codereview.chromium.org/2861713005 Cr-Commit-Position: refs/heads/master@{#469521} Committed: https://chromium.googlesource.com/chromium/src/+/d72c255b78bb35804ac82597879c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d72c255b78bb35804ac82597879c... |
