Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
831903004:
[WebView] Add a new flushVisualState API to AwContents. (Closed)
|
Created:
5 years, 11 months ago by Ignacio Solla Modified:
5 years, 10 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nduca Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WebView] Add a new flushVisualState API to AwContents.
I'm taking over the implementation of this API from mkosiba, who has
written a large part of this change here:
https://codereview.chromium.org/784343002/
Committed: https://crrev.com/37c8d8b3af9e52e8309f35e7a51697c7c11e8658
Cr-Commit-Position: refs/heads/master@{#314827}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Nit #Patch Set 3 : Only draw immediately when calling API and not when explictly loading blank #Patch Set 4 : Clarify comment #
Total comments: 22
Patch Set 5 : Rename to loadBlankNow and test for paused and offscreen #
Total comments: 4
Patch Set 6 : Address review comments #
Total comments: 15
Patch Set 7 : Propagate isLoadingBlank bit to BVR, address other review comments #Patch Set 8 : Rebase #Patch Set 9 : Remove loadBlankNow and error codes. #Patch Set 10 : Fix for crbug/452530 #
Total comments: 16
Patch Set 11 : Address review comments #Patch Set 12 : int -> uint62 everywhere for real. #Messages
Total messages: 38 (5 generated)
igsolla@chromium.org changed reviewers: + jam@chromium.org, mkosiba@chromium.org, tsepez@chromium.org
tsepez@chromium.org: Please review changes in messages mkosiba@chromium.org: Please review all changes jam@chromium.org: Please review changes in content/
Messages LGTM https://codereview.chromium.org/831903004/diff/1/content/common/frame_messages.h File content/common/frame_messages.h (right): https://codereview.chromium.org/831903004/diff/1/content/common/frame_message... content/common/frame_messages.h:383: // Requests that the RenderFrame send back a response synchronized with the nit: not sure what "synchronized with the mechansim" means. After the frame has been delivered? After all visual state has been flushed?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/831903004/diff/1/content/common/frame_messages.h File content/common/frame_messages.h (right): https://codereview.chromium.org/831903004/diff/1/content/common/frame_message... content/common/frame_messages.h:383: // Requests that the RenderFrame send back a response synchronized with the On 2015/01/08 18:15:27, Tom Sepez wrote: > nit: not sure what "synchronized with the mechansim" means. After the frame has > been delivered? After all visual state has been flushed? Let me know if the comment is now clearer.
> Let me know if the comment is now clearer. Nice. Thanks.
Is the intent of the content changes that a user of WebView knows when that background color has been flushed to the screen? If so, why isn't WebContentsObserver::DidFirstVisuallyNonEmptyPaint enough?
On 2015/01/12 22:26:46, jam wrote: > Is the intent of the content changes that a user of WebView knows when that > background color has been flushed to the screen? If so, why isn't > WebContentsObserver::DidFirstVisuallyNonEmptyPaint enough? The content changes will allow us to expose a more powerful flush primitive to apps in the future and that's the main purpose of the added complexity. For example, they will be able to run some JS and wait until the results are on screen. For this use case, we don't know whether DidFirstVisuallyNonEmptyPaint corresponds to "about:blank" or a previous loaded url. Using the flush API guarantees that the contents of the page loaded before "about:blank" are not longer visible. Also, blink does not always fire the DidFirstVisuallyNonEmptyPaint, it is flaky, so we cannot rely on that.
On 2015/01/13 14:05:41, Ignacio Solla wrote: > On 2015/01/12 22:26:46, jam wrote: > > Is the intent of the content changes that a user of WebView knows when that > > background color has been flushed to the screen? If so, why isn't > > WebContentsObserver::DidFirstVisuallyNonEmptyPaint enough? > > The content changes will allow us to expose a more powerful flush primitive to > apps in the future and that's the main purpose of the added complexity. For > example, they will be able to run some JS and wait until the results are on > screen. how about we add this new code then? > > For this use case, we don't know whether DidFirstVisuallyNonEmptyPaint > corresponds to "about:blank" or a previous loaded url. oh I see, so this isn't just for the initial use of a WebView? > Using the flush API > guarantees that the contents of the page loaded before "about:blank" are not > longer visible. Also, blink does not always fire the > DidFirstVisuallyNonEmptyPaint, it is flaky, so we cannot rely on that. hmm, if DidFirstVisuallyNonEmptyPaint is flaky, then it should either be removed or fixed
On 2015/01/13 18:38:58, jam wrote: > On 2015/01/13 14:05:41, Ignacio Solla wrote: > > On 2015/01/12 22:26:46, jam wrote: > > > Is the intent of the content changes that a user of WebView knows when that > > > background color has been flushed to the screen? If so, why isn't > > > WebContentsObserver::DidFirstVisuallyNonEmptyPaint enough? > > > > The content changes will allow us to expose a more powerful flush primitive to > > apps in the future and that's the main purpose of the added complexity. For > > example, they will be able to run some JS and wait until the results are on > > screen. > > how about we add this new code then? But it is also needed for this API. It makes the implementation of this API quite simple instead of having to do something more complicated with the existing callbacks. > > > > > For this use case, we don't know whether DidFirstVisuallyNonEmptyPaint > > corresponds to "about:blank" or a previous loaded url. > > oh I see, so this isn't just for the initial use of a WebView? The use case this is intended for is an app recycling WebViews, for example in a ScrollView. > > > Using the flush API > > guarantees that the contents of the page loaded before "about:blank" are not > > longer visible. Also, blink does not always fire the > > DidFirstVisuallyNonEmptyPaint, it is flaky, so we cannot rely on that. > > hmm, if DidFirstVisuallyNonEmptyPaint is flaky, then it should either be removed > or fixed Completely agree, but that's orthogonal to this change.
On 2015/01/13 18:38:58, jam wrote: > hmm, if DidFirstVisuallyNonEmptyPaint is flaky, then it should either be removed > or fixed Well, Chrome on Android still uses it for 'syncing' the background color. The thing is that we're not really interested in the FirstVisuallyNonEmptyPaint, we're interested in the first paint, regardless of its contents. FWIW the bug for fixing DidFirstVisuallyNonEmptyPaint is http://crbug.com/398471
https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1087: if (mIsLoadingBlankImmediately && BLANK_URL.equals(url)) { this seems race-prone as we might end up running this code as a response to some previous 'random' navigation to about:blank. I guess that shouldn't be a problem in practice unless there is some code in the renderer that navigates to about:blank and then immediately somewhere else. https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1175: public void loadBlankImmediately() { maybe ignore any extra calls for as long as mIsLoadingBlankImmediately is true? https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2065: public abstract static class VisualStateFlushCallback { these usually go at the top of the file https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:18: public AwWebContentsObserver(WebContents webContents, AwContents awContents, please don't create this cyclic dependency, it just makes it that much harder to understand how the calls flow. Also, onPageFinished is a massive jumble of corner cases, let's not use that. Add this into AwContents private class AwAwOuchWebContentsOberser extends AwWebContentsObserver { @Override public void didNavigateMainFrame(String url, ...) { super.didNavigate.. this.AwContents.maybeCompleteFlushVisualState(url); } public void didFailLoad(boolean isProvisionalLoad, ...) { super.didFail... this.AwContents.maybeCompleteFlushVisualState(url); } } https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:606: final AtomicReference<AwContents> awContentsRef = new AtomicReference<>(); why do you need this? Why can't you just declare a `final AwContents awContents` field and use that? https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:715: private Bitmap drawIntoBitmap(AwContents awContents) { we already have like 2 places in tests where we do something similar (testCalledWithCorrectHasUserGestureParam, AwContentsRenderTest) - care to unify those? https://codereview.chromium.org/831903004/diff/80001/android_webview/native/a... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/native/a... android_webview/native/aw_contents.cc:1053: Java_AwContents_flushVisualStateCallback( this doesn't look like valid formatting. did you run the CL through clang-format? https://codereview.chromium.org/831903004/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/831903004/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1537: static int next_id = 1; or maybe uint64 to be extra safe :P
https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1184: mIsLoadingBlankNow = true; we probably want to ignore TouchEvents when this is true. Maybe even cause this to send a TOUHC_CANCEL to 'reset' the touch pipeline. Otherwise you'll be able to click on links even though the WebView's not drawing.
i see, ok, lgtm
Thanks for the good feedback! https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1087: if (mIsLoadingBlankImmediately && BLANK_URL.equals(url)) { On 2015/01/14 18:17:12, mkosiba wrote: > this seems race-prone as we might end up running this code as a response to some > previous 'random' navigation to about:blank. I guess that shouldn't be a problem > in practice unless there is some code in the renderer that navigates to > about:blank and then immediately somewhere else. I doubt that the race that you describe will actually happen in practice, so I'be tempted to not worry about it. If you feel that we should fix this potential problem, a solution would be to pass a token with the about:blank load to the renderer, and only clear the state when the token is received back, but I think that that would be overkill for sth. that we are not even sure that it occurs in practice. https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1175: public void loadBlankImmediately() { On 2015/01/14 18:17:12, mkosiba wrote: > maybe ignore any extra calls for as long as mIsLoadingBlankImmediately is true? Done. https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2065: public abstract static class VisualStateFlushCallback { On 2015/01/14 18:17:12, mkosiba wrote: > these usually go at the top of the file Done. https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:18: public AwWebContentsObserver(WebContents webContents, AwContents awContents, On 2015/01/14 18:17:12, mkosiba wrote: > please don't create this cyclic dependency, it just makes it that much harder to > understand how the calls flow. Also, onPageFinished is a massive jumble of > corner cases, let's not use that. > > Add this into AwContents > > private class AwAwOuchWebContentsOberser extends AwWebContentsObserver { > @Override > public void didNavigateMainFrame(String url, ...) { > super.didNavigate.. > this.AwContents.maybeCompleteFlushVisualState(url); > } > > public void didFailLoad(boolean isProvisionalLoad, ...) { > super.didFail... > this.AwContents.maybeCompleteFlushVisualState(url); > } > } Done. Thanks, good sugestion! https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:606: final AtomicReference<AwContents> awContentsRef = new AtomicReference<>(); On 2015/01/14 18:17:12, mkosiba wrote: > why do you need this? Why can't you just declare a `final AwContents awContents` > field and use that? I can only initialize awContents after createAwTestContainerViewOnMainSync once I have the testView, but I need to use it in the implementation of createAwTestContainerViewOnMainSync. https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:715: private Bitmap drawIntoBitmap(AwContents awContents) { On 2015/01/14 18:17:12, mkosiba wrote: > we already have like 2 places in tests where we do something similar > (testCalledWithCorrectHasUserGestureParam, AwContentsRenderTest) - care to unify > those? Done. https://codereview.chromium.org/831903004/diff/80001/android_webview/native/a... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/native/a... android_webview/native/aw_contents.cc:1053: Java_AwContents_flushVisualStateCallback( On 2015/01/14 18:17:12, mkosiba wrote: > this doesn't look like valid formatting. did you run the CL through > clang-format? Done. https://codereview.chromium.org/831903004/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/831903004/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1537: static int next_id = 1; On 2015/01/14 18:17:12, mkosiba wrote: > or maybe uint64 to be extra safe :P Done. https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1184: mIsLoadingBlankNow = true; On 2015/01/15 10:28:32, mkosiba wrote: > we probably want to ignore TouchEvents when this is true. Maybe even cause this > to send a TOUHC_CANCEL to 'reset' the touch pipeline. Otherwise you'll be able > to click on links even though the WebView's not drawing. Very good point. Do you mind if I address this in a smaller follow-up change? I have filed http://crbug.com/449526 so that I don't forget.
lgtm https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1087: if (mIsLoadingBlankImmediately && BLANK_URL.equals(url)) { On 2015/01/16 16:02:07, Ignacio Solla wrote: > On 2015/01/14 18:17:12, mkosiba wrote: > > this seems race-prone as we might end up running this code as a response to > some > > previous 'random' navigation to about:blank. I guess that shouldn't be a > problem > > in practice unless there is some code in the renderer that navigates to > > about:blank and then immediately somewhere else. > > I doubt that the race that you describe will actually happen in practice, so > I'be tempted to not worry about it. > > If you feel that we should fix this potential problem, a solution would be to > pass a token with the about:blank load to the renderer, and only clear the state > when the token is received back, but I think that that would be overkill for > sth. that we are not even sure that it occurs in practice. > yeath, sorry. I just wanted to say "at first glance this looks like a race, but I don't think it is". Probably worth a comment explaining why we're not using that extra token you described. https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:606: final AtomicReference<AwContents> awContentsRef = new AtomicReference<>(); On 2015/01/16 16:02:07, Ignacio Solla wrote: > On 2015/01/14 18:17:12, mkosiba wrote: > > why do you need this? Why can't you just declare a `final AwContents > awContents` > > field and use that? > > I can only initialize awContents after createAwTestContainerViewOnMainSync once > I have the testView, but I need to use it in the implementation of > createAwTestContainerViewOnMainSync. oh, I didn't notice the "circular" ref. This is fine then https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:715: private Bitmap drawIntoBitmap(AwContents awContents) { On 2015/01/16 16:02:07, Ignacio Solla wrote: > On 2015/01/14 18:17:12, mkosiba wrote: > > we already have like 2 places in tests where we do something similar > > (testCalledWithCorrectHasUserGestureParam, AwContentsRenderTest) - care to > unify > > those? > > Done. Thanks! https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1184: mIsLoadingBlankNow = true; On 2015/01/16 16:02:08, Ignacio Solla wrote: > On 2015/01/15 10:28:32, mkosiba wrote: > > we probably want to ignore TouchEvents when this is true. Maybe even cause > this > > to send a TOUHC_CANCEL to 'reset' the touch pipeline. Otherwise you'll be able > > to click on links even though the WebView's not drawing. > > Very good point. Do you mind if I address this in a smaller follow-up change? I > have filed http://crbug.com/449526 so that I don't forget. separate patch is fine. you probably want to add Jared to the review when you're sending it out. https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:583: // Waiting for didNavigateMainFrame before flashing ensures that the flush callback will nit: before flushing? https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1024: public Context getContext() { I don't think you need this, the base test class has a getActivity() method https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1214: mIsLoadingBlankNow = false; I think Bo mentioned you need to postInvalidateOnAnimation from here https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:573: There are enough of these tests to warrant a separate test class, maybe VisualStateTest ? this class is for "misc" stuff that we didn't know where to put :P https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:750: ((Activity) awContents.getContext()).getWindowManager().getDefaultDisplay().getSize(size); you can use getActivity() in the test case instead of this :) https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/util/GraphicsTestUtils.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/util/GraphicsTestUtils.java:15: public class GraphicsTestUtils { thanks for doing this! https://codereview.chromium.org/831903004/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/831903004/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:183: for (const auto& iter : flush_visual_state_callbacks_) { IIRC clang-format says no space before the :
In addition to the WebView tests in this CL, I'll add content level tests in a follow-up patch. I have filed crbug/450042 as a reminder. https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1087: if (mIsLoadingBlankImmediately && BLANK_URL.equals(url)) { On 2015/01/19 11:15:57, mkosiba wrote: > On 2015/01/16 16:02:07, Ignacio Solla wrote: > > On 2015/01/14 18:17:12, mkosiba wrote: > > > this seems race-prone as we might end up running this code as a response to > > some > > > previous 'random' navigation to about:blank. I guess that shouldn't be a > > problem > > > in practice unless there is some code in the renderer that navigates to > > > about:blank and then immediately somewhere else. > > > > I doubt that the race that you describe will actually happen in practice, so > > I'be tempted to not worry about it. > > > > If you feel that we should fix this potential problem, a solution would be to > > pass a token with the about:blank load to the renderer, and only clear the > state > > when the token is received back, but I think that that would be overkill for > > sth. that we are not even sure that it occurs in practice. > > > > yeath, sorry. I just wanted to say "at first glance this looks like a race, but > I don't think it is". Probably worth a comment explaining why we're not using > that extra token you described. Acknowledged. https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:606: final AtomicReference<AwContents> awContentsRef = new AtomicReference<>(); On 2015/01/19 11:15:57, mkosiba wrote: > On 2015/01/16 16:02:07, Ignacio Solla wrote: > > On 2015/01/14 18:17:12, mkosiba wrote: > > > why do you need this? Why can't you just declare a `final AwContents > > awContents` > > > field and use that? > > > > I can only initialize awContents after createAwTestContainerViewOnMainSync > once > > I have the testView, but I need to use it in the implementation of > > createAwTestContainerViewOnMainSync. > > oh, I didn't notice the "circular" ref. This is fine then Acknowledged. https://codereview.chromium.org/831903004/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:715: private Bitmap drawIntoBitmap(AwContents awContents) { On 2015/01/19 11:15:57, mkosiba wrote: > On 2015/01/16 16:02:07, Ignacio Solla wrote: > > On 2015/01/14 18:17:12, mkosiba wrote: > > > we already have like 2 places in tests where we do something similar > > > (testCalledWithCorrectHasUserGestureParam, AwContentsRenderTest) - care to > > unify > > > those? > > > > Done. > > Thanks! Acknowledged. https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/100001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1184: mIsLoadingBlankNow = true; On 2015/01/19 11:15:58, mkosiba wrote: > On 2015/01/16 16:02:08, Ignacio Solla wrote: > > On 2015/01/15 10:28:32, mkosiba wrote: > > > we probably want to ignore TouchEvents when this is true. Maybe even cause > > this > > > to send a TOUHC_CANCEL to 'reset' the touch pipeline. Otherwise you'll be > able > > > to click on links even though the WebView's not drawing. > > > > Very good point. Do you mind if I address this in a smaller follow-up change? > I > > have filed http://crbug.com/449526 so that I don't forget. > > separate patch is fine. you probably want to add Jared to the review when you're > sending it out. Acknowledged. https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:583: // Waiting for didNavigateMainFrame before flashing ensures that the flush callback will On 2015/01/19 11:15:58, mkosiba wrote: > nit: before flushing? Done. https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1024: public Context getContext() { On 2015/01/19 11:15:58, mkosiba wrote: > I don't think you need this, the base test class has a getActivity() method Done. https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1214: mIsLoadingBlankNow = false; On 2015/01/19 11:15:58, mkosiba wrote: > I think Bo mentioned you need to postInvalidateOnAnimation from here I propagate this bit to BVR. I'll ask Bo if we still need to postInvalidate given that BVR ensures continues invalidation while the bit is set. https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:573: On 2015/01/19 11:15:58, mkosiba wrote: > There are enough of these tests to warrant a separate test class, maybe > VisualStateTest ? this class is for "misc" stuff that we didn't know where to > put :P Done. https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:750: ((Activity) awContents.getContext()).getWindowManager().getDefaultDisplay().getSize(size); On 2015/01/19 11:15:58, mkosiba wrote: > you can use getActivity() in the test case instead of this :) Done. https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/util/GraphicsTestUtils.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/util/GraphicsTestUtils.java:15: public class GraphicsTestUtils { On 2015/01/19 11:15:58, mkosiba wrote: > thanks for doing this! Acknowledged. https://codereview.chromium.org/831903004/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/831903004/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:183: for (const auto& iter : flush_visual_state_callbacks_) { On 2015/01/19 11:15:58, mkosiba wrote: > IIRC clang-format says no space before the : Done.
https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1214: mIsLoadingBlankNow = false; On 2015/01/19 18:26:41, Ignacio Solla wrote: > On 2015/01/19 11:15:58, mkosiba wrote: > > I think Bo mentioned you need to postInvalidateOnAnimation from here > > I propagate this bit to BVR. I'll ask Bo if we still need to postInvalidate > given that BVR ensures continues invalidation while the bit is set. Bo confirmed offline that EnsureContinuousInvalidate is enough and already handles this, so we don't need an additional call to postInvalidate when we unset the bit.
igsolla@chromium.org changed reviewers: + boliu@chromium.org
On 2015/01/19 18:31:57, Ignacio Solla wrote: > https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/831903004/diff/120001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwContents.java:1214: > mIsLoadingBlankNow = false; > On 2015/01/19 18:26:41, Ignacio Solla wrote: > > On 2015/01/19 11:15:58, mkosiba wrote: > > > I think Bo mentioned you need to postInvalidateOnAnimation from here > > > > I propagate this bit to BVR. I'll ask Bo if we still need to postInvalidate > > given that BVR ensures continues invalidation while the bit is set. > > Bo confirmed offline that EnsureContinuousInvalidate is enough and already > handles this, so we don't need an additional call to postInvalidate when we > unset the bit. Please Bo, take a look at BVR and LGTM if you're happy.
On 2015/01/19 18:47:44, Ignacio Solla wrote: > Please Bo, take a look at BVR and LGTM if you're happy. browser_view_renderer lgtm
aelias@chromium.org changed reviewers: + aelias@chromium.org
I heard about this new API from Bo today and I'm skeptical about it. It looks like this is a redundant, pure convenience wrapper around stuff we can achieve with the Flush API. While that means it should be a manageable maintenance burden, it also feels like poor API design (layering-violationy) to put this in the same giant WebView class that also will have the Flush API, since it belongs in a higher level of abstraction. This led me to think, it seems there's a whole class of issues around groups of multiple/recycled WebViews that developers have trouble dealing with and might justify creating a new wrapper class called something like WebViewContainer. We could create several modes of container for the typical use cases: recycler list view, side-swiping panels, etc, representing efficient, jank-free ways of doing that without making every developer poorly roll their own. That would be a good place to put convenience APIs like this.
On 2015/01/21 01:59:19, aelias wrote: > I heard about this new API from Bo today and I'm skeptical about it. It looks > like this is a redundant, pure convenience wrapper around stuff we can achieve > with the Flush API. > While that means it should be a manageable maintenance > burden, it also feels like poor API design (layering-violationy) to put this in > the same giant WebView class that also will have the Flush API, since it belongs > in a higher level of abstraction. Not true. This API is not merely a convenience wrapper around the flush API as it also needs access to BVR to ensure that the compositor does not get stuck. As such, it cannot be implemented as a layer above. > > This led me to think, it seems there's a whole class of issues around groups of > multiple/recycled WebViews that developers have trouble dealing with and might > justify creating a new wrapper class called something like WebViewContainer. We > could create several modes of container for the typical use cases: recycler list > view, side-swiping panels, etc, representing efficient, jank-free ways of doing > that without making every developer poorly roll their own. That would be a good > place to put convenience APIs like this. Good idea but orthogonal to this API for the reason above.
On 2015/01/21 at 11:43:38, igsolla wrote: > On 2015/01/21 01:59:19, aelias wrote: > > I heard about this new API from Bo today and I'm skeptical about it. It looks > > like this is a redundant, pure convenience wrapper around stuff we can achieve > > with the Flush API. > > While that means it should be a manageable maintenance > > burden, it also feels like poor API design (layering-violationy) to put this in > > the same giant WebView class that also will have the Flush API, since it belongs > > in a higher level of abstraction. > > Not true. This API is not merely a convenience wrapper around the flush API as it also needs access to BVR to ensure that the compositor does not get stuck. As such, it cannot be implemented as a layer above. That contradicts what mkosiba@ said in an email thread today; he agreed with me that it can be fully a wrapper. I think if Flush doesn't cover this use case then that calls for a change to Flush, and I also think that this API would be a harmful maintenance burden if we cannot implement it in terms of Flush. Could you explain exactly what scenario you mean by "needs access to BVR to ensure that the compositor does not get stuck" and why Flush can't address it?
On 2015/01/21 18:43:24, aelias wrote: > > Not true. This API is not merely a convenience wrapper around the flush API as > it also needs access to BVR to ensure that the compositor does not get stuck. As > such, it cannot be implemented as a layer above. > > That contradicts what mkosiba@ said in an email thread today; he agreed with me > that it can be fully a wrapper. I think if Flush doesn't cover this use case > then that calls for a change to Flush, and I also think that this API would be a > harmful maintenance burden if we cannot implement it in terms of Flush. Could > you explain exactly what scenario you mean by "needs access to BVR to ensure > that the compositor does not get stuck" and why Flush can't address it? So actually both of you are right, I think. The way this API is currently implemented means we're not calling the BVR from onDraw at all. This causes some problems (we get a frame of stale content and activations are slower as they rely on fallback tick I think) We could just make that call to BVR and then draw background color over what compositor just drew. It would be more expensive but it would not require any extra plumbing and would be 100% "kosher". I guess the fact that we need the extra state in BVR is caused by the pre-rasterization not being implemented yet?
Bo just told me the fallback tick problem arises only in the case where the WebView is currently onPaused. This is a not particularly useful corner case and we could easily address it in the Flush API by mandating the WebView is not paused and failing with an error code if it is. But because loadBlankNow has no way to error out, it's forced to have a complex workaround for this scenario. Is that an accurate summary of the situation? If so, this sounds like an unnecessarily messy problem space we're incurring on ourselves by not just committing to Flush.
On 2015/01/21 at 20:36:38, aelias wrote: > Bo just told me the fallback tick problem arises only in the case where the WebView is currently onPaused. This is a not particularly useful corner case and we could easily address it in the Flush API by mandating the WebView is not paused and failing with an error code if it is. But because loadBlankNow has no way to error out, it's forced to have a complex workaround for this scenario. Is that an accurate summary of the situation? If so, this sounds like an unnecessarily messy problem space we're incurring on ourselves by not just committing to Flush. OK, on further thought, the above summary is *not* complete. We could avoid the fallback tick problem entirely, requiring neither an error code nor a workaround, if we tied the system to activation instead of swap.
On 2015/01/21 22:25:46, aelias wrote: > OK, on further thought, the above summary is *not* complete. We could avoid the > fallback tick problem entirely, requiring neither an error code nor a > workaround, if we tied the system to activation instead of swap. I've actually tried this before and at the time I was implementing the SendMessage API activations required the fallback tick. Is this not the case anymore?
On 2015/01/22 11:10:06, mkosiba wrote: > On 2015/01/21 22:25:46, aelias wrote: > > OK, on further thought, the above summary is *not* complete. We could avoid > the > > fallback tick problem entirely, requiring neither an error code nor a > > workaround, if we tied the system to activation instead of swap. > > I've actually tried this before and at the time I was implementing the > SendMessage > API activations required the fallback tick. Is this not the case anymore? There is a plan to get rid of fallback ticks in webview, at least for all the existing use cases: crbug.com/439275. I haven't thought about how it will/should interact with the flush api.
On 2015/01/22 16:43:28, boliu wrote: > On 2015/01/22 11:10:06, mkosiba wrote: > > On 2015/01/21 22:25:46, aelias wrote: > > > OK, on further thought, the above summary is *not* complete. We could avoid > > the > > > fallback tick problem entirely, requiring neither an error code nor a > > > workaround, if we tied the system to activation instead of swap. > > > > I've actually tried this before and at the time I was implementing the > > SendMessage > > API activations required the fallback tick. Is this not the case anymore? > > There is a plan to get rid of fallback ticks in webview, at least for all the > existing use cases: crbug.com/439275. I haven't thought about how it will/should > interact with the flush api. As discussed offline we have decided to remove the loadBlankNow API and instead focus on the flush API immediately. No big changes in the last patch set to existing code, mainly just updating the tests and removing the error codes from the flush callback. Please take another look and confirm that you're happy. Unless I hear otherwise I will land this change in a couple of days or so.
https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2160: new Handler().post(new Runnable() { uh.. creating a new handler... could we maybe obtain the Handler from the containerview and use that instead? https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java (right): https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java:104: Bitmap rebBitmap = GraphicsTestUtils.drawAwContents( reb? you mean red right? also, maybe just call it 'screenshot' or something https://codereview.chromium.org/831903004/diff/200001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/831903004/diff/200001/content/common/frame_me... content/common/frame_messages.h:387: IPC_MESSAGE_ROUTED1(FrameMsg_FlushVisualStateRequest, int /* id */) weren't these supposed to be int64? https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... content/renderer/render_frame_impl.cc:1453: void RenderFrameImpl::OnFlushVisualStateRequest(int id) { int64? https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... content/renderer/render_frame_impl.h:602: void OnFlushVisualStateRequest(int key); int64?
aw still lgtm https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2160: new Handler().post(new Runnable() { On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > uh.. creating a new handler... could we maybe obtain the Handler from the > containerview and use that instead? Let's keep this api fast and avoid any java allocation if possible, so should get rid of the new Runnable as well. In that sense apps needing to allocate a new VisualStateFlushCallback kinda sucks too :( I'm ok with filing a bug for this and fix it later, but definitely before we ship. https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java (right): https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java:68: final AtomicReference<AwContents> awContentsRef = new AtomicReference<>(); Minor comment. TestAwContentsClient really needs a SetAwContents :/
https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2160: new Handler().post(new Runnable() { On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > uh.. creating a new handler... could we maybe obtain the Handler from the > containerview and use that instead? Done. https://codereview.chromium.org/831903004/diff/200001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2160: new Handler().post(new Runnable() { On 2015/01/30 01:45:42, boliu wrote: > On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > > uh.. creating a new handler... could we maybe obtain the Handler from the > > containerview and use that instead? > > Let's keep this api fast and avoid any java allocation if possible, so should > get rid of the new Runnable as well. > > In that sense apps needing to allocate a new VisualStateFlushCallback kinda > sucks too :( > > I'm ok with filing a bug for this and fix it later, but definitely before we > ship. The Runnable will not be needed when we move to activations, so no point on worrying about that (we already have a bug for that: crbug/452530). I have filed crbug/455651 for the VisualStateFlushCallback https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java (right): https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java:68: final AtomicReference<AwContents> awContentsRef = new AtomicReference<>(); On 2015/01/30 01:45:42, boliu wrote: > Minor comment. TestAwContentsClient really needs a SetAwContents :/ Acknowledged. https://codereview.chromium.org/831903004/diff/200001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/VisualStateTest.java:104: Bitmap rebBitmap = GraphicsTestUtils.drawAwContents( On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > reb? you mean red right? also, maybe just call it 'screenshot' or something Done. https://codereview.chromium.org/831903004/diff/200001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/831903004/diff/200001/content/common/frame_me... content/common/frame_messages.h:387: IPC_MESSAGE_ROUTED1(FrameMsg_FlushVisualStateRequest, int /* id */) On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > weren't these supposed to be int64? Done. https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... content/renderer/render_frame_impl.cc:1453: void RenderFrameImpl::OnFlushVisualStateRequest(int id) { On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > int64? Done. https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... content/renderer/render_frame_impl.cc:1453: void RenderFrameImpl::OnFlushVisualStateRequest(int id) { On 2015/01/27 22:07:20, mkosiba (inactive) wrote: > int64? Done. https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... content/renderer/render_frame_impl.h:602: void OnFlushVisualStateRequest(int key); On 2015/01/27 22:07:21, mkosiba (inactive) wrote: > int64? Done. https://codereview.chromium.org/831903004/diff/200001/content/renderer/render... content/renderer/render_frame_impl.h:602: void OnFlushVisualStateRequest(int key); On 2015/01/27 22:07:21, mkosiba (inactive) wrote: > int64? Done.
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/831903004/240001
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/37c8d8b3af9e52e8309f35e7a51697c7c11e8658 Cr-Commit-Position: refs/heads/master@{#314827} |
