Added field to error structures to signal existence of a stale copy in

Added field to error structures to signal existence of a stale copy in
the cache, for use by the navigation source.

BUG=329620
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165973

[jamesr, 2014-01-15 00:22:42]

I'm not a good reviewer for navigation

[Randy Smith (Not in Mondays), 2014-01-15 00:41:51]

On 2014/01/15 00:22:42, jamesr wrote:
> I'm not a good reviewer for navigation

Any suggestions as to who would be?  I'm not familiar with the blink ownership
space.

[tony, 2014-01-15 19:11:42]

On 2014/01/15 00:41:51, rdsmith wrote:
> On 2014/01/15 00:22:42, jamesr wrote:
> > I'm not a good reviewer for navigation
> 
> Any suggestions as to who would be?  I'm not familiar with the blink ownership
> space.

Maybe japhet or abarth?  The chrome side change is
https://codereview.chromium.org/138513002/ .

[Randy Smith (Not in Mondays), 2014-01-15 19:17:30]

On 2014/01/15 19:11:42, tony wrote:
> On 2014/01/15 00:41:51, rdsmith wrote:
> > On 2014/01/15 00:22:42, jamesr wrote:
> > > I'm not a good reviewer for navigation
> > 
> > Any suggestions as to who would be?  I'm not familiar with the blink
ownership
> > space.
> 
> Maybe japhet or abarth?  The chrome side change is
> https://codereview.chromium.org/138513002/ .

Thanks!  Requested on the chromium side.

[Nate Chapin, 2014-01-15 19:24:08]

On 2014/01/15 19:17:30, rdsmith wrote:
> On 2014/01/15 19:11:42, tony wrote:
> > On 2014/01/15 00:41:51, rdsmith wrote:
> > > On 2014/01/15 00:22:42, jamesr wrote:
> > > > I'm not a good reviewer for navigation
> > > 
> > > Any suggestions as to who would be?  I'm not familiar with the blink
> ownership
> > > space.
> > 
> > Maybe japhet or abarth?  The chrome side change is
> > https://codereview.chromium.org/138513002/ .
> 
> Thanks!  Requested on the chromium side.

My gut reaction is that this state should be indicated in as an error code in
"int reason;", rather than as a separate bit. What are the merits of doing it
this way?

[Randy Smith (Not in Mondays), 2014-01-15 19:28:16]

On 2014/01/15 19:24:08, Nate Chapin wrote:
> On 2014/01/15 19:17:30, rdsmith wrote:
> > On 2014/01/15 19:11:42, tony wrote:
> > > On 2014/01/15 00:41:51, rdsmith wrote:
> > > > On 2014/01/15 00:22:42, jamesr wrote:
> > > > > I'm not a good reviewer for navigation
> > > > 
> > > > Any suggestions as to who would be?  I'm not familiar with the blink
> > ownership
> > > > space.
> > > 
> > > Maybe japhet or abarth?  The chrome side change is
> > > https://codereview.chromium.org/138513002/ .
> > 
> > Thanks!  Requested on the chromium side.
> 
> My gut reaction is that this state should be indicated in as an error code in
> "int reason;", rather than as a separate bit. What are the merits of doing it
> this way?

Because, at least in the chromium embedder, the state is orthogonal to the error
code.  Whether or not we have information in the cache is separate from why the
request failed--whether or not we have state in the cache is purely extra
information.  It doesn't even really indicate that an error's happened.

[Nate Chapin, 2014-01-15 19:41:10]

On 2014/01/15 19:28:16, rdsmith wrote:
> On 2014/01/15 19:24:08, Nate Chapin wrote:
> > On 2014/01/15 19:17:30, rdsmith wrote:
> > > On 2014/01/15 19:11:42, tony wrote:
> > > > On 2014/01/15 00:41:51, rdsmith wrote:
> > > > > On 2014/01/15 00:22:42, jamesr wrote:
> > > > > > I'm not a good reviewer for navigation
> > > > > 
> > > > > Any suggestions as to who would be?  I'm not familiar with the blink
> > > ownership
> > > > > space.
> > > > 
> > > > Maybe japhet or abarth?  The chrome side change is
> > > > https://codereview.chromium.org/138513002/ .
> > > 
> > > Thanks!  Requested on the chromium side.
> > 
> > My gut reaction is that this state should be indicated in as an error code
in
> > "int reason;", rather than as a separate bit. What are the merits of doing
it
> > this way?
> 
> Because, at least in the chromium embedder, the state is orthogonal to the
error
> code.  Whether or not we have information in the cache is separate from why
the
> request failed--whether or not we have state in the cache is purely extra
> information.  It doesn't even really indicate that an error's happened.

Ah, ok. But the state is on WebURLError because it only matters to Blink if an
error ends up happening.

In that case, this seems reasonable to me, but I'll defer to abarth, since I'm
not in public/OWNERS.

[Randy Smith (Not in Mondays), 2014-01-15 19:48:01]

On 2014/01/15 19:41:10, Nate Chapin wrote:
> On 2014/01/15 19:28:16, rdsmith wrote:
> > On 2014/01/15 19:24:08, Nate Chapin wrote:
> > > On 2014/01/15 19:17:30, rdsmith wrote:
> > > > On 2014/01/15 19:11:42, tony wrote:
> > > > > On 2014/01/15 00:41:51, rdsmith wrote:
> > > > > > On 2014/01/15 00:22:42, jamesr wrote:
> > > > > > > I'm not a good reviewer for navigation
> > > > > > 
> > > > > > Any suggestions as to who would be?  I'm not familiar with the blink
> > > > ownership
> > > > > > space.
> > > > > 
> > > > > Maybe japhet or abarth?  The chrome side change is
> > > > > https://codereview.chromium.org/138513002/ .
> > > > 
> > > > Thanks!  Requested on the chromium side.
> > > 
> > > My gut reaction is that this state should be indicated in as an error code
> in
> > > "int reason;", rather than as a separate bit. What are the merits of doing
> it
> > > this way?
> > 
> > Because, at least in the chromium embedder, the state is orthogonal to the
> error
> > code.  Whether or not we have information in the cache is separate from why
> the
> > request failed--whether or not we have state in the cache is purely extra
> > information.  It doesn't even really indicate that an error's happened.
> 
> Ah, ok. But the state is on WebURLError because it only matters to Blink if an
> error ends up happening.

Exactly.  In this case, blink is just acting as a conduit for the error; it gets
passed in by the embedder on one side, and used by the embedder on the other. 
But yeah, it's in the error struct because it only matters if an error happens.

> In that case, this seems reasonable to me, but I'll defer to abarth, since I'm
> not in public/OWNERS.

Good enough for me for now--I just wanted a sanity check from a knowledgeable
blinker (sic?).

[abarth-chromium, 2014-01-26 02:05:41]

It seems suboptimal for Blink to need to know about this state.  Can we not
store this information in the ExtraData associated with the response?  That way
the concern can be handled entirely at the Chromium layer.

[Randy Smith (Not in Mondays), 2014-01-26 23:02:59]

On 2014/01/26 02:05:41, abarth wrote:
> It seems suboptimal for Blink to need to know about this state.  Can we not
> store this information in the ExtraData associated with the response?  That
way
> the concern can be handled entirely at the Chromium layer.

[Adam and I chatted offline about this plan, and the context it comes from that
blink has an ExtraData design pattern where classes often have contained classes
that consumers can use to provide data blink doesn't need to know about.  I said
I'd go off and understand the situation and report back with a proposed plan.  ]

So first the simple evaluation: It doesn't look to me like WebURLResponse (or
it's ExtraData subclass) is passed in the error case.  The chromium version of
WebURLResponse::ExtraData (WebURLResponseExtraDataImpl) is created by
WebURLLoaderImpl::PopulateURLResponse.  That's called from a couple of different
places, but the key one appears to be
WebURLLoaderImpl::Context::OnReceivedResponse, which is indirectly called when
the renderer receives a ResourceMsg_ReceivedResponse message.  That message
isn't sent if the URLRequest fails at the beginning; it is sent when the
response headers are seen.  Matching this,
RenderFrameImpl::didFailProvisionalLoad (the location in chromium I'm trying to
get this information to) doesn't get a WebURLResponse; its arguments are a
WebFrame and a WebURLError.

So I believe to use this pattern to solve my problem, I'll need to add an
WebURLError::ExtraData class.  This is made somewhat more complicated by the
fact that WebURLError is expected to be copied around (and transformed to/from a
ResourceError).  Following the pattern in WebURLResponse (with a bit of a
simplification; it uses a private indirection), this means that I'll need to
create a second class which wraps WebURLError::ExtraData but is reference
counted, so if copies are made of WebURLError, the ExtraData class won't be
copied (it doesn't matter if the boolean I'm passing is copied, but there's no
way with the simple |virtual ~ExtraData() {}| interface to copy the opaque extra
data class).  I could change the WebURLResponse pattern and include a "Copy();"
method as part of the interface, but that seems gratuitously different, and
limits the type of data that can be added to WebURLError in the future.

Alternatively, I could solve this problem completely at the chromium level by
reserving a bit in WebURLError::reason for the stale cache info.  Bit that feels
very hacky to me--currently that reason is used everywhere (in chromium) as a
net::Error, and there isn't currently any functional interface for translating
between WebURLError::reason and net::Error.  The fact that the error cases are
represented by negative numbers would also make the implementation a bit tricky.
 

So I think the right thing to do for proper abstraction is to implement the
refcounted wrapper in WebURLError and ResourceError.  But I wanted to check back
in before I dove into doing that, because to my naive eye it looks like a fair
amount of work and a lot of complicating code in a simple DS.  So: Adam, given
the above, is that what you'd like me to do?

[abarth-chromium, 2014-01-27 18:05:31]

https://codereview.chromium.org/138493002/diff/1/public/platform/WebURLError.h
File public/platform/WebURLError.h (right):

https://codereview.chromium.org/138493002/diff/1/public/platform/WebURLError....
public/platform/WebURLError.h:57: bool staleCopyInCache;
How likely do you think it is that we'll want to attach additional information
to WebURLError in the future?  Meaning, if we build the complicated general
thing, how likely is it that we'll make use of its generality in the future?

[Randy Smith (Not in Mondays), 2014-01-27 19:13:56]

https://codereview.chromium.org/138493002/diff/1/public/platform/WebURLError.h
File public/platform/WebURLError.h (right):

https://codereview.chromium.org/138493002/diff/1/public/platform/WebURLError....
public/platform/WebURLError.h:57: bool staleCopyInCache;
On 2014/01/27 18:05:31, abarth wrote:
> How likely do you think it is that we'll want to attach additional information
> to WebURLError in the future?  Meaning, if we build the complicated general
> thing, how likely is it that we'll make use of its generality in the future?

Good question.  In the area that I'm currently working in (improving chrome
experience when you're offline) I can't think of anything we're likely to want. 
I could imagine wanting more nuance and detail about cache state (why is it
stale?  what are the headers?  Is it partial storage (range request) or complete
storage?) but really, given the racing with eviction we're doing, the only real
information here is a hint to the renderer that it might be worth trying a
LOAD_ONLY_FROM_CACHE request, and I don't think the extra information would
really help the renderer make that decision (or if it did, we can subtly tweak
the meaning of the boolean on the other side).  So I don't believe this work is
likely to produce more information to put in WebURLError.  I'm certainly happy
to sign up for the complicated version in the future if I end up wanting to put
more stuff in.

Expanding beyond the offline situation, the answer I come up is "maybe".  The
general question is, might there be expanded information about the error that
we'd want to communicate with the renderer, and I could imagine there being such
(network status (local bandwidth, captive portal, radio vs. wifi, flakiness
measurements, etc.)  But the further question is, will that information be
synchronous with the request failure, or is it better sent through other
messages that don't involve blink.  My current guess (I also chatted with Matt
Menke, cc'd, who's the error page expert) is that the answer is "no", but the
future isn't certain.

So I think I'll summarize that to "not very likely", with caveats :-}.

Let me ask about a different possible implementation pathway, which could make
this question moot.  Is it possible for the blink embedder (Chromium, in this
case) to export a type that blink makes use of?  If so, we could make
WebURLError::reason be a copyable class that Chromium defines.  This strikes me
as truer to the spirit of what that value is than making it an "int", it doesn't
require a lot of complexity in blink, and not too much in Chromium either.  But
I'm not sure it's legal to have a blink->embedder dependency like that.

[Randy Smith (Not in Mondays), 2014-01-27 19:46:55]

I want to call out: I'm happy to do the work.  I'm just concerned that it's a
lot of code to add to a simple data structure in blink, and I'd be bummed if I
did the work and you said "on second thought, this is too complex; just add the
bool" :-}.  So I want to make sure it's the right thing before I dive in.

On 2014/01/27 19:13:56, rdsmith wrote:
> https://codereview.chromium.org/138493002/diff/1/public/platform/WebURLError.h
> File public/platform/WebURLError.h (right):
> 
>
https://codereview.chromium.org/138493002/diff/1/public/platform/WebURLError....
> public/platform/WebURLError.h:57: bool staleCopyInCache;
> On 2014/01/27 18:05:31, abarth wrote:
> > How likely do you think it is that we'll want to attach additional
information
> > to WebURLError in the future?  Meaning, if we build the complicated general
> > thing, how likely is it that we'll make use of its generality in the future?
> 
> Good question.  In the area that I'm currently working in (improving chrome
> experience when you're offline) I can't think of anything we're likely to
want. 
> I could imagine wanting more nuance and detail about cache state (why is it
> stale?  what are the headers?  Is it partial storage (range request) or
complete
> storage?) but really, given the racing with eviction we're doing, the only
real
> information here is a hint to the renderer that it might be worth trying a
> LOAD_ONLY_FROM_CACHE request, and I don't think the extra information would
> really help the renderer make that decision (or if it did, we can subtly tweak
> the meaning of the boolean on the other side).  So I don't believe this work
is
> likely to produce more information to put in WebURLError.  I'm certainly happy
> to sign up for the complicated version in the future if I end up wanting to
put
> more stuff in.
> 
> Expanding beyond the offline situation, the answer I come up is "maybe".  The
> general question is, might there be expanded information about the error that
> we'd want to communicate with the renderer, and I could imagine there being
such
> (network status (local bandwidth, captive portal, radio vs. wifi, flakiness
> measurements, etc.)  But the further question is, will that information be
> synchronous with the request failure, or is it better sent through other
> messages that don't involve blink.  My current guess (I also chatted with Matt
> Menke, cc'd, who's the error page expert) is that the answer is "no", but the
> future isn't certain.
> 
> So I think I'll summarize that to "not very likely", with caveats :-}.
> 
> Let me ask about a different possible implementation pathway, which could make
> this question moot.  Is it possible for the blink embedder (Chromium, in this
> case) to export a type that blink makes use of?  If so, we could make
> WebURLError::reason be a copyable class that Chromium defines.  This strikes
me
> as truer to the spirit of what that value is than making it an "int", it
doesn't
> require a lot of complexity in blink, and not too much in Chromium either. 
But
> I'm not sure it's legal to have a blink->embedder dependency like that.

[abarth-chromium, 2014-01-28 18:41:33]

LGTM

Sorry for sending you on a wild goose chase w.r.t. adding an exra data
container.  If we add many more of these, we might want to reconsider having a
container for them.  Thanks for investigating alternative approaches.

[commit-bot: I haz the power, 2014-01-28 18:44:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/138493002/1

[commit-bot: I haz the power, 2014-01-28 23:02:37]

Change committed as 165973

[commit-bot: I haz the power, 2014-01-28 23:03:46]

CQ bit was unchecked on CL. Ignoring.