Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103243002:
Factor out ContentViewAndroidDelegate (Closed)
|
Created:
4 years, 5 months ago by Jinsuk Kim Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, Peter Beverloo, jam, darin-cc_chromium.org, android-webview-reviews_chromium.org, jochen+watch_chromium.org, hush (inactive), keishi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out ContentViewAndroidDelegate
Took ContentViewAndroidDelegate out of ContentViewCore
so that embedders come with their own implementation of
ViewAndroidDelegate. This helps content layer
not have to know about Android View/WebView-specific details.
BUG=617750
Committed: https://crrev.com/9ee8865d84c2156728f1b9f78fb8f8b812e4ba19
Cr-Commit-Position: refs/heads/master@{#410003}
Patch Set 1 : Split out ContentViewAndroidDelegate #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Rebased #
Total comments: 34
Patch Set 4 : Addressed Comments/Fixed some tests #Patch Set 5 : ScopedAnchorView #
Total comments: 40
Patch Set 6 : Addressed Comments #
Total comments: 14
Patch Set 7 : Rebased #
Total comments: 2
Patch Set 8 : Addressed comments #
Total comments: 4
Patch Set 9 : Addressed comments/Fixed tests #
Total comments: 4
Patch Set 10 : addressed webview comments #Patch Set 11 : rebased #Patch Set 12 : Removed isValidView() #
Total comments: 19
Patch Set 13 : webview issues addressed #
Total comments: 48
Patch Set 14 : Addressed comments chrome/webview #
Total comments: 23
Patch Set 15 : rebased & comments #
Total comments: 9
Patch Set 16 : comments #Patch Set 17 : removed anchorWidth param #
Total comments: 8
Patch Set 18 : use px values/rebased #
Total comments: 2
Patch Set 19 : s/base/display #Patch Set 20 : rebased #
Total comments: 1
Messages
Total messages: 152 (70 generated)
jinsukkim@chromium.org changed reviewers: + sievers@chromium.org
Basic ViewAndroidDelegate impl (ContentViewAndroidDelegate) is still in content layer since it is shared by chrome, shell, cast shell etc.
Patchset #1 (id:1) has been deleted
Description was changed from ========== Factor out ContentViewAndroidDelegate Took ContentViewAndroidDelegate out of ContentViewCore so that embedders come with their own implementation of ViewAndroidDelegate. This helps content layer not have to know about Android View/WebView-specific details. BUG=617750 ========== to ========== Factor out ContentViewAndroidDelegate Took ContentViewAndroidDelegate out of ContentViewCore so that embedders come with their own implementation of ViewAndroidDelegate. This helps content layer not have to know about Android View/WebView-specific details. BUG=617750 ==========
sievers@chromium.org changed reviewers: + sgurun@chromium.org, tedchoc@chromium.org
+Selim, Ted, see my comment https://codereview.chromium.org/2103243002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:99: * continuous build & test in the open source SDK-based tree). nit: accident https://codereview.chromium.org/2103243002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2103243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:233: ContentViewAndroidDelegate delegate = new ContentViewAndroidDelegate( Can we implement a ViewAndroid anonymously here? This doesn't need to do anything at all since the container view for overlay panels is never added to the View hierarchy. But if we expose the delegate here it will be more straightforward to implement this in a way where it can actually add views to the right parent view. https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:53: * indefinitely prevents Android WebView from being garbage collected. So then this should really also be in the embedder (AwContents). https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:61: protected final Map<View, Position> mAnchorViews = new LinkedHashMap<View, Position>(); This map is only used for WebView, can we also move that up? https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:112: startMargin = containerView.getMeasuredWidth() - Math.round((width + x) * scale); The WebView path doesn't have this logic, @sgurun: So it doesn't support RTL? I think this is probably why: This code is not needed anymore here. Only PowerSaveBlocker calls setAnchorViewPosition - why it even does is unclear to me. The html select dropdown and autofill popup use DropdownPopupWindow.java which does dpi as well as RTL adjustments. In fact it looks like it does it through View layout params which seems better. +Ted to confirm that removing all of this layout and positioning logic in here is fine. In fact then we can remove this class. The embedder just needs an addView()/removeView() hook which it can implement anonymously. The anchor view should also be owned by the caller I think.
https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:112: startMargin = containerView.getMeasuredWidth() - Math.round((width + x) * scale); On 2016/06/28 22:26:10, sievers wrote: > The WebView path doesn't have this logic, > > @sgurun: So it doesn't support RTL? > > I think this is probably why: This code is not needed anymore here. > > Only PowerSaveBlocker calls setAnchorViewPosition - why it even does is unclear > to me. > The html select dropdown and autofill popup use DropdownPopupWindow.java which > does dpi as well as RTL adjustments. > In fact it looks like it does it through View layout params which seems better. > > +Ted to confirm that removing all of this layout and positioning logic in here > is fine. > In fact then we can remove this class. The embedder just needs an > addView()/removeView() hook which it can implement anonymously. > > The anchor view should also be owned by the caller I think. Actually I got this wrong: DropdownPopupWindow also goes through here. But there is still some mess going on wrt who does what and how this can work correctly in both Chrome and WebView. And I think we should make it work by putting what's needed in AutofillPopup/SelectPopupDropdown/DropdownPopupWindow and then just call up to add the view. WebView then overrides what it needs for supporting switching container views (since this is not generally supported and doesn't work with other things see comments in CVC.java).
How about this: DropdownPopupWindow gets the logic to scale to DIP and setLayoutParams(). Then the delegate only has one method 'addView(View view)' and WebView does get/setLayoutParams() and adjusts it for its AbsoluteLayout. DropDownPopupWindow and PowerSaveBlocker also store a reference to the anchor view to be able to remove it. Then ContentViewAndroidDelegate.java is not needed and all of the implementation lives in a reasonable place.
On 2016/06/30 00:49:44, sievers wrote: > How about this: > > DropdownPopupWindow gets the logic to scale to DIP and setLayoutParams(). > Then the delegate only has one method 'addView(View view)' and WebView does > get/setLayoutParams() and adjusts it for its AbsoluteLayout. > > DropDownPopupWindow and PowerSaveBlocker also store a reference to the anchor > view to be able to remove it. > > Then ContentViewAndroidDelegate.java is not needed and all of the implementation > lives in a reasonable place. Sounds promising. One small detail I'd like to get your input on is that this idea causes the embedder (DropdowndPopupWindow) to have to know about container view which has been well hidden inside ViewAndroidDelegate, like the container's width, context, and measured width. Which looks better - pass the entire container view to the embedder's constructor or define a struct like ContainerInfo to pass only what's necessary? Maybe just trying out with passing the whole container view first?
On 2016/06/30 05:24:25, Jinsuk wrote: > On 2016/06/30 00:49:44, sievers wrote: > > How about this: > > > > DropdownPopupWindow gets the logic to scale to DIP and setLayoutParams(). > > Then the delegate only has one method 'addView(View view)' and WebView does > > get/setLayoutParams() and adjusts it for its AbsoluteLayout. > > > > DropDownPopupWindow and PowerSaveBlocker also store a reference to the anchor > > view to be able to remove it. > > > > Then ContentViewAndroidDelegate.java is not needed and all of the > implementation > > lives in a reasonable place. > > Sounds promising. One small detail I'd like to get your input on is that this > idea causes the embedder (DropdowndPopupWindow) to have to know about container > view which has been well hidden inside ViewAndroidDelegate, like the container's > width, context, and measured width. Which looks better - pass the entire > container view to the embedder's constructor or define a struct like > ContainerInfo to pass only what's necessary? Maybe just trying out with passing > the whole container view first? We need RenderCoordinates.contentOffsetYPix() as well.
On 2016/06/30 00:49:44, sievers wrote: > How about this: > > DropdownPopupWindow gets the logic to scale to DIP and setLayoutParams(). > Then the delegate only has one method 'addView(View view)' and WebView does > get/setLayoutParams() and adjusts it for its AbsoluteLayout. > > DropDownPopupWindow and PowerSaveBlocker also store a reference to the anchor > view to be able to remove it. > > Then ContentViewAndroidDelegate.java is not needed and all of the implementation > lives in a reasonable place. Hm, as I looked into further this seems a bit tricky since those creating DropdownPopupWindow - PasswordGenerationPopupBridg and SelectPopupDropdown - need to access CVC to get container view + render coordinates but it is not always possible (PasswordGenerationPopupBridg is in ui/). One way to handle would be to add getContainerDimension() to the interface providing width/contentOffsetYPix, etc., though I understand you'd prefer to simplifying the ViewAndroidDelegate interface.
On 2016/06/30 16:39:32, Jinsuk wrote: > On 2016/06/30 00:49:44, sievers wrote: > > How about this: > > > > DropdownPopupWindow gets the logic to scale to DIP and setLayoutParams(). > > Then the delegate only has one method 'addView(View view)' and WebView does > > get/setLayoutParams() and adjusts it for its AbsoluteLayout. > > > > DropDownPopupWindow and PowerSaveBlocker also store a reference to the anchor > > view to be able to remove it. > > > > Then ContentViewAndroidDelegate.java is not needed and all of the > implementation > > lives in a reasonable place. > > Hm, as I looked into further this seems a bit tricky since those creating > DropdownPopupWindow - PasswordGenerationPopupBridg and SelectPopupDropdown - > need to access CVC to get container view + render coordinates but it is not > always possible (PasswordGenerationPopupBridg is in ui/). (PasswordGenerationPopupBridge is actually in chrome/ actually.) Only WebView needs RenderCoordinates.getScrollX/YPixInt() and can get it from CVC. For the rest - RTL, view dimensions, and content offset - it seems a bit roundabout to go back to the delegate since these popups are created by the renderer based on the notion of layout, size and content offset that was passed down to that layer. I feel like the more natural flow would be to create the anchor view (or at least determine the rect) through the ViewAndroid which then delegates adding the anchor view as a child to its ViewAndroidDelegate. I *think* all the call sites could be refactored so that we natively go through ViewAndroid first to deal with the anchor rect and add the View through its Delegate. This would actually allow us to keep the implementation for adjusting the coordinates where it is now in ContentViewCore. But the routing seems better i.e. content wants to create popup -> calls ViewAndroid to get an anchor view laid out and added -> creates Popup with anchor view rather than content wants to create popup -> calls up into Java passing up a ViewAndroidDelegate which it got from the ViewAndroid -> that calls shared code in ui/ to get the View laid out and added which is implemented in content again (and if the anchor rect changes it originates from native but here has to flow through all of this again instead of the native object just having a reference to the anchor and adjusting it directly). And the Popup implementation code in Java would not need the ViewAndroidDelegate at all.
Let me describe more clearly: The native code wants to create popup -> ViewAndroid::acquireAnchorView(...), take this and create a Popup through Java with the anchor ContentViewCore implements acquireAnchorView() and does the adjustments/calculations and delegates up to add the View. That means that logic can stay where it is inside ContentViewCore but at the same time is not part of the interface to the CVC embedder (i.e. Chrome or WebView). But we split up the implementation in a better way.
>Only WebView needs RenderCoordinates.getScrollX/YPixInt() and can get it from >CVC. I guess I don't quite understand why only webview needs it. Can you clarify it more?
On 2016/06/30 23:29:37, sgurun wrote:
> >Only WebView needs RenderCoordinates.getScrollX/YPixInt() and can get it from
> >CVC.
>
> I guess I don't quite understand why only webview needs it. Can you clarify it
> more?
It's this:
249 } else if (containerView instanceof
android.widget.AbsoluteLayout) {
250 // This fixes the offset due to a difference in
251 // scrolling model of WebView vs. Chrome.
252 // TODO(sgurun) fix this to use
mContainerViewAtCreation.getScroll[X/Y]()
253 // as it naturally accounts for scroll differences
between
254 // these models.
255 leftMargin += mRenderCoordinates.getScrollXPixInt();
256 topMargin += mRenderCoordinates.getScrollYPixInt();
And that's moved here now:
https://codereview.chromium.org/2103243002/diff/40001/android_webview/java/sr...
On 2016/06/30 23:36:36, sievers wrote:
> On 2016/06/30 23:29:37, sgurun wrote:
> > >Only WebView needs RenderCoordinates.getScrollX/YPixInt() and can get it
from
> > >CVC.
> >
> > I guess I don't quite understand why only webview needs it. Can you clarify
it
> > more?
>
> It's this:
>
> 249 } else if (containerView instanceof
> android.widget.AbsoluteLayout) {
> 250 // This fixes the offset due to a difference in
> 251 // scrolling model of WebView vs. Chrome.
> 252 // TODO(sgurun) fix this to use
> mContainerViewAtCreation.getScroll[X/Y]()
> 253 // as it naturally accounts for scroll differences
> between
> 254 // these models.
> 255 leftMargin += mRenderCoordinates.getScrollXPixInt();
> 256 topMargin += mRenderCoordinates.getScrollYPixInt();
>
> And that's moved here now:
>
>
https://codereview.chromium.org/2103243002/diff/40001/android_webview/java/sr...
I think you missed the one used for both(getContentOffsetYPix()):
int topMargin = Math.round(mRenderCoordinates.getContentOffsetYPix() + y *
scale); /// <---
int scaledWidth = Math.round(width * scale);
// ContentViewCore currently only supports these two container view types.
if (containerView instanceof FrameLayout) {
int startMargin;
...
On 2016/07/01 04:00:34, Jinsuk wrote:
> On 2016/06/30 23:36:36, sievers wrote:
> > On 2016/06/30 23:29:37, sgurun wrote:
> > > >Only WebView needs RenderCoordinates.getScrollX/YPixInt() and can get it
> from
> > > >CVC.
> > >
> > > I guess I don't quite understand why only webview needs it. Can you
clarify
> it
> > > more?
> >
> > It's this:
> >
> > 249 } else if (containerView instanceof
> > android.widget.AbsoluteLayout) {
> > 250 // This fixes the offset due to a difference in
> > 251 // scrolling model of WebView vs. Chrome.
> > 252 // TODO(sgurun) fix this to use
> > mContainerViewAtCreation.getScroll[X/Y]()
> > 253 // as it naturally accounts for scroll differences
> > between
> > 254 // these models.
> > 255 leftMargin += mRenderCoordinates.getScrollXPixInt();
> > 256 topMargin += mRenderCoordinates.getScrollYPixInt();
> >
> > And that's moved here now:
> >
> >
>
https://codereview.chromium.org/2103243002/diff/40001/android_webview/java/sr...
>
> I think you missed the one used for both(getContentOffsetYPix()):
>
> int topMargin = Math.round(mRenderCoordinates.getContentOffsetYPix() + y *
> scale); /// <---
> int scaledWidth = Math.round(width * scale);
> // ContentViewCore currently only supports these two container view types.
> if (containerView instanceof FrameLayout) {
> int startMargin;
> ...
That's for the omnibox offset. It actually comes from native, see
RenderWidgetHostViewAndroid::OnFrameMetadataUpdated:
content_view_core_->UpdateFrameInfo(
...
frame_metadata.location_bar_offset,
So if we moved this to native, we might have to expose it in ViewAndroid also.
On 2016/07/01 17:28:23, sievers wrote:
> On 2016/07/01 04:00:34, Jinsuk wrote:
> > On 2016/06/30 23:36:36, sievers wrote:
> > > On 2016/06/30 23:29:37, sgurun wrote:
> > > > >Only WebView needs RenderCoordinates.getScrollX/YPixInt() and can get
it
> > from
> > > > >CVC.
> > > >
> > > > I guess I don't quite understand why only webview needs it. Can you
> clarify
> > it
> > > > more?
> > >
> > > It's this:
> > >
> > > 249 } else if (containerView instanceof
> > > android.widget.AbsoluteLayout) {
> > > 250 // This fixes the offset due to a difference in
> > > 251 // scrolling model of WebView vs. Chrome.
> > > 252 // TODO(sgurun) fix this to use
> > > mContainerViewAtCreation.getScroll[X/Y]()
> > > 253 // as it naturally accounts for scroll differences
> > > between
> > > 254 // these models.
> > > 255 leftMargin +=
mRenderCoordinates.getScrollXPixInt();
> > > 256 topMargin += mRenderCoordinates.getScrollYPixInt();
> > >
> > > And that's moved here now:
> > >
> > >
> >
>
https://codereview.chromium.org/2103243002/diff/40001/android_webview/java/sr...
> >
> > I think you missed the one used for both(getContentOffsetYPix()):
> >
> > int topMargin = Math.round(mRenderCoordinates.getContentOffsetYPix() + y *
> > scale); /// <---
> > int scaledWidth = Math.round(width * scale);
> > // ContentViewCore currently only supports these two container view types.
> > if (containerView instanceof FrameLayout) {
> > int startMargin;
> > ...
>
> That's for the omnibox offset. It actually comes from native, see
> RenderWidgetHostViewAndroid::OnFrameMetadataUpdated:
> content_view_core_->UpdateFrameInfo(
> ...
> frame_metadata.location_bar_offset,
>
Sorry, the one below:
location_bar_content_translation
> So if we moved this to native, we might have to expose it in ViewAndroid also.
PTAL. It's not completed yet but I incorporated the basic idea in your feedback. Stopped here because I saw crrev.com/2122403002 turn CVC/VA relationship from is-a to has-a. Then view_android::AcquireAnchorWindow -> ContentViewCoreImpl::AcquireAnchorWindow -> Java CVC.acquireAnchorWindow flow is not possible any more native classes like AutofillPopupViewAndroid like I did in the patch. I think we may have to go back to add AcquireView() in ViewAndroidDelegate. Let me know what you think. https://codereview.chromium.org/2103243002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:99: * continuous build & test in the open source SDK-based tree). On 2016/06/28 22:26:10, sievers wrote: > nit: accident Well it caught my eye - just wanted to set it right. https://codereview.chromium.org/2103243002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2103243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:233: ContentViewAndroidDelegate delegate = new ContentViewAndroidDelegate( On 2016/06/28 22:26:10, sievers wrote: > Can we implement a ViewAndroid anonymously here? > > This doesn't need to do anything at all since the container view for overlay > panels is never added to the View hierarchy. But if we expose the delegate here > it will be more straightforward to implement this in a way where it can actually > add views to the right parent view. Done. https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:53: * indefinitely prevents Android WebView from being garbage collected. On 2016/06/28 22:26:10, sievers wrote: > So then this should really also be in the embedder (AwContents). Done. https://codereview.chromium.org/2103243002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:61: protected final Map<View, Position> mAnchorViews = new LinkedHashMap<View, Position>(); On 2016/06/28 22:26:10, sievers wrote: > This map is only used for WebView, can we also move that up? Done.
Patchset #2 (id:40001) has been deleted
For the JNI issue I'm wondering if it should just be ViewAndroid::Acquire/RemoveAnchorView(). Then that could internally include ViewAndroidDelegate_Jni.h. I'm also wondering if we really need to expose ViewAndroid::GetViewAndroidDelegate() then, wdyt? We only seem to be passing it up to Java now so that the popups can remove themselves from the anchor view. But if they are owned by some native object or controller, then it *seems* like that we should also be able to do the removal from native. https://codereview.chromium.org/2103243002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (left): https://codereview.chromium.org/2103243002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:56: Java_AutofillPopupBridge_setAnchorRect( So for this one we have to support repositioning it? https://codereview.chromium.org/2103243002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:43: protected final RenderCoordinates mRenderCoordinates; Looks like this is the only content dependency. If the ViewAndroid knew about this, then this interface could be implemented directly in ui/android/ViewAndroid.java. But we can do that as a follow-up.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
On 2016/07/12 21:57:28, sievers wrote: > For the JNI issue I'm wondering if it should just be > ViewAndroid::Acquire/RemoveAnchorView(). Then that could internally include > ViewAndroidDelegate_Jni.h. > Per our offline chat, we also need SetViewPostion to reposition some of the anchored view. > I'm also wondering if we really need to expose > ViewAndroid::GetViewAndroidDelegate() then, wdyt? > > We only seem to be passing it up to Java now so that the popups can remove > themselves from the anchor view. But if they are owned by some native object or > controller, then it *seems* like that we should also be able to do the removal > from native. > Neat. Removed it in the new patch.
PTAL. https://codereview.chromium.org/2103243002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (left): https://codereview.chromium.org/2103243002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:56: Java_AutofillPopupBridge_setAnchorRect( On 2016/07/12 21:57:27, sievers wrote: > So for this one we have to support repositioning it? You're correct. Added back |ViewAndroidDelegate.setViewPosition()|. https://codereview.chromium.org/2103243002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:43: protected final RenderCoordinates mRenderCoordinates; On 2016/07/12 21:57:27, sievers wrote: > Looks like this is the only content dependency. If the ViewAndroid knew about > this, then this interface could be implemented directly in > ui/android/ViewAndroid.java. But we can do that as a follow-up. Moved to native ViewAndroid.
nice! So the main thing seems to be (see comments) that it's a bit tricky to make sure that we remove the anchor views again and don't leak them. What you could also consider is creating a wrapper object that ViewAndroid dispenses that has both the ref to the View but also to the delegate and when it goes out of scope it can remove itself. https://codereview.chromium.org/2103243002/diff/140001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/140001/android_webview/native... android_webview/native/aw_autofill_client.cc:140: anchor_view_.Reset(view_android_->AcquireAnchorView()); Does this need to also be removed again somewhere? https://codereview.chromium.org/2103243002/diff/140001/android_webview/native... android_webview/native/aw_autofill_client.cc:220: view_android_ = view_android; Actually, you can get ViewAndroid dynamically above from web_contents_->GetContentNativeView(). https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:49: controller_ = NULL; need to remove anchor view from hierarchy here also? https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:39: if (controller_) { We have to be careful not to leak the View (by leaving it attached). Maybe it's good enough to also remove it from Hide()? Should we also reset |popup_|? https://codereview.chromium.org/2103243002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:12: import org.chromium.ui.base.ViewAndroidDelegate; Actually there are no more content dependencies now. Should this maybe move into ui/? Or should this actually just merge into ViewAndroid.java? Not sure having the pure interface is that useful since everybody uses this base implementation right? https://codereview.chromium.org/2103243002/diff/140001/content/public/browser... File content/public/browser/android/content_view_core.h (right): https://codereview.chromium.org/2103243002/diff/140001/content/public/browser... content/public/browser/android/content_view_core.h:60: virtual ui::ViewAndroid* GetViewAndroid() = 0; I think we don't need this, see other comment. https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:69: if (view_android_) { Here we also have to figure out how to not leak it. WebContents owns WakeLockServiceContext which owns the PowerSaveBlocker. So I'm not sure why we care about WebContentsDestroyed in WakeLockServiceContext (besides having to think about the exact destruction order between CVC , which is a WebContents::UserData for the ViewAndroid and the WakeLockServiceContext). https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroid.java (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroid.java:20: private final ViewAndroidDelegate mViewDelegate; Do you think it's possible to not have this class and have view_android.cc call into ViewAndroidDelegate instead? Just for the sake of keeping the interface very explicitly one-directional (delegate up to Java). For things that need to eventually call down into native (let's say touch events) I was hoping we could then just also have a different interface (let's say EventHandler or so) that ViewAndroid implements. https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroid.java:22: private ViewAndroid(Context context, ViewAndroidDelegate delegate) { You can actually get the right context from WindowAndroid.getContext (ViewAndroid::GetWindowAndroid()). https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (left): https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:75: return parent_ ? parent_->GetViewAndroidDelegate() : delegate_; see above, I think you want to keep the recursion here internally at least (as a private method maybe). https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:73: if (delegate_.is_null()) Doesn't it need to go to the parent's delegate if |this| has none? For example if somebody wants to add a popup to a RWHV, then the RWHV's ViewAndroid does not have one (but the parent CVC ViewAndroid or someone further up might know how to add this to the container view). https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:88: float content_offset_y_pix = GetWindowAndroid()->content_offset().y(); nit: can you put a TODO? content_offset() should be on ViewAndroid, not WindowAndroid since it's specific to a given web contents/render widget. https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.h:56: void SetAnchorRect(const jobject& anchor, nit: you can use 'const JavaRef<jobject>&>' for the argument. (So it will downcast from ScopedGlobal/LocalRef when passing in.)
https://codereview.chromium.org/2103243002/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2103243002/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:558: if (select_popup_.is_null()) { !select_popup_.is_null()
On 2016/07/14 23:14:32, sievers wrote: > nice! > > So the main thing seems to be (see comments) that it's a bit tricky to make sure > that we remove the anchor views again and don't leak them. > > What you could also consider is creating a wrapper object that ViewAndroid > dispenses that has both the ref to the View but also to the delegate and when it > goes out of scope it can remove itself. Maybe I got your suggestion wrong but anchorView is not owned by ViewAndroid.. Do you mean (anchorView, delegate) to be passed to ViewAndroid by that?
https://codereview.chromium.org/2103243002/diff/140001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/140001/android_webview/native... android_webview/native/aw_autofill_client.cc:140: anchor_view_.Reset(view_android_->AcquireAnchorView()); On 2016/07/14 23:14:31, sievers wrote: > Does this need to also be removed again somewhere? Added |Dismissed| and hooked it up to Java layer. https://codereview.chromium.org/2103243002/diff/140001/android_webview/native... android_webview/native/aw_autofill_client.cc:220: view_android_ = view_android; On 2016/07/14 23:14:31, sievers wrote: > Actually, you can get ViewAndroid dynamically above from > web_contents_->GetContentNativeView(). Ah, that's nice. Done. https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:49: controller_ = NULL; On 2016/07/14 23:14:31, sievers wrote: > need to remove anchor view from hierarchy here also? Done. https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:39: if (controller_) { On 2016/07/14 23:14:31, sievers wrote: > We have to be careful not to leak the View (by leaving it attached). > > Maybe it's good enough to also remove it from Hide()? > > Should we also reset |popup_|? Done. https://codereview.chromium.org/2103243002/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2103243002/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:558: if (select_popup_.is_null()) { On 2016/07/14 23:31:17, sievers wrote: > !select_popup_.is_null() Thanks for catching it. Done. https://codereview.chromium.org/2103243002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:12: import org.chromium.ui.base.ViewAndroidDelegate; On 2016/07/14 23:14:31, sievers wrote: > Actually there are no more content dependencies now. Should this maybe move into > ui/? > Or should this actually just merge into ViewAndroid.java? > > Not sure having the pure interface is that useful since everybody uses this base > implementation right? Sounds better. Made it an inner class of AndroidView. Let me know how it looks. I think moving it to ui/ will be also good though. https://codereview.chromium.org/2103243002/diff/140001/content/public/browser... File content/public/browser/android/content_view_core.h (right): https://codereview.chromium.org/2103243002/diff/140001/content/public/browser... content/public/browser/android/content_view_core.h:60: virtual ui::ViewAndroid* GetViewAndroid() = 0; On 2016/07/14 23:14:32, sievers wrote: > I think we don't need this, see other comment. Done. https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:69: if (view_android_) { On 2016/07/14 23:14:32, sievers wrote: > Here we also have to figure out how to not leak it. > > WebContents owns WakeLockServiceContext which owns the PowerSaveBlocker. > So I'm not sure why we care about WebContentsDestroyed in WakeLockServiceContext > (besides having to think about the exact destruction order between CVC , which > is a WebContents::UserData for the ViewAndroid and the WakeLockServiceContext). Added RemoveAnchorView/Reset() https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroid.java (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroid.java:20: private final ViewAndroidDelegate mViewDelegate; On 2016/07/14 23:14:32, sievers wrote: > Do you think it's possible to not have this class and have view_android.cc call > into ViewAndroidDelegate instead? > > Just for the sake of keeping the interface very explicitly one-directional > (delegate up to Java). > For things that need to eventually call down into native (let's say touch > events) I was hoping we could then just also have a different interface (let's > say EventHandler or so) that ViewAndroid implements. I'm not sure... ViewAndroidDelegate can be overriden, for which we need to invoke the overriden method not having view_android.cc call Java_ViewAndroidDelegate_xxx. https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroid.java:22: private ViewAndroid(Context context, ViewAndroidDelegate delegate) { On 2016/07/14 23:14:32, sievers wrote: > You can actually get the right context from WindowAndroid.getContext > (ViewAndroid::GetWindowAndroid()). Nice! Removed the context passed from ContentViewCore to native. https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (left): https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:75: return parent_ ? parent_->GetViewAndroidDelegate() : delegate_; On 2016/07/14 23:14:32, sievers wrote: > see above, I think you want to keep the recursion here internally at least (as a > private method maybe). Done. https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:73: if (delegate_.is_null()) On 2016/07/14 23:14:32, sievers wrote: > Doesn't it need to go to the parent's delegate if |this| has none? > > For example if somebody wants to add a popup to a RWHV, then the RWHV's > ViewAndroid does not have one (but the parent CVC ViewAndroid or someone further > up might know how to add this to the container view). Makes sense. In fact other methods |SetAnchorRect| |RemoveAnchorView| should have the logic too. https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:88: float content_offset_y_pix = GetWindowAndroid()->content_offset().y(); On 2016/07/14 23:14:32, sievers wrote: > nit: can you put a TODO? content_offset() should be on ViewAndroid, not > WindowAndroid since it's specific to a given web contents/render widget. Meant to do it in this CL but it involved changes in a few more files not relevant to what's being done here. Left a TODO. https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/view_androi... ui/android/view_android.h:56: void SetAnchorRect(const jobject& anchor, On 2016/07/14 23:14:32, sievers wrote: > nit: you can use 'const JavaRef<jobject>&>' for the argument. (So it will > downcast from ScopedGlobal/LocalRef when passing in.) Done.
On 2016/07/15 05:46:14, Jinsuk wrote:
> On 2016/07/14 23:14:32, sievers wrote:
> > nice!
> >
> > So the main thing seems to be (see comments) that it's a bit tricky to make
> sure
> > that we remove the anchor views again and don't leak them.
> >
> > What you could also consider is creating a wrapper object that ViewAndroid
> > dispenses that has both the ref to the View but also to the delegate and
when
> it
> > goes out of scope it can remove itself.
>
> Maybe I got your suggestion wrong but anchorView is not owned by ViewAndroid..
> Do you mean (anchorView, delegate) to be passed to ViewAndroid by that?
Something like
class ScopedAnchorView {
public:
ScopedAnchorView(JavaRef<jobject> jview, JavaRef<jobject> jdelegate);
// calls RemoveView() on the delegate
~ScopedAnchorView();
ScopedAnchorView(ScopedAnchorView&& rvalue);
ScopedAnchorView& operator=(ScopedAnchorView&& rhs);
private:
ScopedJavaGlobalRef<jobject> view_;
ScopedJavaGlobalRef<jobject> delegate_;
}
ScopedAnchorView view = view_android->AcquireAnchorView();
etc.
The main idea being that it's ok to hold a strong ref to the delegate (since
that's sort of the detachable link between the container view and the other
code).
While we don't own the ViewAndroid, which can go away.
This could be a nested class in ViewAndroid so it can access some private
function to trigger a static function for RemoveView() JNI.
https://codereview.chromium.org/2103243002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:12: import org.chromium.ui.base.ViewAndroidDelegate; On 2016/07/15 05:46:26, Jinsuk wrote: > On 2016/07/14 23:14:31, sievers wrote: > > Actually there are no more content dependencies now. Should this maybe move > into > > ui/? > > Or should this actually just merge into ViewAndroid.java? > > > > Not sure having the pure interface is that useful since everybody uses this > base > > implementation right? > > Sounds better. Made it an inner class of AndroidView. Let me know how it looks. > I think moving it to ui/ will be also good though. Sorry there was a typo. I meant to say just put it into ViewAndroid*Delegate*.java But that I guess depends on getting the JNI to work that way, see other comment. https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:69: if (view_android_) { On 2016/07/15 05:46:26, Jinsuk wrote: > On 2016/07/14 23:14:32, sievers wrote: > > Here we also have to figure out how to not leak it. > > > > WebContents owns WakeLockServiceContext which owns the PowerSaveBlocker. > > So I'm not sure why we care about WebContentsDestroyed in > WakeLockServiceContext > > (besides having to think about the exact destruction order between CVC , which > > is a WebContents::UserData for the ViewAndroid and the > WakeLockServiceContext). > > Added RemoveAnchorView/Reset() The problem here is this: if (view_android_) { } So we might still leak it if !view_android_. That's why I was thinking maybe the scoped object that holds a ref to the delegate an can *always* remove the View might be simpler than having to clean up the destruction order and lifetime here for the wake locks. (The use of WeakPtr here was probably just a lazy fix.) https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroid.java (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroid.java:20: private final ViewAndroidDelegate mViewDelegate; On 2016/07/15 05:46:26, Jinsuk wrote: > On 2016/07/14 23:14:32, sievers wrote: > > Do you think it's possible to not have this class and have view_android.cc > call > > into ViewAndroidDelegate instead? > > > > Just for the sake of keeping the interface very explicitly one-directional > > (delegate up to Java). > > For things that need to eventually call down into native (let's say touch > > events) I was hoping we could then just also have a different interface (let's > > say EventHandler or so) that ViewAndroid implements. > > I'm not sure... ViewAndroidDelegate can be overriden, for which we need to > invoke the overriden method not having view_android.cc call > Java_ViewAndroidDelegate_xxx. I think that would call the correct overloaded function through JNI. I *think* you might be able to just generate ViewAndroidDelegate_Jni.h and then include that in the view_android.cc file.
The CQ bit was checked by jinsukkim@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...
Thanks for the code snippet. PTAL. https://codereview.chromium.org/2103243002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java:12: import org.chromium.ui.base.ViewAndroidDelegate; On 2016/07/15 18:35:00, sievers wrote: > On 2016/07/15 05:46:26, Jinsuk wrote: > > On 2016/07/14 23:14:31, sievers wrote: > > > Actually there are no more content dependencies now. Should this maybe move > > into > > > ui/? > > > Or should this actually just merge into ViewAndroid.java? > > > > > > Not sure having the pure interface is that useful since everybody uses this > > base > > > implementation right? > > > > Sounds better. Made it an inner class of AndroidView. Let me know how it > looks. > > I think moving it to ui/ will be also good though. > > Sorry there was a typo. I meant to say just put it into > ViewAndroid*Delegate*.java > But that I guess depends on getting the JNI to work that way, see other comment. Should have seen that :) Done. https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/140001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:69: if (view_android_) { On 2016/07/15 18:35:00, sievers wrote: > On 2016/07/15 05:46:26, Jinsuk wrote: > > On 2016/07/14 23:14:32, sievers wrote: > > > Here we also have to figure out how to not leak it. > > > > > > WebContents owns WakeLockServiceContext which owns the PowerSaveBlocker. > > > So I'm not sure why we care about WebContentsDestroyed in > > WakeLockServiceContext > > > (besides having to think about the exact destruction order between CVC , > which > > > is a WebContents::UserData for the ViewAndroid and the > > WakeLockServiceContext). > > > > Added RemoveAnchorView/Reset() > > The problem here is this: > if (view_android_) { > } > > So we might still leak it if !view_android_. > That's why I was thinking maybe the scoped object that holds a ref to the > delegate an can *always* remove the View might be simpler than having to clean > up the destruction order and lifetime here for the wake locks. (The use of > WeakPtr here was probably just a lazy fix.) Now I got it. Added ScopedAnchorView to remove the view regardless of |ViewAndroid| https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroid.java (right): https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroid.java:20: private final ViewAndroidDelegate mViewDelegate; On 2016/07/15 18:35:00, sievers wrote: > On 2016/07/15 05:46:26, Jinsuk wrote: > > On 2016/07/14 23:14:32, sievers wrote: > > > Do you think it's possible to not have this class and have view_android.cc > > call > > > into ViewAndroidDelegate instead? > > > > > > Just for the sake of keeping the interface very explicitly one-directional > > > (delegate up to Java). > > > For things that need to eventually call down into native (let's say touch > > > events) I was hoping we could then just also have a different interface > (let's > > > say EventHandler or so) that ViewAndroid implements. > > > > I'm not sure... ViewAndroidDelegate can be overriden, for which we need to > > invoke the overriden method not having view_android.cc call > > Java_ViewAndroidDelegate_xxx. > > I think that would call the correct overloaded function through JNI. > I *think* you might be able to just generate ViewAndroidDelegate_Jni.h and then > include that in the view_android.cc file. Done. I tried to annotate the overriden methods of AwViewAndroidDelegate with @CalledByNative but couldn't since jni generator complained.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2016/07/18 06:45:46, Jinsuk wrote: > Done. I tried to annotate the overriden methods of AwViewAndroidDelegate with > @CalledByNative but couldn't since jni generator complained. That seems ok not to annotate them since the application that overrides this shouldn't need to care whether this gets called from native or not.
This is great. Just a bunch of final nits, polish and paranoia. https://codereview.chromium.org/2103243002/diff/180001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/180001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1182: mViewAndroidDelegate = (AwViewAndroidDelegate) mContentViewCore.getViewAndroidDelegate(); Since we create the delegate in createAndInitializeContentViewCore() can we store the reference then? (So CVC.getViewAndroidDelegate() can go away.) https://codereview.chromium.org/2103243002/diff/180001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/180001/android_webview/native... android_webview/native/aw_autofill_client.cc:52: view_android_ = web_contents_->GetContentNativeView(); Sorry, I changed my mind after looking at the autofill popup call sites and think you should call web_contents_->GetNativeView() here. (GetNativeView() returns the container i.e. CVC ViewAndroid, GetContentNativeView() returns the RWHVA's ViewAndroid which can change during navigation and what not.) And that one should never be null, which is good. nit: Also, no need to store it, but you can just call it below. https://codereview.chromium.org/2103243002/diff/180001/android_webview/native... android_webview/native/aw_autofill_client.cc:142: } I'd handle |anchor_view_.view_.is_null()| here and early out. https://codereview.chromium.org/2103243002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java (right): https://codereview.chromium.org/2103243002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java:57: activity.getCurrentContentViewCore().getViewAndroidDelegate(); This is a weird test. It's really a unit test for AutofillPopup, since it doesn't use any of the actual implementation's logic (doesn't even use AutofillClient). It spins up a whole browser to then extract the delegate from the container view and then unit test the feature. I think it should either be a proper integration test based on some html that we load and then somehow tests if we can properly populate the form. Or it can stay as is and then doesn't need to spin up a whole browser but just instantiate an AutofillPopup (and giving it a dummy anchor view). We can do this separately, but maybe CVC.getViewAndroidDelegate() can then become @VisibleForTesting with a TODO to remove it. https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:39: popup_view_.reset(view_android->AcquireAnchorView()); I'd handle |anchor_view_.view_.is_null()| here and early out. (Need to handle |java_object_.is_null() then in the code below also.) https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:51: popup_view_.reset(); See comment in password_generation_popup_view_android.cc. (I'm now wondering if we maybe better avoid causing removeView() *before* onDismiss(). Previously the order was also the opposite.) https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:65: popup_.reset(view_android->AcquireAnchorView()); I'd handle |popup_.view_.is_null()| here and early-out. https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:76: popup_.reset(); I wonder if triggering removeView() here might have some unwanted side effects right before calling hide/dismiss below. For example, could it trigger DropdownPopupWindow.mLayoutChangeListener.onLayoutChange and a DropdownPopupWindow.this.show()? Probably ok to just do nothing here, since |this| will likely go away soon (I'm guessing Java_PasswordGenerationPopupBridge_hide causes Dismissed() and 'delete this'.) https://codereview.chromium.org/2103243002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:560: public ViewAndroidDelegate getViewAndroidDelegate() { Is this still used? Looks like it's only used in AutofillTest.java, I left a comment in there. Also, the 'NOTE' from above is maybe a bit misplaced now since it's up to WebView to implement/override this interface correctly (or not). Admittedly, there is still code in here that access mContainerView directly, but that's for crbug.com/622864. But I don't understand the comment under NOTE here fully anyways. https://codereview.chromium.org/2103243002/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java (right): https://codereview.chromium.org/2103243002/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java:44: public void testAddAndRemoveAnchorViews() { I feel like the tests in this file should also be unit tests (probably even the webview ones you moved). I think this would work through junit, for example see ui/android/junit/. But maybe just file a bug. Someone else can probably pick this up too later. https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... File device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java (right): https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java:35: assert mKeepScreenOnView != null; I think this assert is not valid (anymore?), see .cc file. (We might not have called applyBlock() since that has an if-check.) https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:60: JNIEnv* env = AttachCurrentThread(); I think you should handle |anchor_view_.view_.is_null()| here and not call applyBlock(). (The Java function will access it.) https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:68: Java_PowerSaveBlocker_removeBlock(AttachCurrentThread(), obj.obj()); It's possible that you fixed a corner case bug here where the screen stayed on by being able to remove the if-check. https://codereview.chromium.org/2103243002/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:16: * Class that acquire, position, and remove anchor views from the implementing View. nit: 'that acquires' or 'to acquire' etc. https://codereview.chromium.org/2103243002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:20: private final ViewGroup mContainerView; Can this be in the implementation (and getContainerView abstract) so that we don't accidentally hold a strong ref to the view in the case of WebView? You can probably just override getContainerView() anonymously in the implementation and pass in a 'final' value for the container view from wherever this gets created. https://codereview.chromium.org/2103243002/diff/180001/ui/android/ui_android_... File ui/android/ui_android_jni_registrar.cc (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/ui_android_... ui/android/ui_android_jni_registrar.cc:18: {"ViewAndroid", ViewAndroid::RegisterWindowAndroid}, You can actually leave this out and put a comment in view_android.h with a TODO(crbug.com/603936) that the method is intentionally not called anywhere, since there are no native methods that are called from Java. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:95: float scale = device_info.GetDIPScale(); Can you call GetScaleFactorForNativeView(this) instead (from ui/base/layout.h)? There is work ongoing to make this support more than the default display, and if you call that, it will do the right thing then automatically. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:113: const JavaRef<jobject>& ViewAndroid::GetViewAndroidDelegate() const { nit: This is unused now. But I wonder if it makes the above two functions a bit simpler if you actually use it, like: ViewAndroid::ScopedAnchorView* ViewAndroid::AcquireAnchorView() { ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate()); JNIEnv* env = base::android::AttachCurrentThread(); return ScopedAnchorView(delegate.is_null() ? nullptr : Java_ViewAndroidDelegate_acquireView(env, delegate.obj()), delegate); } https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.h:69: jobject obj() const { return view_.obj(); } nit: Could this just have one method to return 'const JavaRef<jobject>&' instead of ref() and obj()? JavaRef<> has obj() as a method anyways. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.h:76: ScopedAnchorView* AcquireAnchorView(); Can you make this a 'move only' type and return by value instead of as a heap object? You only need to define a move constructor that takes an rvalue reference (maybe '=' operator also for completeness). That will automatically delete the default copy constructor also, making it 'move only'. ScopedAnchorView(ScopedAnchorView&& other) { JNIEnv* env = base::android::AttachCurrentThread(); view_.Reset(env, other.view_.Release()); delegate_.Reset(env, other.delegate_.Release()); }
The CQ bit was checked by jinsukkim@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Patchset #7 (id:220001) has been deleted
https://codereview.chromium.org/2103243002/diff/180001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/180001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1182: mViewAndroidDelegate = (AwViewAndroidDelegate) mContentViewCore.getViewAndroidDelegate(); On 2016/07/18 22:14:16, sievers wrote: > Since we create the delegate in createAndInitializeContentViewCore() can we > store the reference then? (So CVC.getViewAndroidDelegate() can go away.) Done. https://codereview.chromium.org/2103243002/diff/180001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/180001/android_webview/native... android_webview/native/aw_autofill_client.cc:52: view_android_ = web_contents_->GetContentNativeView(); On 2016/07/18 22:14:16, sievers wrote: > Sorry, I changed my mind after looking at the autofill popup call sites and > think you should call web_contents_->GetNativeView() here. (GetNativeView() > returns the container i.e. CVC ViewAndroid, GetContentNativeView() returns the > RWHVA's ViewAndroid which can change during navigation and what not.) And that > one should never be null, which is good. > > nit: Also, no need to store it, but you can just call it below. Done. https://codereview.chromium.org/2103243002/diff/180001/android_webview/native... android_webview/native/aw_autofill_client.cc:142: } On 2016/07/18 22:14:16, sievers wrote: > I'd handle |anchor_view_.view_.is_null()| here and early out. Done. https://codereview.chromium.org/2103243002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java (right): https://codereview.chromium.org/2103243002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java:57: activity.getCurrentContentViewCore().getViewAndroidDelegate(); On 2016/07/18 22:14:16, sievers wrote: > This is a weird test. It's really a unit test for AutofillPopup, since it > doesn't use any of the actual implementation's logic (doesn't even use > AutofillClient). > > It spins up a whole browser to then extract the delegate from the container view > and then unit test the feature. > > I think it should either be a proper integration test based on some html that we > load and then somehow tests if we can properly populate the form. > > Or it can stay as is and then doesn't need to spin up a whole browser but just > instantiate an AutofillPopup (and giving it a dummy anchor view). > > We can do this separately, but maybe CVC.getViewAndroidDelegate() can then > become @VisibleForTesting with a TODO to remove it. Marked the method with @VisibleForTesting and left a TODO. https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:39: popup_view_.reset(view_android->AcquireAnchorView()); On 2016/07/18 22:14:16, sievers wrote: > I'd handle |anchor_view_.view_.is_null()| here and early out. > > (Need to handle |java_object_.is_null() then in the code below also.) Done. https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:51: popup_view_.reset(); On 2016/07/18 22:14:16, sievers wrote: > See comment in password_generation_popup_view_android.cc. > (I'm now wondering if we maybe better avoid causing removeView() *before* > onDismiss(). Previously the order was also the opposite.) Done. Removed it as we do not use unique_ptr any more. In other places where I want to delete the view immediately, I set it to a dummy ScopedAnchorView so that the move assignment can take care of it. https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:65: popup_.reset(view_android->AcquireAnchorView()); On 2016/07/18 22:14:16, sievers wrote: > I'd handle |popup_.view_.is_null()| here and early-out. Done. https://codereview.chromium.org/2103243002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:76: popup_.reset(); On 2016/07/18 22:14:16, sievers wrote: > I wonder if triggering removeView() here might have some unwanted side effects > right before calling hide/dismiss below. For example, could it trigger > DropdownPopupWindow.mLayoutChangeListener.onLayoutChange and a > DropdownPopupWindow.this.show()? > > Probably ok to just do nothing here, since |this| will likely go away soon (I'm > guessing Java_PasswordGenerationPopupBridge_hide causes Dismissed() and 'delete > this'.) Removed. The anchor view will be deleted in Dismissed when 'delete this' is called. https://codereview.chromium.org/2103243002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:560: public ViewAndroidDelegate getViewAndroidDelegate() { On 2016/07/18 22:14:16, sievers wrote: > Is this still used? Looks like it's only used in AutofillTest.java, I left a > comment in there. > > Also, the 'NOTE' from above is maybe a bit misplaced now since it's up to > WebView to implement/override this interface correctly (or not). > > Admittedly, there is still code in here that access mContainerView directly, but > that's for crbug.com/622864. But I don't understand the comment under NOTE here > fully anyways. > Acknowledged. Will clean it up once all the call sites are removed in a follow-up CL. https://codereview.chromium.org/2103243002/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java (right): https://codereview.chromium.org/2103243002/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java:44: public void testAddAndRemoveAnchorViews() { On 2016/07/18 22:14:16, sievers wrote: > I feel like the tests in this file should also be unit tests (probably even the > webview ones you moved). > > I think this would work through junit, for example see ui/android/junit/. > But maybe just file a bug. Someone else can probably pick this up too later. Filed a bug crbug.com/629417 https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... File device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java (right): https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java:35: assert mKeepScreenOnView != null; On 2016/07/18 22:14:16, sievers wrote: > I think this assert is not valid (anymore?), see .cc file. (We might not have > called applyBlock() since that has an if-check.) Done. https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:60: JNIEnv* env = AttachCurrentThread(); On 2016/07/18 22:14:16, sievers wrote: > I think you should handle |anchor_view_.view_.is_null()| here and not call > applyBlock(). (The Java function will access it.) Done. https://codereview.chromium.org/2103243002/diff/180001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:68: Java_PowerSaveBlocker_removeBlock(AttachCurrentThread(), obj.obj()); On 2016/07/18 22:14:16, sievers wrote: > It's possible that you fixed a corner case bug here where the screen stayed on > by being able to remove the if-check. :) https://codereview.chromium.org/2103243002/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:16: * Class that acquire, position, and remove anchor views from the implementing View. On 2016/07/18 22:14:16, sievers wrote: > nit: 'that acquires' or 'to acquire' etc. Done. https://codereview.chromium.org/2103243002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:20: private final ViewGroup mContainerView; On 2016/07/18 22:14:16, sievers wrote: > Can this be in the implementation (and getContainerView abstract) so that we > don't accidentally hold a strong ref to the view in the case of WebView? > > You can probably just override getContainerView() anonymously in the > implementation and pass in a 'final' value for the container view from wherever > this gets created. Done. You can see that the pattern (anonymous ViewAndroidDelegate()) repeated in many places as a result. How about a static helper method |ViewAndroidDelegate.createDefault()| that has |mContainerView|? https://codereview.chromium.org/2103243002/diff/180001/ui/android/ui_android_... File ui/android/ui_android_jni_registrar.cc (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/ui_android_... ui/android/ui_android_jni_registrar.cc:18: {"ViewAndroid", ViewAndroid::RegisterWindowAndroid}, On 2016/07/18 22:14:16, sievers wrote: > You can actually leave this out and put a comment in view_android.h with a > TODO(crbug.com/603936) that the method is intentionally not called anywhere, > since there are no native methods that are called from Java. Done. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:95: float scale = device_info.GetDIPScale(); On 2016/07/18 22:14:16, sievers wrote: > Can you call GetScaleFactorForNativeView(this) instead (from ui/base/layout.h)? > > There is work ongoing to make this support more than the default display, and if > you call that, it will do the right thing then automatically. Done. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:113: const JavaRef<jobject>& ViewAndroid::GetViewAndroidDelegate() const { On 2016/07/18 22:14:16, sievers wrote: > nit: This is unused now. But I wonder if it makes the above two functions a bit > simpler if you actually use it, like: > > ViewAndroid::ScopedAnchorView* ViewAndroid::AcquireAnchorView() { > ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate()); > > JNIEnv* env = base::android::AttachCurrentThread(); > return ScopedAnchorView(delegate.is_null() ? nullptr : > Java_ViewAndroidDelegate_acquireView(env, delegate.obj()), delegate); > } That's neat. Done. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.h:69: jobject obj() const { return view_.obj(); } On 2016/07/18 22:14:17, sievers wrote: > nit: Could this just have one method to return 'const JavaRef<jobject>&' instead > of ref() and obj()? > > JavaRef<> has obj() as a method anyways. Done. Also added |is_null()| to not to have to call |view().is_null()|. https://codereview.chromium.org/2103243002/diff/180001/ui/android/view_androi... ui/android/view_android.h:76: ScopedAnchorView* AcquireAnchorView(); On 2016/07/18 22:14:17, sievers wrote: > Can you make this a 'move only' type and return by value instead of as a heap > object? > > You only need to define a move constructor that takes an rvalue reference (maybe > '=' operator also for completeness). > That will automatically delete the default copy constructor also, making it > 'move only'. > > ScopedAnchorView(ScopedAnchorView&& other) { > JNIEnv* env = base::android::AttachCurrentThread(); > view_.Reset(env, other.view_.Release()); > delegate_.Reset(env, other.delegate_.Release()); > } Done. The way I changed it uses move assignment though. Please see if it looks like what you had in mind.
https://codereview.chromium.org/2103243002/diff/200001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/200001/android_webview/native... android_webview/native/aw_autofill_client.cc:216: anchor_view_ = ui::ViewAndroid::ScopedAnchorView(); nit: might be nice to add a Reset()/Release() method to ScopedAnchorView() to call Reset() on its two members. https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:47: if (java_object_.is_null()) nit: I'd not handle this since it's just a factory and we don't handle 'new' failures. (The above can fail because the ViewAndroid might be detached and/or not have a delegate theoretically.) However, you have to handle it below in some functions if we earlied out in line 41 and because of that |java_object_| is null now. https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:77: JNIEnv* env = base::android::AttachCurrentThread(); This needs to avoid the Java call now if |!java_object_| because of the early-out you added above. https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:99: same here https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... ui/android/view_android.cc:24: : view_(jview), delegate_(jdelegate) { } nit: DCHECK(!jdelegate.is_null() || jview.is_null()); (If there's a view, then need a delegate to remove it). https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... ui/android/view_android.cc:37: JNIEnv* env = base::android::AttachCurrentThread(); only assign if (this != &other) https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... ui/android/view_android.cc:45: if (!delegate_.is_null()) better to check if (!view_.is_null()) (and see DCHECK above) https://codereview.chromium.org/2103243002/diff/240001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/240001/ui/android/view_androi... ui/android/view_android.cc:166: Java_ViewAndroidDelegate_startDragAndDrop(env, Thanks, for cleaning that up!
https://codereview.chromium.org/2103243002/diff/200001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/200001/android_webview/native... android_webview/native/aw_autofill_client.cc:216: anchor_view_ = ui::ViewAndroid::ScopedAnchorView(); On 2016/07/19 22:29:30, sievers wrote: > nit: might be nice to add a Reset()/Release() method to ScopedAnchorView() to > call Reset() on its two members. Done. https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:47: if (java_object_.is_null()) On 2016/07/19 22:29:30, sievers wrote: > nit: I'd not handle this since it's just a factory and we don't handle 'new' > failures. (The above can fail because the ViewAndroid might be detached and/or > not have a delegate theoretically.) > > However, you have to handle it below in some functions if we earlied out in line > 41 and because of that |java_object_| is null now. Done. Please see if the PopupDismissedInternal() looks okay - I think the flow from Hide() -> Dismiss() should be done even when java_object_ is not set. https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:77: JNIEnv* env = base::android::AttachCurrentThread(); On 2016/07/19 22:29:30, sievers wrote: > This needs to avoid the Java call now if |!java_object_| because of the > early-out you added above. Done. https://codereview.chromium.org/2103243002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:99: On 2016/07/19 22:29:30, sievers wrote: > same here Done. https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... ui/android/view_android.cc:24: : view_(jview), delegate_(jdelegate) { } On 2016/07/19 22:29:30, sievers wrote: > nit: DCHECK(!jdelegate.is_null() || jview.is_null()); > > (If there's a view, then need a delegate to remove it). Done. https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... ui/android/view_android.cc:37: JNIEnv* env = base::android::AttachCurrentThread(); On 2016/07/19 22:29:30, sievers wrote: > only assign if (this != &other) Done. https://codereview.chromium.org/2103243002/diff/200001/ui/android/view_androi... ui/android/view_android.cc:45: if (!delegate_.is_null()) On 2016/07/19 22:29:30, sievers wrote: > better to check if (!view_.is_null()) (and see DCHECK above) Done. https://codereview.chromium.org/2103243002/diff/240001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/240001/ui/android/view_androi... ui/android/view_android.cc:166: Java_ViewAndroidDelegate_startDragAndDrop(env, On 2016/07/19 22:29:30, sievers wrote: > Thanks, for cleaning that up! :)
LGTM https://codereview.chromium.org/2103243002/diff/280001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/280001/android_webview/native... android_webview/native/aw_autofill_client.cc:140: if (anchor_view_.is_null()) you meant 'if (!view_android)' https://codereview.chromium.org/2103243002/diff/280001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/280001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:57: PopupDismissedInternal(); Yes, good catch to not leak |this| here. But now that you point that out, I wonder if there is a bug in the preexisting code here - and similarly for PasswordGenerationPopup. There are if-checks in the Java dismiss/hide functions called here and I'm worried that there are corner cases where |this| still leaks here on the native side. wdyt? maybe worth filing a separate bug. It might be more robust to always delete |this| here directly (and making sure that at that point either the Java objects are gone or at least cannot call back into |this| and cause use-after-free anymore). nit: Since we reset |controller| I'd consider just calling 'delete this' here directly. It makes it a bit more obvious since that has to be the last thing we do in this function. And if you call another function that deletes |this| it's a bit risky as an unnoticed side effect. So I'd put at least a comment. Maybe just mention in general that either way Hide() has to delete |this| - whether it's through the Java dismiss path or directly here.
Thanks! https://codereview.chromium.org/2103243002/diff/280001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/280001/android_webview/native... android_webview/native/aw_autofill_client.cc:140: if (anchor_view_.is_null()) On 2016/07/20 18:46:04, sievers wrote: > you meant 'if (!view_android)' This was to preserve what Java layer was previously doing 'if (anchorView != null) { create anchor view; }'. But I can see it a bit confusing. Added the checking |!view_android| as well. https://codereview.chromium.org/2103243002/diff/280001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/280001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:57: PopupDismissedInternal(); On 2016/07/20 18:46:04, sievers wrote: > Yes, good catch to not leak |this| here. > > But now that you point that out, I wonder if there is a bug in the preexisting > code here - and similarly for PasswordGenerationPopup. There are if-checks in > the Java dismiss/hide functions called here and I'm worried that there are > corner cases where |this| still leaks here on the native side. wdyt? maybe worth > filing a separate bug. > It might be more robust to always delete |this| here directly (and making sure > that at that point either the Java objects are gone or at least cannot call back > into |this| and cause use-after-free anymore). > > nit: Since we reset |controller| I'd consider just calling 'delete this' here > directly. It makes it a bit more obvious since that has to be the last thing we > do in this function. And if you call another function that deletes |this| it's a > bit risky as an unnoticed side effect. So I'd put at least a comment. Maybe just > mention in general that either way Hide() has to delete |this| - whether it's > through the Java dismiss path or directly here. Acknowledged. Called |delete this| directly and added a comment.
Patchset #8 (id:260001) has been deleted
jinsukkim@chromium.org changed reviewers: + halliwell@chromium.org, hashimoto@chromium.org
halliwell@chromium.org: Please review changes in chromecast/ hashimoto@chromium.org: Please review changes in device/power_save_blocker
On 2016/07/20 23:28:37, Jinsuk wrote: > mailto:halliwell@chromium.org: Please review changes in chromecast/ > > mailto:hashimoto@chromium.org: Please review changes in device/power_save_blocker chromecast/ lgtm
The CQ bit was checked by jinsukkim@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:827: * Implementation of the interface {@link org.chromium.ui.base.ViewAndroid.ViewAndroidDelegate} nit: Implementation of the abstract class https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:923: if (!mAnchorViews.containsKey(anchorView)) return; the old logic used to check view's parent and if null early out. is this not necessary anymore?
https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:827: * Implementation of the interface {@link org.chromium.ui.base.ViewAndroid.ViewAndroidDelegate} On 2016/07/21 23:24:18, sgurun wrote: > nit: Implementation of the abstract class Done. https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:923: if (!mAnchorViews.containsKey(anchorView)) return; On 2016/07/21 23:24:18, sgurun wrote: > the old logic used to check view's parent and if null early out. is this not > necessary anymore? Thanks for catching. The code is in the super class which the flow here can miss. Added |isValidView()| to do the checking.
The CQ bit was checked by jinsukkim@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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.
On 2016/07/22 02:54:33, Jinsuk wrote: > https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwContents.java:827: * > Implementation of the interface {@link > org.chromium.ui.base.ViewAndroid.ViewAndroidDelegate} > On 2016/07/21 23:24:18, sgurun wrote: > > nit: Implementation of the abstract class > > Done. > > https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwContents.java:923: if > (!mAnchorViews.containsKey(anchorView)) return; > On 2016/07/21 23:24:18, sgurun wrote: > > the old logic used to check view's parent and if null early out. is this not > > necessary anymore? > > Thanks for catching. The code is in the super class which the flow here can > miss. Added |isValidView()| to do the checking. Why is the view not valid though? Don't we only have to be careful about the container view going away? Which again is WebView-specific. But I'm wondering... why don't we just check if getContainerView() returns null in setViewPosition()?
On 2016/07/22 17:09:32, sievers wrote: > On 2016/07/22 02:54:33, Jinsuk wrote: > > > https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... > > File android_webview/java/src/org/chromium/android_webview/AwContents.java > > (right): > > > > > https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... > > android_webview/java/src/org/chromium/android_webview/AwContents.java:827: * > > Implementation of the interface {@link > > org.chromium.ui.base.ViewAndroid.ViewAndroidDelegate} > > On 2016/07/21 23:24:18, sgurun wrote: > > > nit: Implementation of the abstract class > > > > Done. > > > > > https://codereview.chromium.org/2103243002/diff/300001/android_webview/java/s... > > android_webview/java/src/org/chromium/android_webview/AwContents.java:923: if > > (!mAnchorViews.containsKey(anchorView)) return; > > On 2016/07/21 23:24:18, sgurun wrote: > > > the old logic used to check view's parent and if null early out. is this not > > > necessary anymore? > > > > Thanks for catching. The code is in the super class which the flow here can > > miss. Added |isValidView()| to do the checking. > > Why is the view not valid though? Don't we only have to be careful about the > container view going away? view not valid: I looked at the code in more detail and I don't know why it was being done. So don't seem to apply here. > Which again is WebView-specific. > But I'm wondering... why don't we just check if getContainerView() returns null > in setViewPosition()? I think that would be ok.
Removed isValidView() and check containerView == null only.
lgtm https://codereview.chromium.org/2103243002/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:924: if (!mAnchorViews.containsKey(anchorView) || containerView == null) return; Yes, I think that's good enough to check because if the anchor view had been released and removed from its parent, then it would also have been removed from the |mAnchorViews| map here (in removeView()).
lgtm
Hi Ted, would you review the changes in chrome/, ui/?
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org - hashimoto@chromium.org
Hi Bo, would you review the changes in device/power_save_blocker?
https://codereview.chromium.org/2103243002/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:830: public static class AwViewAndroidDelegate extends ViewAndroidDelegate { pull out into separate file? https://codereview.chromium.org/2103243002/diff/360001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:624: @DisableHardwareAccelerationForTest why? https://codereview.chromium.org/2103243002/diff/360001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:627: public void testMovedAndRemovedAnchorViewIsNotTransferred() throws Throwable { pull everything out into a new file, maybe AwAnchorViewTest or something? https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... android_webview/native/aw_autofill_client.h:126: ui::ViewAndroid::ScopedAnchorView anchor_view_; another gc root, what's the lifetime guanratee here that this is ok and won't leak things? https://codereview.chromium.org/2103243002/diff/360001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/360001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:37: ui::ViewAndroid::ScopedAnchorView anchor_view_; this is a new gc root, ie it will prevent the anchor view from being garbage collected, probably not ok https://codereview.chromium.org/2103243002/diff/360001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/360001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:33: getContainerView().addView(anchorView); should have null checks here and below too, (even if it's not needed right now) https://codereview.chromium.org/2103243002/diff/360001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:101: protected abstract ViewGroup getContainerView(); comment here return value may be null https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_androi... ui/android/view_android.h:80: base::android::ScopedJavaGlobalRef<jobject> view_; maybe will need a weak reference version of this, I didn't check, but are all usage of this class ok with having the gc root here?
https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... android_webview/native/aw_autofill_client.h:126: ui::ViewAndroid::ScopedAnchorView anchor_view_; On 2016/07/25 22:15:26, boliu wrote: > another gc root, what's the lifetime guanratee here that this is ok and won't > leak things? For the View or the Delegate? The whole point is that it doesn't matter. That's what the ViewAndroidDelegate is for. WebView now implements this internally where it can either use a WeakRef or reset the reference when necessary. Holding on to the Delegate should be harmless. https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_androi... ui/android/view_android.h:80: base::android::ScopedJavaGlobalRef<jobject> view_; On 2016/07/25 22:15:27, boliu wrote: > maybe will need a weak reference version of this, I didn't check, but are all > usage of this class ok with having the gc root here? Hmm why would the child view be a GC root? And how would this patch change the behavior from before? The Java popup classes also reference the anchor view and they did it before this patch.
https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... android_webview/native/aw_autofill_client.h:126: ui::ViewAndroid::ScopedAnchorView anchor_view_; On 2016/07/25 22:24:40, sievers wrote: > On 2016/07/25 22:15:26, boliu wrote: > > another gc root, what's the lifetime guanratee here that this is ok and won't > > leak things? > > For the View or the Delegate? View, which has parent references back to the whole view tree, while attached anyway. > > The whole point is that it doesn't matter. That's what the ViewAndroidDelegate > is for. > WebView now implements this internally where it can either use a WeakRef or > reset the reference when necessary. > Holding on to the Delegate should be harmless. https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_androi... ui/android/view_android.h:80: base::android::ScopedJavaGlobalRef<jobject> view_; On 2016/07/25 22:24:41, sievers wrote: > On 2016/07/25 22:15:27, boliu wrote: > > maybe will need a weak reference version of this, I didn't check, but are all > > usage of this class ok with having the gc root here? > > Hmm why would the child view be a GC root? Any strong reference from native code is a gc root, ie it looks like a global variable to the garbage collector. > > And how would this patch change the behavior from before? > The Java popup classes also reference the anchor view and they did it before > this patch. Err, I dunno. How did it used to work?
https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... android_webview/native/aw_autofill_client.h:126: ui::ViewAndroid::ScopedAnchorView anchor_view_; On 2016/07/25 22:29:24, boliu wrote: > On 2016/07/25 22:24:40, sievers wrote: > > On 2016/07/25 22:15:26, boliu wrote: > > > another gc root, what's the lifetime guanratee here that this is ok and > won't > > > leak things? > > > > For the View or the Delegate? > > View, which has parent references back to the whole view tree, while attached > anyway. Actually delegate too. The delegate is holding strongly onto the anchor views, and delegate is held strongly from here too. Maybe a better pattern is this: ViewAndroidDelegate holds weakly onto (anchor|container)views Pass ViewAndroidDelegate through jni Java code gets the views from ViewAndroidDelegate Safer as there is no possibility of leak then in webview. Could even write a gc test. > > > > > The whole point is that it doesn't matter. That's what the ViewAndroidDelegate > > is for. > > WebView now implements this internally where it can either use a WeakRef or > > reset the reference when necessary. > > Holding on to the Delegate should be harmless. >
On 2016/07/25 22:29:24, boliu wrote: > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > File android_webview/native/aw_autofill_client.h (right): > > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > android_webview/native/aw_autofill_client.h:126: > ui::ViewAndroid::ScopedAnchorView anchor_view_; > On 2016/07/25 22:24:40, sievers wrote: > > On 2016/07/25 22:15:26, boliu wrote: > > > another gc root, what's the lifetime guanratee here that this is ok and > won't > > > leak things? > > > > For the View or the Delegate? > > View, which has parent references back to the whole view tree, while attached > anyway. > > > > > The whole point is that it doesn't matter. That's what the ViewAndroidDelegate > > is for. > > WebView now implements this internally where it can either use a WeakRef or > > reset the reference when necessary. > > Holding on to the Delegate should be harmless. > > https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_android.h > File ui/android/view_android.h (right): > > https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_androi... > ui/android/view_android.h:80: base::android::ScopedJavaGlobalRef<jobject> view_; > On 2016/07/25 22:24:41, sievers wrote: > > On 2016/07/25 22:15:27, boliu wrote: > > > maybe will need a weak reference version of this, I didn't check, but are > all > > > usage of this class ok with having the gc root here? > > > > Hmm why would the child view be a GC root? > > Any strong reference from native code is a gc root, ie it looks like a global > variable to the garbage collector. > > > > > And how would this patch change the behavior from before? > > The Java popup classes also reference the anchor view and they did it before > > this patch. > > Err, I dunno. How did it used to work? DropdownPopupWindow.mAnchorView exists before and after this patch. That's used for selection handles for example in WebView. All popups are created by native and referenced in a way that if either the native object that owns the View goes away or the popup is dismissed through the View system, it will delete itself. This new native reference is only there to allow more consistent and simplified adding and removal (and position update) from the code that drives this. (And the Java code doesn't care anymore where the anchor view comes from i.e. doesn't need the delegate reference also.) So I don't *think* this changes lifetime related to the anchor views. But I hadn't considered that the children we add to the View for autofill, selection handles, powersave blocker create a GC root to the WebView. It seems sketchy (again, before this patch). The only reason we need to keep a reference is so that we can update the position and also detach the View when necessary. Does that mean we should really only keep a WeakReference anywhere?
On 2016/07/25 22:37:16, sievers wrote: > On 2016/07/25 22:29:24, boliu wrote: > > > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > > File android_webview/native/aw_autofill_client.h (right): > > > > > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > > android_webview/native/aw_autofill_client.h:126: > > ui::ViewAndroid::ScopedAnchorView anchor_view_; > > On 2016/07/25 22:24:40, sievers wrote: > > > On 2016/07/25 22:15:26, boliu wrote: > > > > another gc root, what's the lifetime guanratee here that this is ok and > > won't > > > > leak things? > > > > > > For the View or the Delegate? > > > > View, which has parent references back to the whole view tree, while attached > > anyway. > > > > > > > > The whole point is that it doesn't matter. That's what the > ViewAndroidDelegate > > > is for. > > > WebView now implements this internally where it can either use a WeakRef or > > > reset the reference when necessary. > > > Holding on to the Delegate should be harmless. > > > > > https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_android.h > > File ui/android/view_android.h (right): > > > > > https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_androi... > > ui/android/view_android.h:80: base::android::ScopedJavaGlobalRef<jobject> > view_; > > On 2016/07/25 22:24:41, sievers wrote: > > > On 2016/07/25 22:15:27, boliu wrote: > > > > maybe will need a weak reference version of this, I didn't check, but are > > all > > > > usage of this class ok with having the gc root here? > > > > > > Hmm why would the child view be a GC root? > > > > Any strong reference from native code is a gc root, ie it looks like a global > > variable to the garbage collector. > > > > > > > > And how would this patch change the behavior from before? > > > The Java popup classes also reference the anchor view and they did it before > > > this patch. > > > > Err, I dunno. How did it used to work? > > DropdownPopupWindow.mAnchorView exists before and after this patch. But that's *not* a gc root, ie if DropdownPopupWindow gets collected, then that reference basically goes away as well. > > That's used for selection handles for example in WebView. > All popups are created by native and referenced in a way that if either the > native object that owns the View goes away or the popup is dismissed through the > View system, it will delete itself. Not always. PowerSaveBlocker was careful not to prevent java side from garbage collected, so view can go away before native object. I think since we are doing this large refactor, it's probably time to fix all cases. > This new native reference is only there to > allow more consistent and simplified adding and removal (and position update) > from the code that drives this. (And the Java code doesn't care anymore where > the anchor view comes from i.e. doesn't need the delegate reference also.) > > So I don't *think* this changes lifetime related to the anchor views. > > But I hadn't considered that the children we add to the View for autofill, > selection handles, powersave blocker create a GC root to the WebView. > It seems sketchy (again, before this patch). The only reason we need to keep a > reference is so that we can update the position and also detach the View when > necessary. > Does that mean we should really only keep a WeakReference anywhere? I had suggestions on how to make webview work in #79 :) Wanna comment on that?
On 2016/07/25 22:33:34, boliu wrote: > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > File android_webview/native/aw_autofill_client.h (right): > > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > android_webview/native/aw_autofill_client.h:126: > ui::ViewAndroid::ScopedAnchorView anchor_view_; > On 2016/07/25 22:29:24, boliu wrote: > > On 2016/07/25 22:24:40, sievers wrote: > > > On 2016/07/25 22:15:26, boliu wrote: > > > > another gc root, what's the lifetime guanratee here that this is ok and > > won't > > > > leak things? > > > > > > For the View or the Delegate? > > > > View, which has parent references back to the whole view tree, while attached > > anyway. > > Actually delegate too. The delegate is holding strongly onto the anchor views, > and delegate is held strongly from here too. > I was really hoping that we could deprecate contracts that are practically not enforceable from the content- and other layers. For example, IMHO it shouldn't be that WebView dispenses strong View references to the lower layers and then expects everyone to not hold one to this strongly. Rather it should pass a WeakReference, or better a delegate abstraction (and then handle the GC root issues in the WebView layer). But this whole issue in the reverse direction is interesting and maybe has been an oversight. > Maybe a better pattern is this: > ViewAndroidDelegate holds weakly onto (anchor|container)views > Pass ViewAndroidDelegate through jni > Java code gets the views from ViewAndroidDelegate > How does passing anything through Java help with the way things are referenced? Isn't this just a matter of strong ref vs. weak ref? Of course there is also a question of where that happens, but is maybe a bit more cosmetic.
On 2016/07/25 22:46:38, boliu wrote: > On 2016/07/25 22:37:16, sievers wrote: > > On 2016/07/25 22:29:24, boliu wrote: > > > > > > https://codereview.chromium.org/2103243002/diff/360001/android_webview/native... > > > File android_webview/native/aw_autofill_client.h (right): > > > > But that's *not* a gc root, ie if DropdownPopupWindow gets collected, then that > reference basically goes away as well. > But DropdownPopupWindow is basically own through the same native object that also has the extra strong ref you complained about. So I still don't see how anything changes, but still agree that it was as sketchy before. Also, yes, let's fix all of it in a good way now.
On 2016/07/25 22:46:55, sievers wrote: > Rather it should pass a WeakReference, or better a delegate abstraction (and > then handle the GC root issues in the WebView layer). WeakRefrence is not enforceable either since you can just get a strong ref from it. Delegate is waaay too much overhead since you need to wrap every single method. > > But this whole issue in the reverse direction is interesting and maybe has been > an oversight. > > > Maybe a better pattern is this: > > ViewAndroidDelegate holds weakly onto (anchor|container)views > > Pass ViewAndroidDelegate through jni > > Java code gets the views from ViewAndroidDelegate > > > > How does passing anything through Java help with the way things are referenced? > Isn't this just a matter of strong ref vs. weak ref? The common mistake is forgetting native strong refs are gc-roots. This prevents that. The issue in java is generally less of a problem, if you add comments to the java method that object should not be held strongly, or make the method return WeakReference<Foo>. Plus gc sees those references, so unless you shove it in a global variable, it *usually* doesn't matter > > Of course there is also a question of where that happens, but is maybe a bit > more cosmetic.
boliu@: Let me work on a follow-up CL that addresses your concerns better. Changed the strong ref to weak one for view/delegate used in ScopedAnchorView for now, and left a TODO to update it soon. I believe this was agreed upon (in order not to make this CL any bigger). Let me know if you see it different - I'll handle the rest of the issues. https://codereview.chromium.org/2103243002/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:830: public static class AwViewAndroidDelegate extends ViewAndroidDelegate { On 2016/07/25 22:15:26, boliu wrote: > pull out into separate file? Pulled out to AwViewAndroidDelegate.java https://codereview.chromium.org/2103243002/diff/360001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2103243002/diff/360001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:624: @DisableHardwareAccelerationForTest On 2016/07/25 22:15:26, boliu wrote: > why? Without it, the container starts with a hardware view already added to it, which is not relevant to these tests. It makes the test results (# of children views) look not intuitive - addAnchorViewTest() results in # of views 2, etc. https://codereview.chromium.org/2103243002/diff/360001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:627: public void testMovedAndRemovedAnchorViewIsNotTransferred() throws Throwable { On 2016/07/25 22:15:26, boliu wrote: > pull everything out into a new file, maybe AwAnchorViewTest or something? AwContentAnchorViewTest.java https://codereview.chromium.org/2103243002/diff/360001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/360001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:33: getContainerView().addView(anchorView); On 2016/07/25 22:15:27, boliu wrote: > should have null checks here and below too, (even if it's not needed right now) Done. https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/360001/ui/android/view_androi... ui/android/view_android.h:80: base::android::ScopedJavaGlobalRef<jobject> view_; On 2016/07/25 22:15:27, boliu wrote: > maybe will need a weak reference version of this, I didn't check, but are all > usage of this class ok with having the gc root here? Done.
https://codereview.chromium.org/2103243002/diff/380001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/380001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwViewAndroidDelegate.java:31: private WeakReference<ViewGroup> mContainerView; I don't see why WeakReference is needed here actually contract in AwContents and ContentViewCore is that native side holds a weak ref to java side, so java is free to hold strongly onto things like the container view. https://codereview.chromium.org/2103243002/diff/380001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (right): https://codereview.chromium.org/2103243002/diff/380001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:30: @DisableHardwareAccelerationForTest remove DisableHardwareAccelerationForTest and update the test to not assume the number of child views at the start of the test instead plus checking count isn't actually entirely valid, you have to actually go look for the anchor view being added https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:70: view_android->SetAnchorRect(popup_view_.view(env), need null check https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:99: view_android->SetAnchorRect(popup_.view(env), controller_->element_bounds()); null check https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.h:77: bool is_null() const { return view_.is_empty(); } this method is no longer valid, since *in theory*, gc can come in and clean up the weak ref between the is_null() check and view() with weak pointer, the semantic needs to be that client need to check whether the return value of view() is null or not.
https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:34: public AutofillPopupBridge(View anchorView, float anchorWidth, How does anchorWidth differ from anchorView.getWidth() or anchorView.getMeasuredWidth()? https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:241: return null; should we add an assert to this (and potentially getContainerView) as the equivalent of NOTREACHED in native? https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:63: if (java_object_.is_null() || popup_view_.is_null()) we are only null checking popup_view here because this is still a "valid" object even is Show fails above right? Ideally, we'd never call this in the popup_view_ == null state, but I guess that requires broader changes in the autofill code? https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:70: view_android->SetAnchorRect(popup_view_.view(env), On 2016/07/27 16:20:26, boliu wrote: > need null check for view_android right? popup_view_ is null checked above https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:643: public void initialize(ViewGroup containerView, ViewAndroidDelegate viewDelegate, why do we need to pass containerView and viewDelegate when viewDelegate has a method getContainerView? https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2360: private void showSelectPopup(View anchorView, float anchorWidth, same question about width vs view https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:29: public SelectPopupDropdown(ContentViewCore contentViewCore, View anchorView, float anchorWidth, same here too https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java:39: mViewAndroidDelegate = new ViewAndroidDelegate() { this seems to be defined in a few places...thoughts on making a simple version of this class useable all the places this is copied? https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... File device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java (left): https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java:32: delegate.setAnchorViewPosition(anchorView, 0, 0, 0, 0); do we need to do something like this for the view? or is that the default? https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... File device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java (right): https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java:37: if (mKeepScreenOnView == null) { this can all fit on a single line (and is valid in java land if the statement and condition all fit on a single line) https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:32: View anchorView = new View(getContainerView().getContext()); cache getContainerView as a local var here and in removeView. https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:43: public void removeView(View anchorView) { are all these public apis called from java? if not, they should be private if they are just called from native. https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:102: // Override to return the container view the views are added to. should be a javadoc /** */ Just specify what the return value is. The fact that it is abstract is a clear indicator that it must be overridden, so the comment should just focus on the intent of what getContainerView() does https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:19: ViewAndroid::ScopedAnchorView::ScopedAnchorView( the ordering of the .cc file should match the .h, so I think you need to move the definition of ScopedAnchorView up in the header to match this https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:50: if (!view_.is_empty()) needs braces since the condition is more than one line (at least I think we do) https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:51: Java_ViewAndroidDelegate_removeView(env, indented too much https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:101: return ViewAndroid::ScopedAnchorView(); -2 indent https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:111: if (bounds.IsEmpty()) return; return needs to go on the next line in c++ https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:170: return; -2 indent
https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:643: public void initialize(ViewGroup containerView, ViewAndroidDelegate viewDelegate, On 2016/07/27 20:10:47, Ted C wrote: > why do we need to pass containerView and viewDelegate when viewDelegate has a > method getContainerView? We could get it from ViewDelegate too. It'd be nice though if eventually there were little or no references directly to the container view from the content- and other layers (crbug.com/622864 for example) but everything would go through the delegate. Then for example that would allow WebView to deal with its GC root caveats in the webview implementation layer (instead of trying to enforce impractical contracts on all the code underneath it). That's why it's nice if ViewAndroidDelegate.getContainerView() is not public. But then again it doesn't really make a big difference I guess while we are still using it here in other places.
The CQ bit was checked by jinsukkim@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...
Patchset #14 (id:400001) has been deleted
PTAL https://codereview.chromium.org/2103243002/diff/380001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/380001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwViewAndroidDelegate.java:31: private WeakReference<ViewGroup> mContainerView; On 2016/07/27 16:20:26, boliu wrote: > I don't see why WeakReference is needed here actually > > contract in AwContents and ContentViewCore is that native side holds a weak ref > to java side, so java is free to hold strongly onto things like the container > view. In fact there was another strong ref in native side (view_android.cc). Turned that into a weak ref as well, and removed WeakReference here. https://codereview.chromium.org/2103243002/diff/380001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (right): https://codereview.chromium.org/2103243002/diff/380001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:30: @DisableHardwareAccelerationForTest On 2016/07/27 16:20:26, boliu wrote: > remove DisableHardwareAccelerationForTest and update the test to not assume the > number of child views at the start of the test instead > > plus checking count isn't actually entirely valid, you have to actually go look > for the anchor view being added Done. https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:34: public AutofillPopupBridge(View anchorView, float anchorWidth, On 2016/07/27 20:10:47, Ted C wrote: > How does anchorWidth differ from anchorView.getWidth() or > anchorView.getMeasuredWidth()? |anchorWidth| is the value from native before dip scale is applied. It is to adjust the position of the view, and only at the final step is the scale applied - before calling |setLayoutParams()|. Previously the whole native value set (x,y,width,height) was passed to Java via |setAnchorRect()| of various popups but width is actually the only value necessary for adjustment. This patch removes |setAnchorRect()| and uses constructor to pass only |anchorWidth|. It is not possible to infer the width from |anchorView| here, because the view is not laid out yet. Added a comment for clarification in |create()| below. https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:241: return null; On 2016/07/27 20:10:47, Ted C wrote: > should we add an assert to this (and potentially getContainerView) as the > equivalent of NOTREACHED in native? Done. https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:63: if (java_object_.is_null() || popup_view_.is_null()) On 2016/07/27 20:10:47, Ted C wrote: > we are only null checking popup_view here because this is still a "valid" object > even is Show fails above right? Ideally, we'd never call this in the > popup_view_ == null state, but I guess that requires broader changes in the > autofill code? That's correct. Added the null checking in defensive manner here since I was not certain if this method won't be called in the state where |popup_view_| is not set. Moved it down below as per the comment from Bo. https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:70: view_android->SetAnchorRect(popup_view_.view(env), On 2016/07/27 20:10:47, Ted C wrote: > On 2016/07/27 16:20:26, boliu wrote: > > need null check > > for view_android right? popup_view_ is null checked above Oh what Bo means by that is I need to change the way to do the null checking for this weak reference. https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:99: view_android->SetAnchorRect(popup_.view(env), controller_->element_bounds()); On 2016/07/27 16:20:26, boliu wrote: > null check Done up above. https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:643: public void initialize(ViewGroup containerView, ViewAndroidDelegate viewDelegate, On 2016/07/27 20:17:47, sievers wrote: > On 2016/07/27 20:10:47, Ted C wrote: > > why do we need to pass containerView and viewDelegate when viewDelegate has a > > method getContainerView? > > We could get it from ViewDelegate too. It'd be nice though if eventually there > were little or no references directly to the container view from the content- > and other layers (crbug.com/622864 for example) but everything would go through > the delegate. Then for example that would allow WebView to deal with its GC root > caveats in the webview implementation layer (instead of trying to enforce > impractical contracts on all the code underneath it). > > That's why it's nice if ViewAndroidDelegate.getContainerView() is not public. > But then again it doesn't really make a big difference I guess while we are > still using it here in other places. Yes we can go without it and get it from the delegate instead. Removed the param, updated the delegate's |getContainerView()| to public and used it. https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2360: private void showSelectPopup(View anchorView, float anchorWidth, On 2016/07/27 20:10:47, Ted C wrote: > same question about width vs view Added more info in the javadoc. https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:29: public SelectPopupDropdown(ContentViewCore contentViewCore, View anchorView, float anchorWidth, On 2016/07/27 20:10:47, Ted C wrote: > same here too Commented in other files. https://codereview.chromium.org/2103243002/diff/380001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java (right): https://codereview.chromium.org/2103243002/diff/380001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java:39: mViewAndroidDelegate = new ViewAndroidDelegate() { On 2016/07/27 20:10:47, Ted C wrote: > this seems to be defined in a few places...thoughts on making a simple version > of this class useable all the places this is copied? Now ViewAndroidDelegate has createBasicDelegate() for that. https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... File device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java (left): https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java:32: delegate.setAnchorViewPosition(anchorView, 0, 0, 0, 0); On 2016/07/27 20:10:47, Ted C wrote: > do we need to do something like this for the view? or is that the default? The anchor view does not need positioning since it's not viewable. It is okay not to call it. https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... File device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java (right): https://codereview.chromium.org/2103243002/diff/380001/device/power_save_bloc... device/power_save_blocker/android/java/src/org/chromium/device/power_save_blocker/PowerSaveBlocker.java:37: if (mKeepScreenOnView == null) { On 2016/07/27 20:10:47, Ted C wrote: > this can all fit on a single line (and is valid in java land if the statement > and condition all fit on a single line) Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:32: View anchorView = new View(getContainerView().getContext()); On 2016/07/27 20:10:47, Ted C wrote: > cache getContainerView as a local var here and in removeView. Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:43: public void removeView(View anchorView) { On 2016/07/27 20:10:47, Ted C wrote: > are all these public apis called from java? if not, they should be private if > they are just called from native. They should be overridable by subclasses, and called by some Java tests (except |startDragAndDrop|, which I updated to private as you suggested). https://codereview.chromium.org/2103243002/diff/380001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:102: // Override to return the container view the views are added to. On 2016/07/27 20:10:47, Ted C wrote: > should be a javadoc /** */ > > Just specify what the return value is. The fact that it is abstract is a clear > indicator that it must be overridden, so the comment should just focus on the > intent of what getContainerView() does Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:19: ViewAndroid::ScopedAnchorView::ScopedAnchorView( On 2016/07/27 20:10:48, Ted C wrote: > the ordering of the .cc file should match the .h, so I think you need to move > the definition of ScopedAnchorView up in the header to match this Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:50: if (!view_.is_empty()) On 2016/07/27 20:10:48, Ted C wrote: > needs braces since the condition is more than one line (at least I think we do) Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:51: Java_ViewAndroidDelegate_removeView(env, On 2016/07/27 20:10:48, Ted C wrote: > indented too much Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:101: return ViewAndroid::ScopedAnchorView(); On 2016/07/27 20:10:48, Ted C wrote: > -2 indent Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:111: if (bounds.IsEmpty()) return; On 2016/07/27 20:10:48, Ted C wrote: > return needs to go on the next line in c++ Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.cc:170: return; On 2016/07/27 20:10:48, Ted C wrote: > -2 indent Done. https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/380001/ui/android/view_androi... ui/android/view_android.h:77: bool is_null() const { return view_.is_empty(); } On 2016/07/27 16:20:26, boliu wrote: > this method is no longer valid, since *in theory*, gc can come in and clean up > the weak ref between the is_null() check and view() > > with weak pointer, the semantic needs to be that client need to check whether > the return value of view() is null or not. Good to know this. Removed |is_null()|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2103243002/diff/420001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/420001/android_webview/native... android_webview/native/aw_autofill_client.cc:143: const ScopedJavaLocalRef<jobject>& current_view = anchor_view_.view(); this (and line 147) below is bad c++, you can't take reference of a temporary variable, the reference becomes dangling immediately after this line, ScopedJavaLocalRef are meant to be copied, so just make a copy, compiler will probalby do return value optimization and avoid the copy anyway looks like clang caught this, but gcc didn't https://codereview.chromium.org/2103243002/diff/420001/android_webview/native... android_webview/native/aw_autofill_client.cc:154: anchor_view_.view().obj(), view.obj() https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:40: const ScopedJavaLocalRef<jobject>& view = popup_view_.view(); ditto https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:67: const ScopedJavaLocalRef<jobject>& view = popup_view_.view(); ditto https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:64: const ScopedJavaLocalRef<jobject>& view = popup_.view(); ditto https://codereview.chromium.org/2103243002/diff/420001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2103243002/diff/420001/content/browser/androi... content/browser/android/content_view_core_impl.cc:545: const ScopedJavaLocalRef<jobject>& popup_view = select_popup_.view(); ditto https://codereview.chromium.org/2103243002/diff/420001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:62: const ScopedJavaLocalRef<jobject>& popup_view = anchor_view_.view(); ditto https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.cc:52: delegate_.get(env).obj(), null check delegate_ https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.cc:164: const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); ditto https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.h:53: }; disallow copy and assign?
https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.h:53: }; On 2016/07/29 20:05:15, boliu wrote: > disallow copy and assign? I think that's already the case because of the move constructor and =operator which implies no copy default versions.
https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.cc:164: const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); and also what clang caught: /include -fno-exceptions -c ../../ui/android/view_android.cc -o obj/ui/android/android/view_android.o ../../ui/android/view_android.cc:166:12: error: returning reference to local temporary object [-Werror,-Wreturn-stack-address] return delegate; ^~~~~~~~ ../../ui/android/view_android.cc:164:38: note: binding reference variable 'delegate' here const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); ^ ~~~~~~~~~~~~~~~~~~ ../../ui/android/view_android.cc:168:56: error: returning reference to local temporary object [-Werror,-Wreturn-stack-address] return parent_ ? parent_->GetViewAndroidDelegate() : delegate; ^~~~~~~~ ../../ui/android/view_android.cc:164:38: note: binding reference variable 'delegate' here const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); ^ ~~~~~~~~~~~~~~~~~~ 2 errors generated.
The CQ bit was checked by jinsukkim@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.
https://codereview.chromium.org/2103243002/diff/420001/android_webview/native... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/2103243002/diff/420001/android_webview/native... android_webview/native/aw_autofill_client.cc:143: const ScopedJavaLocalRef<jobject>& current_view = anchor_view_.view(); On 2016/07/29 20:05:15, boliu wrote: > this (and line 147) below is bad c++, you can't take reference of a temporary > variable, the reference becomes dangling immediately after this line, > ScopedJavaLocalRef are meant to be copied, so just make a copy, compiler will > probalby do return value optimization and avoid the copy anyway > > looks like clang caught this, but gcc didn't Embarrassing :) Fixed. https://codereview.chromium.org/2103243002/diff/420001/android_webview/native... android_webview/native/aw_autofill_client.cc:154: anchor_view_.view().obj(), On 2016/07/29 20:05:15, boliu wrote: > view.obj() Done. https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:40: const ScopedJavaLocalRef<jobject>& view = popup_view_.view(); On 2016/07/29 20:05:15, boliu wrote: > ditto Done. https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:67: const ScopedJavaLocalRef<jobject>& view = popup_view_.view(); On 2016/07/29 20:05:15, boliu wrote: > ditto Done. https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/password_generation_popup_view_android.cc:64: const ScopedJavaLocalRef<jobject>& view = popup_.view(); On 2016/07/29 20:05:15, boliu wrote: > ditto Done. https://codereview.chromium.org/2103243002/diff/420001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2103243002/diff/420001/content/browser/androi... content/browser/android/content_view_core_impl.cc:545: const ScopedJavaLocalRef<jobject>& popup_view = select_popup_.view(); On 2016/07/29 20:05:15, boliu wrote: > ditto Done. https://codereview.chromium.org/2103243002/diff/420001/device/power_save_bloc... File device/power_save_blocker/power_save_blocker_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/device/power_save_bloc... device/power_save_blocker/power_save_blocker_android.cc:62: const ScopedJavaLocalRef<jobject>& popup_view = anchor_view_.view(); On 2016/07/29 20:05:15, boliu wrote: > ditto Done. https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.cc:52: delegate_.get(env).obj(), On 2016/07/29 20:05:15, boliu wrote: > null check delegate_ view_ != null && delegate_ == null is DCHECK'd in the constructor. https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.cc:164: const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); On 2016/07/29 23:44:06, sgurun-OOO wrote: > and also what clang caught: > > > /include -fno-exceptions -c ../../ui/android/view_android.cc -o > obj/ui/android/android/view_android.o > ../../ui/android/view_android.cc:166:12: error: returning reference to local > temporary object [-Werror,-Wreturn-stack-address] > return delegate; > ^~~~~~~~ > ../../ui/android/view_android.cc:164:38: note: binding reference variable > 'delegate' here > const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); > ^ ~~~~~~~~~~~~~~~~~~ > ../../ui/android/view_android.cc:168:56: error: returning reference to local > temporary object [-Werror,-Wreturn-stack-address] > return parent_ ? parent_->GetViewAndroidDelegate() : delegate; > ^~~~~~~~ > ../../ui/android/view_android.cc:164:38: note: binding reference variable > 'delegate' here > const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); > ^ ~~~~~~~~~~~~~~~~~~ > 2 errors generated. Fixed https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.cc:164: const ScopedJavaLocalRef<jobject>& delegate = delegate_.get(env); On 2016/07/29 20:05:15, boliu wrote: > ditto Done. https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/420001/ui/android/view_androi... ui/android/view_android.h:53: }; On 2016/07/29 20:05:15, boliu wrote: > disallow copy and assign? Left a comment here for the clarification.
https://codereview.chromium.org/2103243002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2103243002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:239: new ViewAndroidDelegate() { I don't own this code so feel free to ignore, but this could be a static class I think https://codereview.chromium.org/2103243002/diff/440001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/440001/ui/android/view_androi... ui/android/view_android.cc:53: delegate_.get(env).obj(), the comment was null check delegate_ as well.., but yes, should check view too :p https://codereview.chromium.org/2103243002/diff/440001/ui/android/view_androi... ui/android/view_android.cc:108: ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate()); missed this one last time, need to null check delegate here as well
https://codereview.chromium.org/2103243002/diff/440001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right): https://codereview.chromium.org/2103243002/diff/440001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:49: mAnchorWidth = anchorWidth; Umm...still confused here. This doesn't seem to be used anywhere of substance in this file. And previously, it would be updated if the anchor view were resized. Now the logic of setAnchorRect is done externally to this class, so this one will never hear of updates. We should probably be updating mAnchorWidth in the layout change listener. Why do we need mAnchorWidth to be in DP in the first place? At this point, I'm wondering why we can't do anchorView.getLayoutParams().width. While it is somewhat roundabout, it doesn't seem any weirder than this flow. Or I wonder if we could change show here to return if mAnchorWidth.getMeasuredWidth() == 0. Then we have the onLayoutChange listener below that updates the visibility when the width becomes attached to the window and sized correctly. This would delay the popup from appearing slightly, but it seems a more sensible solution if we want to really abstract this class away from the anchor view more.
https://codereview.chromium.org/2103243002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2103243002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:239: new ViewAndroidDelegate() { On 2016/08/01 18:18:57, boliu wrote: > I don't own this code so feel free to ignore, but this could be a static class I > think Acknowledged. https://codereview.chromium.org/2103243002/diff/440001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right): https://codereview.chromium.org/2103243002/diff/440001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:49: mAnchorWidth = anchorWidth; On 2016/08/01 20:44:39, Ted C wrote: > Umm...still confused here. This doesn't seem to be used anywhere of substance > in this file. > > And previously, it would be updated if the anchor view were resized. Now the > logic of setAnchorRect is done externally to this class, so this one will never > hear of updates. > > We should probably be updating mAnchorWidth in the layout change listener. > > Why do we need mAnchorWidth to be in DP in the first place? > > At this point, I'm wondering why we can't do anchorView.getLayoutParams().width. > While it is somewhat roundabout, it doesn't seem any weirder than this flow. > > Or I wonder if we could change show here to return if > mAnchorWidth.getMeasuredWidth() == 0. Then we have the onLayoutChange listener > below that updates the visibility when the width becomes attached to the window > and sized correctly. This would delay the popup from appearing slightly, but it > seems a more sensible solution if we want to really abstract this class away > from the anchor view more. Better if I could get rid of param width from the constructor. Will try your suggestion and get back to you. https://codereview.chromium.org/2103243002/diff/440001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/440001/ui/android/view_androi... ui/android/view_android.cc:53: delegate_.get(env).obj(), On 2016/08/01 18:18:58, boliu wrote: > the comment was null check delegate_ as well.., but yes, should check view too > :p Done. https://codereview.chromium.org/2103243002/diff/440001/ui/android/view_androi... ui/android/view_android.cc:108: ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate()); On 2016/08/01 18:18:58, boliu wrote: > missed this one last time, need to null check delegate here as well Done.
lgtm
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
The CQ bit was checked by jinsukkim@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...
Found it simpler to use |mAnchorView.getLayoutParams().width / dipScale| in
{PasswordGenerationPopupBridge, DropdownPopupWindow}.show() rather than passing
the unscaled value through their constructors. Verified it works as expected.
Removed the param in plumbing. PTAL
https://codereview.chromium.org/2103243002/diff/440001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right):
https://codereview.chromium.org/2103243002/diff/440001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:49: mAnchorWidth =
anchorWidth;
On 2016/08/01 21:49:25, Jinsuk wrote:
> On 2016/08/01 20:44:39, Ted C wrote:
> > Umm...still confused here. This doesn't seem to be used anywhere of
substance
> > in this file.
> >
> > And previously, it would be updated if the anchor view were resized. Now
the
> > logic of setAnchorRect is done externally to this class, so this one will
> never
> > hear of updates.
> >
> > We should probably be updating mAnchorWidth in the layout change listener.
> >
> > Why do we need mAnchorWidth to be in DP in the first place?
> >
> > At this point, I'm wondering why we can't do
> anchorView.getLayoutParams().width.
> > While it is somewhat roundabout, it doesn't seem any weirder than this
flow.
> >
> > Or I wonder if we could change show here to return if
> > mAnchorWidth.getMeasuredWidth() == 0. Then we have the onLayoutChange
> listener
> > below that updates the visibility when the width becomes attached to the
> window
> > and sized correctly. This would delay the popup from appearing slightly,
but
> it
> > seems a more sensible solution if we want to really abstract this class away
> > from the anchor view more.
>
> Better if I could get rid of param width from the constructor. Will try your
> suggestion and get back to you.
Removed the param |anchorWidth| and got the unscaled version by
|mAnchorView.getLayoutParams().width / dipScale|. This looks better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tedchoc@: please take another look.
lgtm w/ a few small remaining comments https://codereview.chromium.org/2103243002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java (right): https://codereview.chromium.org/2103243002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:121: explanationTextLinkRangeStart, explanationTextLinkRangeEnd, anchorWidthInDip); Internally in this file it just converts to px. So let's just pass the px value instead. https://codereview.chromium.org/2103243002/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: InternalAccessDelegate internalDispatcher, WebContents webContents, this looks like it could fit on total fewer lines now that you've removed one item https://codereview.chromium.org/2103243002/diff/480001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right): https://codereview.chromium.org/2103243002/diff/480001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:99: float anchorWidthInDip = mAnchorView.getLayoutParams().width / dipScale; we should add an assert that getLayoutParams().width is > 0. If someone were to use WRAP_CONTENT or MATCH_CONTENT, then this will fail in an impressive way. https://codereview.chromium.org/2103243002/diff/480001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:102: if (contentWidthInDip + padding.left + padding.right > anchorWidthInDip) { While this predates your change, this continues to baffle me. getPadding(...) as far as I can tell returns pixels. granted, they will be scaled if the density of the source image differs from the device's density...but they should be pixel values not DIP values. I still don't think we should need to convert these widths to DIPs.
The CQ bit was checked by jinsukkim@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by jinsukkim@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...
Patchset #18 (id:500001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
cc'ed keishi@ who wrote DropdownPopupWindow in case he has feedback. Will follow it up if necessary. https://codereview.chromium.org/2103243002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java (right): https://codereview.chromium.org/2103243002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PasswordGenerationPopupBridge.java:121: explanationTextLinkRangeStart, explanationTextLinkRangeEnd, anchorWidthInDip); On 2016/08/03 23:11:15, Ted C wrote: > Internally in this file it just converts to px. So let's just pass the px value > instead. Done. https://codereview.chromium.org/2103243002/diff/480001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2103243002/diff/480001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:644: InternalAccessDelegate internalDispatcher, WebContents webContents, On 2016/08/03 23:11:15, Ted C wrote: > this looks like it could fit on total fewer lines now that you've removed one > item |Internal... Dispatcher| is still too long. https://codereview.chromium.org/2103243002/diff/480001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right): https://codereview.chromium.org/2103243002/diff/480001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:99: float anchorWidthInDip = mAnchorView.getLayoutParams().width / dipScale; On 2016/08/03 23:11:15, Ted C wrote: > we should add an assert that getLayoutParams().width is > 0. If someone were to > use WRAP_CONTENT or MATCH_CONTENT, then this will fail in an impressive way. Done. https://codereview.chromium.org/2103243002/diff/480001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:102: if (contentWidthInDip + padding.left + padding.right > anchorWidthInDip) { On 2016/08/03 23:11:15, Ted C wrote: > While this predates your change, this continues to baffle me. > > getPadding(...) as far as I can tell returns pixels. granted, they will be > scaled if the density of the source image differs from the device's > density...but they should be pixel values not DIP values. > > I still don't think we should need to convert these widths to DIPs. Used px for conversion.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, sgurun@chromium.org, sievers@chromium.org, boliu@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2103243002/#ps520001 (title: "use px values/rebased")
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...)
jinsukkim@chromium.org changed reviewers: + sadrul@chromium.org
Hi sadrul@ Could you give lgtm for the change below?
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
'+ui/base/layout.h',
https://codereview.chromium.org/2103243002/diff/520001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/520001/ui/android/view_androi... ui/android/view_android.cc:125: float scale = GetScaleFactorForNativeView(this); Is this the only API you are using from ui/base? ui/base pulls in a lot of stuff. Are you sure you don't want to just use the display::Screen/display::Display API here instead to get the scale factor?
PTAL https://codereview.chromium.org/2103243002/diff/520001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2103243002/diff/520001/ui/android/view_androi... ui/android/view_android.cc:125: float scale = GetScaleFactorForNativeView(this); On 2016/08/04 15:54:50, sadrul wrote: > Is this the only API you are using from ui/base? ui/base pulls in a lot of > stuff. Are you sure you don't want to just use the > display::Screen/display::Display API here instead to get the scale factor? Pulled in display/{display,screen}.h instead.
cool. lgtm
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, boliu@chromium.org, tedchoc@chromium.org, halliwell@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2103243002/#ps540001 (title: "s/base/display")
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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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 jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, sadrul@chromium.org, boliu@chromium.org, tedchoc@chromium.org, halliwell@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2103243002/#ps560001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Factor out ContentViewAndroidDelegate Took ContentViewAndroidDelegate out of ContentViewCore so that embedders come with their own implementation of ViewAndroidDelegate. This helps content layer not have to know about Android View/WebView-specific details. BUG=617750 ========== to ========== Factor out ContentViewAndroidDelegate Took ContentViewAndroidDelegate out of ContentViewCore so that embedders come with their own implementation of ViewAndroidDelegate. This helps content layer not have to know about Android View/WebView-specific details. BUG=617750 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== Factor out ContentViewAndroidDelegate Took ContentViewAndroidDelegate out of ContentViewCore so that embedders come with their own implementation of ViewAndroidDelegate. This helps content layer not have to know about Android View/WebView-specific details. BUG=617750 ========== to ========== Factor out ContentViewAndroidDelegate Took ContentViewAndroidDelegate out of ContentViewCore so that embedders come with their own implementation of ViewAndroidDelegate. This helps content layer not have to know about Android View/WebView-specific details. BUG=617750 Committed: https://crrev.com/9ee8865d84c2156728f1b9f78fb8f8b812e4ba19 Cr-Commit-Position: refs/heads/master@{#410003} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/9ee8865d84c2156728f1b9f78fb8f8b812e4ba19 Cr-Commit-Position: refs/heads/master@{#410003}
Message was sent while issue was closed.
https://codereview.chromium.org/2103243002/diff/560001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2103243002/diff/560001/ui/android/view_androi... ui/android/view_android.h:70: void SetWindowAndroid(WindowAndroid* root_window); I think this was a bad rebase. This was removed by sievers@ in: https://codereview.chromium.org/2136373002/diff/60001/ui/android/view_android.h but added back here (and is unused)
Message was sent while issue was closed.
On 2016/08/24 17:26:54, Ted C wrote: > https://codereview.chromium.org/2103243002/diff/560001/ui/android/view_android.h > File ui/android/view_android.h (right): > > https://codereview.chromium.org/2103243002/diff/560001/ui/android/view_androi... > ui/android/view_android.h:70: void SetWindowAndroid(WindowAndroid* root_window); > I think this was a bad rebase. This was removed by sievers@ in: > https://codereview.chromium.org/2136373002/diff/60001/ui/android/view_android.h > > but added back here (and is unused) Thanks for pointing it out. Realized it later and fixing it here https://codereview.chromium.org/2272673003/ |
