New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 773200 link

Starred by 1 user
Status: Fixed
Owner:
jinsuk...@chromium.org
Closed: Nov 2017
Cc:
█ aelias@chromium.org
boliu@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 617324
Blocking:
issue 598880



Sign in to add a comment

RenderCoordinates API to WebContents

Project Member Reported by jinsuk...@chromium.org, Oct 10 2017 
RenderCoordinates is owned by & sits behind ContentViewCore, providing various view-related size/metric/scale factor. It is either directly exposed via ContentViewCore.getRenderCoordinates() or accessed through CVC by API such as ContentViewCore.getContentWidthCss() by embedders. Having WebContents own the instance will help embedders avoid accessing ContentViewCore.

RenderCoordinates itself can be considered as implementation details, so direct exposure to embedders should be disallowed. I'm considering all the API's it provides to be bridged by WebContents.

- getContentsOffsetYPix
- getContentWidth/HeightCss
- get/setDeviceScaleFactor
- getPageScaleFactor
- fromLocalCssToPix
- getScrollX/YPixInt, etc. See https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java for all the API's. Otherwise the class needs to be in content_public.

Any feedbacks are welcome.

Comment 1 by jinsuk...@chromium.org, Oct 10 2017

Blocking: 598880
FYI This refactoring idea is mainly based on https://docs.google.com/document/d/13obX4RB5nbX3w7vwLzYFuo9LvHzCobIPCDNv09vbHxI/edit The credit goes to sievers@.

Comment 2 by boliu@chromium.org, Oct 13 2017 

Generally, things from RendererCoordinate should not be exposed through content java api as its own thing. Looking at that CL, the two production cases can be fixed, and the remaining use cases are from tests. Maybe can expose some test-only interface for those things.

Within content side, it kinda makes more sense for its lifetime to be tied to rwhva. But if you want to keep the current one RendererCoordinator instance per WebContents pattern, then keeping it in WebContentsImpl is ok.

Comment 3 by jinsuk...@chromium.org, Oct 16 2017 

> Within content side, it kinda makes more sense for its lifetime to be tied to rwhva. But if you want to keep the current one RendererCoordinator instance per WebContents pattern, then keeping it in WebContentsImpl is ok.

For access in content, WebContentsImpl being package-private makes it tricky to have impl-only methods without exposing them to public interface. Each class may have to go down to native.. may not work as nicely as intended though.

Comment 4 by boliu@chromium.org, Oct 16 2017 

Well, there's a TODO to make WebContentsImpl public as well...

Comment 5 by boliu@chromium.org, Oct 16 2017 

Maybe it's at the point we attempt to fix DEPS first, to ensure there are no regressions. Exclude public, and add content_public to rules of upper layers (chrome, android_webview, chromecast, components (?)).

Then add !rules for individual files that's still needed, like CVC. Then at least  no one should be causing any regressions

Comment 6 by jinsuk...@chromium.org, Oct 16 2017

Blockedon: 617324
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4488f8f3cb8aa7c9aa63f600ae7efd533349593e

commit 4488f8f3cb8aa7c9aa63f600ae7efd533349593e
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Nov 06 02:06:02 2017

WebContents manages RenderCoordinates

Let WebContents manage RenderCoordinates that provides various view-
related size/metric/scale factor. Before this CL, it was owned by
ContentViewCore, either exposed via |getRenderCoordinates()| or
accessed indirectly via CVC. This CL hides it behind WebContents
so that implementation details won't leak to embedders. It becomes
available within content layer only, through WebContentsImpl.

All the references to the class from embedders were replaced with
alternatives, such as RenderCoordinates.getDeviceScaleFactor()
-> WindowAndroid.getDisplay().getDipScale().

Also added a util class Coordinates available only for tests so that
they can get values indirectly.

Bug:  773200 
Change-Id: If7dc6a987650015aa3db6e9ac627d3ce073607f3
Reviewed-on: https://chromium-review.googlesource.com/711614
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Yash Malik <ymalik@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514079}
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/android_webview/java/DEPS
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/android_webview/java/src/org/chromium/android_webview/AwViewAndroidDelegate.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/android_webview/javatests/DEPS
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/android/javatests/src/org/chromium/chrome/browser/FocusedEditableTextFieldZoomTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestRule.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellControllerInputTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/test/android/javatests/src/org/chromium/chrome/test/util/PrerenderTestHelper.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/TabLoadObserver.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/browser/android/content_view_core.cc
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/browser/web_contents/web_contents_android.h
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/ContentView.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/accessibility/KitKatWebContentsAccessibility.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopWebContentsAccessibility.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/accessibility/OWebContentsAccessibility.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/accessibility/WebContentsAccessibility.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplPixelTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/android/junit/src/org/chromium/content/browser/SelectionPopupControllerTest.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/test/android/BUILD.gn
[add] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/Coordinates.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java
[modify] https://crrev.com/4488f8f3cb8aa7c9aa63f600ae7efd533349593e/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java

Comment 8 by jinsuk...@chromium.org, Nov 16 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment