[WebView] Remove onFailure from VisualStateCallback.

[WebView] Remove onFailure from VisualStateCallback.

onFailure is only invoked when the render frame has
been destroyed, but it is not really useful to
distinguish onComplete from onFailure in that case.
This also addresses the feedback that we received
from API Council.

BUG=459539, 457184
COLLABORATOR=tobiasjs@chromium.org

[Ignacio Solla, 2015-02-19 18:28:09]

igsolla@chromium.org changed reviewers:
+ boliu@chromium.org, jam@chromium.org

[boliu, 2015-02-19 18:43:42]

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:207: }
There's a bit inconsistent between renderer and browser shutdown.

In renderer shutdown, all the callback still fire (with COMMIT_FAILS). But now
if browser is shutdown first, all the callbacks are just ignored.

How about keep this block, so at least the content layer guarantees these
callbacks runs no matter what, and it's up for android_webview to decide what to
do in those cases.

https://codereview.chromium.org/943633003/diff/1/content/public/browser/rende...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/943633003/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:101: typedef base::Callback<void()>
VisualStateCallback;
base::Closure

[Ignacio Solla, 2015-02-20 11:10:41]

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:207: }
On 2015/02/19 18:43:41, boliu wrote:
> There's a bit inconsistent between renderer and browser shutdown.
> 
> In renderer shutdown, all the callback still fire (with COMMIT_FAILS). But now
> if browser is shutdown first, all the callbacks are just ignored.
> 
> How about keep this block, so at least the content layer guarantees these
> callbacks runs no matter what, and it's up for android_webview to decide what
to
> do in those cases.

Thanks, good catch. 

So how about do not deliver the callback if the renderer is shuting down? I
think I prefer that option so that the content layer guarantees that these
callbacks only run when the contents are ready. Do you agree? I'm happy to
change it to whatever you want though.

https://codereview.chromium.org/943633003/diff/1/content/public/browser/rende...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/943633003/diff/1/content/public/browser/rende...
content/public/browser/render_frame_host.h:101: typedef base::Callback<void()>
VisualStateCallback;
On 2015/02/19 18:43:41, boliu wrote:
> base::Closure

Done.

[boliu, 2015-02-20 16:50:54]

boliu@chromium.org changed reviewers:
+ piman@chromium.org

[boliu, 2015-02-20 16:50:55]

+piman

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:207: }
On 2015/02/20 11:10:41, Ignacio Solla wrote:
> On 2015/02/19 18:43:41, boliu wrote:
> > There's a bit inconsistent between renderer and browser shutdown.
> > 
> > In renderer shutdown, all the callback still fire (with COMMIT_FAILS). But
now
> > if browser is shutdown first, all the callbacks are just ignored.
> > 
> > How about keep this block, so at least the content layer guarantees these
> > callbacks runs no matter what, and it's up for android_webview to decide
what
> to
> > do in those cases.
> 
> Thanks, good catch. 
> 
> So how about do not deliver the callback if the renderer is shuting down? I
> think I prefer that option so that the content layer guarantees that these
> callbacks only run when the contents are ready. Do you agree? I'm happy to
> change it to whatever you want though.

+piman to decide

I like hard guarantees over silent "failures". Also I think SwapPromise is used
for other features not related to webview, and I don't know what requirements
those features have about this.

[Ignacio Solla, 2015-02-20 16:56:54]

On 2015/02/20 16:50:55, boliu wrote:
> +piman
> 
>
https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
> File content/browser/frame_host/render_frame_host_impl.cc (left):
> 
>
https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
> content/browser/frame_host/render_frame_host_impl.cc:207: }
> On 2015/02/20 11:10:41, Ignacio Solla wrote:
> > On 2015/02/19 18:43:41, boliu wrote:
> > > There's a bit inconsistent between renderer and browser shutdown.
> > > 
> > > In renderer shutdown, all the callback still fire (with COMMIT_FAILS). But
> now
> > > if browser is shutdown first, all the callbacks are just ignored.
> > > 
> > > How about keep this block, so at least the content layer guarantees these
> > > callbacks runs no matter what, and it's up for android_webview to decide
> what
> > to
> > > do in those cases.
> > 
> > Thanks, good catch. 
> > 
> > So how about do not deliver the callback if the renderer is shuting down? I
> > think I prefer that option so that the content layer guarantees that these
> > callbacks only run when the contents are ready. Do you agree? I'm happy to
> > change it to whatever you want though.
> 
> +piman to decide
> 
> I like hard guarantees over silent "failures". Also I think SwapPromise is
used
> for other features not related to webview, and I don't know what requirements
> those features have about this.

I like true positives over false positives :)

We have our own implementation of a SwapPromise for the visual state stuff, ie.
queue_message_swap_promise.cc. This is used to:
1. deliver the didFirstNonEmptyPaint with the visual state
2. for the implementation of InsertVisualStateCallback
We can anything that we want there without affecting others.

[piman, 2015-02-20 18:37:31]

On 2015/02/20 16:50:55, boliu wrote:
> +piman
> 
>
https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
> File content/browser/frame_host/render_frame_host_impl.cc (left):
> 
>
https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
> content/browser/frame_host/render_frame_host_impl.cc:207: }
> On 2015/02/20 11:10:41, Ignacio Solla wrote:
> > On 2015/02/19 18:43:41, boliu wrote:
> > > There's a bit inconsistent between renderer and browser shutdown.
> > > 
> > > In renderer shutdown, all the callback still fire (with COMMIT_FAILS). But
> now
> > > if browser is shutdown first, all the callbacks are just ignored.
> > > 
> > > How about keep this block, so at least the content layer guarantees these
> > > callbacks runs no matter what, and it's up for android_webview to decide
> what
> > to
> > > do in those cases.
> > 
> > Thanks, good catch. 
> > 
> > So how about do not deliver the callback if the renderer is shuting down? I
> > think I prefer that option so that the content layer guarantees that these
> > callbacks only run when the contents are ready. Do you agree? I'm happy to
> > change it to whatever you want though.
> 
> +piman to decide
> 
> I like hard guarantees over silent "failures". Also I think SwapPromise is
used
> for other features not related to webview, and I don't know what requirements
> those features have about this.

I like hard guarantees too. Otherwise, some code might be tempted to delay work
until the callback comes, which will never happen causing leaks etc. And then
the natural outcome tends to be for the callsite to reverse-engineer when the
callback may not come by observing the frame or the renderer and that leads to
poorer, more fragile code.

This is what we did for e.g. asynchronous layer capture / screenshots where it
is in theory possible to know when the callback will not come, but is very
fragile (lots of conditions to test) but when instead we call the callback with
a failure enum it dramatically reduces the coupling.

Bottom line, I think at the content layer you should keep the callback, and
maybe deal with it in the webview code if that's the contract you have with the
embedder.

[Ignacio Solla, 2015-02-24 12:49:35]

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/943633003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:207: }
On 2015/02/19 18:43:41, boliu wrote:
> There's a bit inconsistent between renderer and browser shutdown.
> 
> In renderer shutdown, all the callback still fire (with COMMIT_FAILS). But now
> if browser is shutdown first, all the callbacks are just ignored.
> 
> How about keep this block, so at least the content layer guarantees these
> callbacks runs no matter what, and it's up for android_webview to decide what
to
> do in those cases.

Done.

[boliu, 2015-02-24 17:55:37]

aw lgtm

[piman, 2015-02-24 21:51:23]

https://codereview.chromium.org/943633003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/943633003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:206: iter.second.Run();
I think we want to keep the bool here. We would want to differentiate success vs
failure.

[Tobias Sargeant, 2015-02-26 17:09:28]

On 2015/02/24 21:51:23, piman (Very slow to review) wrote:
 
>
https://codereview.chromium.org/943633003/diff/40001/content/browser/frame_ho...
> content/browser/frame_host/render_frame_host_impl.cc:206: iter.second.Run();
> I think we want to keep the bool here. We would want to differentiate success
vs
> failure.

I started looking at what would be required to report the bool correctly in the
case where a swap failed (as discussed in crbug/459539) and pretty quickly hit a
problem. The response object is constructed as soon as the request is received,
and at that point it's not known whether the swap will fail. By the time that
the messages are requeued for delivery in FrameSwapMessageQueue::Did(Not)Swap,
we are dealing with IPC::Message pointers, so it seems like it's not possible to
insert the value of the boolean at the point where we finally know.

I can think of a couple of ways around this, but none of them feel particularly
clean.

In light of this, would you prefer to drop the bool, or find a way to deliver it
accurately?

[piman, 2015-02-27 22:16:26]

On Thu, Feb 26, 2015 at 9:09 AM, <tobiasjs@chromium.org> wrote:

> On 2015/02/24 21:51:23, piman (Very slow to review) wrote:
>
>
> https://codereview.chromium.org/943633003/diff/40001/
> content/browser/frame_host/render_frame_host_impl.cc#newcode206
>
>> content/browser/frame_host/render_frame_host_impl.cc:206:
>> iter.second.Run();
>> I think we want to keep the bool here. We would want to differentiate
>> success
>>
> vs
>
>> failure.
>>
>
> I started looking at what would be required to report the bool correctly
> in the
> case where a swap failed (as discussed in crbug/459539) and pretty quickly
> hit a
> problem. The response object is constructed as soon as the request is
> received,
> and at that point it's not known whether the swap will fail. By the time
> that
> the messages are requeued for delivery in FrameSwapMessageQueue::Did(
> Not)Swap,
> we are dealing with IPC::Message pointers, so it seems like it's not
> possible to
> insert the value of the boolean at the point where we finally know.
>
> I can think of a couple of ways around this, but none of them feel
> particularly
> clean.
>
> In light of this, would you prefer to drop the bool, or find a way to
> deliver it
> accurately?
>

It seems from the bug that the only case we want to return false is on tear
down (via COMMIT_FAILS). When that happens, we'll eventually tear down the
frame on the host side and send the callback here (with false).
So all we need to do is to not send the IPC if COMMIT_FAILS (i.e. it would
be equivalent to the renderer crashing). You can probably do that by
changing FrameSwapMessageQueue::DidNotSwap, so that instead of returning
the  visual_state_queue_ messages on COMMIT_FAILS, they'd get deleted.


>
> https://codereview.chromium.org/943633003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.