Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22849016:
Implement clearView to match classic (Closed)
|
Created:
7 years, 4 months ago by Kristian Monsen Modified:
7 years, 3 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement clearView to match classic
Note that it needs a separate CL downstream to actually call this
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224511
Patch Set 1 #Patch Set 2 : Removed stray newline #Patch Set 3 : Add clipping and invalidation #
Total comments: 6
Patch Set 4 : Invalidating #
Total comments: 11
Messages
Total messages: 16 (0 generated)
It makes parts (that starts out outside of the view) of the webview white. It varies a bit how much and if it happens at all, but quite reliable. But it is never everything that is outside of the viewport.
likely missing some invalidate() calls https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:739: nativeEnableOnNewPicture(mNativeAwContents, true); invalidate(); https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:762: nativeEnableOnNewPicture(mNativeAwContents, enabled); suggest collecting all three nativeEnableOnNewPicture() calls into a single make-it-so method: syncOnNewPictureStateToNative() { nativeEnableOnNewPicture(mNativeAwContents, mPictureListenerEnabled || mClearViewActive); https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:1703: mClearViewActive = false; if (mClearViewActive) { invalidate(); mClearViewActive = false; syncOnNewPictureStateToNative(); } could even change the last line to: if (!mPictureListenerEnabled) { syncOnNewPictureStateToNative(); return; }
It seems to help, but I feel the view is more "sluggy" afterwards? Not really sure how that can be, so maybe it was just always like that. https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:739: nativeEnableOnNewPicture(mNativeAwContents, true); On 2013/08/16 21:41:04, joth wrote: > invalidate(); Done. https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:762: nativeEnableOnNewPicture(mNativeAwContents, enabled); On 2013/08/16 21:41:04, joth wrote: > suggest collecting all three nativeEnableOnNewPicture() calls into a single > make-it-so method: > > syncOnNewPictureStateToNative() { > nativeEnableOnNewPicture(mNativeAwContents, mPictureListenerEnabled || > mClearViewActive); Done. https://codereview.chromium.org/22849016/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:1703: mClearViewActive = false; On 2013/08/16 21:41:04, joth wrote: > if (mClearViewActive) { > invalidate(); > mClearViewActive = false; > syncOnNewPictureStateToNative(); > } > > could even change the last line to: > if (!mPictureListenerEnabled) { > syncOnNewPictureStateToNative(); > return; > } Done.
https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:715: mContainerView.invalidate(); Badness right there...should let InProcessViewRenderer know. Follow up clean up way down the road I guess. https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:740: mContainerView.invalidate(); Is this invalidate needed? Documentation only says next onDraw, doesn't say that this has to cause a redraw. Documentation also mentions something about onMeasure, but doesn't matter much there. Also should use postInvalidateOnAnimation lest we break ics upstream. https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:1707: mContainerView.invalidate(); Again is this needed? If cc activates, it should send invalidate pretty quickly after that already.
https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:715: mContainerView.invalidate(); On 2013/08/16 23:04:55, boliu wrote: > Badness right there...should let InProcessViewRenderer know. Follow up clean up > way down the road I guess. could you elaborate why invalidating the view is bad?
https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:715: mContainerView.invalidate(); On 2013/08/19 16:14:14, mkosiba wrote: > On 2013/08/16 23:04:55, boliu wrote: > > Badness right there...should let InProcessViewRenderer know. Follow up clean > up > > way down the road I guess. > > could you elaborate why invalidating the view is bad? InProcessViewRenderer has logic for triggering/throttling invalidates and scheduling the fallback tick. We should combine these and not have two independent invalidate loops. So mostly a refactor issue. Also not so bad post-jb, but should call AwContents.postInvalidateOnAnimation instead of straight up invalidate as well.
https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:715: mContainerView.invalidate(); On 2013/08/19 16:21:49, boliu wrote: > On 2013/08/19 16:14:14, mkosiba wrote: > > On 2013/08/16 23:04:55, boliu wrote: > > > Badness right there...should let InProcessViewRenderer know. Follow up clean > > up > > > way down the road I guess. > > > > could you elaborate why invalidating the view is bad? > > InProcessViewRenderer has logic for triggering/throttling invalidates and > scheduling the fallback tick. We should combine these and not have two > independent invalidate loops. So mostly a refactor issue. > > Also not so bad post-jb, but should call AwContents.postInvalidateOnAnimation > instead of straight up invalidate as well. yeath, but this isn't an invalidate for the contents, it's an invalidate for the mOverScrollGlow which is drawing on top of what InProcessViewRenderer draws.
PTAL, I would like to land this one today. https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:715: mContainerView.invalidate(); On 2013/08/19 16:30:17, mkosiba wrote: > On 2013/08/19 16:21:49, boliu wrote: > > On 2013/08/19 16:14:14, mkosiba wrote: > > > On 2013/08/16 23:04:55, boliu wrote: > > > > Badness right there...should let InProcessViewRenderer know. Follow up > clean > > > up > > > > way down the road I guess. > > > > > > could you elaborate why invalidating the view is bad? > > > > InProcessViewRenderer has logic for triggering/throttling invalidates and > > scheduling the fallback tick. We should combine these and not have two > > independent invalidate loops. So mostly a refactor issue. > > > > Also not so bad post-jb, but should call AwContents.postInvalidateOnAnimation > > instead of straight up invalidate as well. > > yeath, but this isn't an invalidate for the contents, it's an invalidate for the > mOverScrollGlow which is drawing on top of what InProcessViewRenderer draws. Not related to this change :-) https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:740: mContainerView.invalidate(); On 2013/08/16 23:04:55, boliu wrote: > Is this invalidate needed? Documentation only says next onDraw, doesn't say that > this has to cause a redraw. > > Documentation also mentions something about onMeasure, but doesn't matter much > there. > > Also should use postInvalidateOnAnimation lest we break ics upstream. The behavior we are trying to match is pretty much a direct redraw. This works at least :-) https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:1707: mContainerView.invalidate(); On 2013/08/16 23:04:55, boliu wrote: > Again is this needed? If cc activates, it should send invalidate pretty quickly > after that already. I think it is safe to keep it in? Remember this is very rarely called, so I don't think we need to worry about performance concerns.
lgtm % last comment https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:703: canvas.drawColor(getEffectiveBackgroundColor()); Documentation says draws white background, not whatever was set by app. We should do what it does, not what it says, did you double check what old webview did?
lgtm with the background color fix/confirmed
https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/22849016/diff/9001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:703: canvas.drawColor(getEffectiveBackgroundColor()); On 2013/09/20 20:55:49, boliu wrote: > Documentation says draws white background, not whatever was set by app. We > should do what it does, not what it says, did you double check what old webview > did? Classic actually does the background color as well. This is just insane. Oh well.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/22849016/9001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
On 2013/09/20 22:43:00, I haz the power (commit-bot) wrote: > Retried try job too often on linux_chromeos for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Adding a new method in Android webview that is not called by anyone. NOTRY=true
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/22849016/9001
Message was sent while issue was closed.
Change committed as 224511 |
