Change decidePolicyForNavigation() to takea WebDataSource::ExtraData

Change decidePolicyForNavigation() to takea WebDataSource::ExtraData
instead of a WebDataSource. It doesn't need the full WebDataSource, and
WebDataSources are difficult to mock.

This is a prerequisite for codereview.chromium.org/14212005

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197118

[abarth-chromium, 2013-04-24 22:55:56]

LGTM

https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2890:
DocumentState::FromDataSource(dataSource)->
Should we add a FromExtraData method ?

[Nate Chapin, 2013-04-24 22:58:30]

On 2013/04/24 22:55:56, abarth wrote:
> LGTM
> 
>
https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
> File content/renderer/render_view_impl.cc (left):
> 
>
https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
> content/renderer/render_view_impl.cc:2890:
> DocumentState::FromDataSource(dataSource)->
> Should we add a FromExtraData method ?

I don't know what standard chromium style is for something like this. I'm happy
to add it if this is how we do things 'round these parts.

[jamesr, 2013-04-24 22:59:33]

lgtm

[jamesr, 2013-04-24 22:59:47]

On 2013/04/24 22:58:30, Nate Chapin wrote:
> On 2013/04/24 22:55:56, abarth wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
> > File content/renderer/render_view_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
> > content/renderer/render_view_impl.cc:2890:
> > DocumentState::FromDataSource(dataSource)->
> > Should we add a FromExtraData method ?
> 
> I don't know what standard chromium style is for something like this. I'm
happy
> to add it if this is how we do things 'round these parts.

Are there any other subclasses of this ExtraData?

[commit-bot: I haz the power, 2013-04-24 23:00:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/14188060/5001

[Nate Chapin, 2013-04-24 23:04:47]

On 2013/04/24 22:59:47, jamesr wrote:
> On 2013/04/24 22:58:30, Nate Chapin wrote:
> > On 2013/04/24 22:55:56, abarth wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
> > > File content/renderer/render_view_impl.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/14188060/diff/5001/content/renderer/render_vi...
> > > content/renderer/render_view_impl.cc:2890:
> > > DocumentState::FromDataSource(dataSource)->
> > > Should we add a FromExtraData method ?
> > 
> > I don't know what standard chromium style is for something like this. I'm
> happy
> > to add it if this is how we do things 'round these parts.
> 
> Are there any other subclasses of this ExtraData?

Just TestShellExtraData, which is specific to DRT/old test shell I believe.

[commit-bot: I haz the power, 2013-04-24 23:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/14188060/16001

[commit-bot: I haz the power, 2013-04-24 23:33:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-25 17:29:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/14188060/16001

[commit-bot: I haz the power, 2013-04-25 17:40:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jamesr, 2013-04-25 18:00:56]

lgtm, but you'll need a content/public OWNERS now to check the new exports.
adding piman@

https://codereview.chromium.org/14188060/diff/9004/content/public/renderer/do...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/14188060/diff/9004/content/public/renderer/do...
content/public/renderer/document_state.h:25: public base::SupportsUserData {
line up the 'public' with the first line

[piman, 2013-04-25 22:03:40]

https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2845: WebFrame* frame,
WebDataSource::ExtraData* extraData,
Can we pass a DocumentState* here, and leave the cast to the callers? The test
would directly pass the DocumentState* since that's what they have, and the
other decidePolicyForNavigation can use DocumentState::FromDataSource as was
there before?

[Nate Chapin, 2013-04-25 22:20:27]

https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2845: WebFrame* frame,
WebDataSource::ExtraData* extraData,
On 2013/04/25 22:03:40, piman wrote:
> Can we pass a DocumentState* here, and leave the cast to the callers? The test
> would directly pass the DocumentState* since that's what they have, and the
> other decidePolicyForNavigation can use DocumentState::FromDataSource as was
> there before?

I was hoping that when I was done, there would only be one version of
decidePolicyForNavigation(), as was the case before I started.

It's fine if we're ok with having 2 variants, one which is called in production
and is just a wrapper, and one which is only ever called directly by tests.

The single function solution can't take a DocumentState*, because it's called by
blink, which doesn't know DocumentState exists.

[piman, 2013-04-25 22:24:06]

On Thu, Apr 25, 2013 at 3:20 PM, <japhet@chromium.org> wrote:

>
> https://codereview.chromium.**org/14188060/diff/44001/**
>
content/renderer/render_view_**impl.cc<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> https://codereview.chromium.**org/14188060/diff/44001/**
>
content/renderer/render_view_**impl.cc#newcode2845<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc#newcode2845>
> content/renderer/render_view_**impl.cc:2845: WebFrame* frame,
> WebDataSource::ExtraData* extraData,
> On 2013/04/25 22:03:40, piman wrote:
>
>> Can we pass a DocumentState* here, and leave the cast to the callers?
>>
> The test
>
>> would directly pass the DocumentState* since that's what they have,
>>
> and the
>
>> other decidePolicyForNavigation can use DocumentState::FromDataSource
>>
> as was
>
>> there before?
>>
>
> I was hoping that when I was done, there would only be one version of
> decidePolicyForNavigation(), as was the case before I started.
>
> It's fine if we're ok with having 2 variants, one which is called in
> production and is just a wrapper, and one which is only ever called
> directly by tests.
>
> The single function solution can't take a DocumentState*, because it's
> called by blink, which doesn't know DocumentState exists.
>

As I'm lacking context on where you started and where you're going... which
one do you want to keep?


>
>
https://codereview.chromium.**org/14188060/<https://codereview.chromium.org/1...
>

[Nate Chapin, 2013-04-25 22:32:37]

On 2013/04/25 22:24:06, piman wrote:
> On Thu, Apr 25, 2013 at 3:20 PM, <mailto:japhet@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/14188060/diff/44001/**
> >
>
content/renderer/render_view_**impl.cc<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc>
> > File content/renderer/render_view_**impl.cc (right):
> >
> > https://codereview.chromium.**org/14188060/diff/44001/**
> >
>
content/renderer/render_view_**impl.cc#newcode2845<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc#newcode2845>
> > content/renderer/render_view_**impl.cc:2845: WebFrame* frame,
> > WebDataSource::ExtraData* extraData,
> > On 2013/04/25 22:03:40, piman wrote:
> >
> >> Can we pass a DocumentState* here, and leave the cast to the callers?
> >>
> > The test
> >
> >> would directly pass the DocumentState* since that's what they have,
> >>
> > and the
> >
> >> other decidePolicyForNavigation can use DocumentState::FromDataSource
> >>
> > as was
> >
> >> there before?
> >>
> >
> > I was hoping that when I was done, there would only be one version of
> > decidePolicyForNavigation(), as was the case before I started.
> >
> > It's fine if we're ok with having 2 variants, one which is called in
> > production and is just a wrapper, and one which is only ever called
> > directly by tests.
> >
> > The single function solution can't take a DocumentState*, because it's
> > called by blink, which doesn't know DocumentState exists.
> >
> 
> As I'm lacking context on where you started and where you're going... which
> one do you want to keep?

It's a long story, but basically...

Currently, on trunk, we assume frame->provisionalDataSource() is guaranteed
non-null at the point where decidePolicyForNavigation() is called. I'm planning
on breaking that assumption as part of the attempt to reign in the insanity that
is blink's FrameLoader.

I was going to resolve this by adding a WebDataSource* parameter to
decidePolicyForNavigation(). This works well for production, but it isn't great
for testing, because there is no lightweight way to create a WebDataSource*
outside of blink (the only implementation of that interface lives in
Webkit/chromium/src and therefore isn't exported).

My thought had therefore been to take a WebDataSource::ExtraData*, the parent
class of DocumentState. It's declared in blink, so it can be passed in from
blink, and it's easily constructable for tests, since the main implementation
lives on content/.

Sorry for not providing more context initially. Does that make at least some
sense? :)

> 
> 
> >
> >
>
https://codereview.chromium.**org/14188060/%3Chttps://codereview.chromium.org...>
> >

[piman, 2013-04-25 22:39:27]

On Thu, Apr 25, 2013 at 3:32 PM, <japhet@chromium.org> wrote:

> On 2013/04/25 22:24:06, piman wrote:
>
>> On Thu, Apr 25, 2013 at 3:20 PM, <mailto:japhet@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.****org/14188060/diff/44001/**
>> >
>>
>
> content/renderer/render_view_****impl.cc<https://codereview.**
>
chromium.org/14188060/diff/**44001/content/renderer/render_**view_impl.cc<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc>
> >
>
>> > File content/renderer/render_view_****impl.cc (right):
>> >
>> > https://codereview.chromium.****org/14188060/diff/44001/**
>> >
>>
>
> content/renderer/render_view_****impl.cc#newcode2845<https://**
> codereview.chromium.org/**14188060/diff/44001/content/**
>
renderer/render_view_impl.cc#**newcode2845<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc#newcode2845>
> >
>
>> > content/renderer/render_view_****impl.cc:2845: WebFrame* frame,
>>
>> > WebDataSource::ExtraData* extraData,
>> > On 2013/04/25 22:03:40, piman wrote:
>> >
>> >> Can we pass a DocumentState* here, and leave the cast to the callers?
>> >>
>> > The test
>> >
>> >> would directly pass the DocumentState* since that's what they have,
>> >>
>> > and the
>> >
>> >> other decidePolicyForNavigation can use DocumentState::FromDataSource
>> >>
>> > as was
>> >
>> >> there before?
>> >>
>> >
>> > I was hoping that when I was done, there would only be one version of
>> > decidePolicyForNavigation(), as was the case before I started.
>> >
>> > It's fine if we're ok with having 2 variants, one which is called in
>> > production and is just a wrapper, and one which is only ever called
>> > directly by tests.
>> >
>> > The single function solution can't take a DocumentState*, because it's
>> > called by blink, which doesn't know DocumentState exists.
>> >
>>
>
>  As I'm lacking context on where you started and where you're going...
>> which
>> one do you want to keep?
>>
>
> It's a long story, but basically...
>
> Currently, on trunk, we assume frame->provisionalDataSource() is guaranteed
> non-null at the point where decidePolicyForNavigation() is called. I'm
> planning
> on breaking that assumption as part of the attempt to reign in the
> insanity that
> is blink's FrameLoader.
>
> I was going to resolve this by adding a WebDataSource* parameter to
> decidePolicyForNavigation(). This works well for production, but it isn't
> great
> for testing, because there is no lightweight way to create a WebDataSource*
> outside of blink (the only implementation of that interface lives in
> Webkit/chromium/src and therefore isn't exported).
>
> My thought had therefore been to take a WebDataSource::ExtraData*, the
> parent
> class of DocumentState. It's declared in blink, so it can be passed in from
> blink, and it's easily constructable for tests, since the main
> implementation
> lives on content/.
>
> Sorry for not providing more context initially. Does that make at least
> some
> sense? :)
>

I see - basically you want to keep the one that passes the
WebDataSource::ExtraData* extraData, and you're trying to remove the one
that doesn't, but you have to leave it for now because of a 3-way patch.
So the static_cast is unavoidable, and we just cross our fingers that
the WebDataSource::ExtraData is always going to be a DocumentState. Can we
add a comment on the method to make that clear (i.e. it's not up to the
caller to decide what type it is)?
After that, LGTM for lack of better option. Maybe in some distant future we
can share more typing information between blink and chromium and we can
clean that up.

>
>
>
>  >
>> >
>>
>
> https://codereview.chromium.****org/14188060/%3Chttps://codere**
> view.chromium.org/14188060/ <http://codereview.chromium.org/14188060/>>
>
>> >
>>
>
>
>
>
https://codereview.chromium.**org/14188060/<https://codereview.chromium.org/1...
>

[Nate Chapin, 2013-04-25 22:47:13]

On 2013/04/25 22:39:27, piman wrote:
> On Thu, Apr 25, 2013 at 3:32 PM, <mailto:japhet@chromium.org> wrote:
> 
> > On 2013/04/25 22:24:06, piman wrote:
> >
> >> On Thu, Apr 25, 2013 at 3:20 PM, <mailto:japhet@chromium.org> wrote:
> >>
> >
> >  >
> >> > https://codereview.chromium.****org/14188060/diff/44001/**
> >> >
> >>
> >
> > content/renderer/render_view_****impl.cc<https://codereview.**
> >
>
chromium.org/14188060/diff/**44001/content/renderer/render_**view_impl.cc<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc>
> > >
> >
> >> > File content/renderer/render_view_****impl.cc (right):
> >> >
> >> > https://codereview.chromium.****org/14188060/diff/44001/**
> >> >
> >>
> >
> > content/renderer/render_view_****impl.cc#newcode2845<https://**
> > codereview.chromium.org/**14188060/diff/44001/content/**
> >
>
renderer/render_view_impl.cc#**newcode2845<https://codereview.chromium.org/14188060/diff/44001/content/renderer/render_view_impl.cc#newcode2845>
> > >
> >
> >> > content/renderer/render_view_****impl.cc:2845: WebFrame* frame,
> >>
> >> > WebDataSource::ExtraData* extraData,
> >> > On 2013/04/25 22:03:40, piman wrote:
> >> >
> >> >> Can we pass a DocumentState* here, and leave the cast to the callers?
> >> >>
> >> > The test
> >> >
> >> >> would directly pass the DocumentState* since that's what they have,
> >> >>
> >> > and the
> >> >
> >> >> other decidePolicyForNavigation can use DocumentState::FromDataSource
> >> >>
> >> > as was
> >> >
> >> >> there before?
> >> >>
> >> >
> >> > I was hoping that when I was done, there would only be one version of
> >> > decidePolicyForNavigation(), as was the case before I started.
> >> >
> >> > It's fine if we're ok with having 2 variants, one which is called in
> >> > production and is just a wrapper, and one which is only ever called
> >> > directly by tests.
> >> >
> >> > The single function solution can't take a DocumentState*, because it's
> >> > called by blink, which doesn't know DocumentState exists.
> >> >
> >>
> >
> >  As I'm lacking context on where you started and where you're going...
> >> which
> >> one do you want to keep?
> >>
> >
> > It's a long story, but basically...
> >
> > Currently, on trunk, we assume frame->provisionalDataSource() is guaranteed
> > non-null at the point where decidePolicyForNavigation() is called. I'm
> > planning
> > on breaking that assumption as part of the attempt to reign in the
> > insanity that
> > is blink's FrameLoader.
> >
> > I was going to resolve this by adding a WebDataSource* parameter to
> > decidePolicyForNavigation(). This works well for production, but it isn't
> > great
> > for testing, because there is no lightweight way to create a WebDataSource*
> > outside of blink (the only implementation of that interface lives in
> > Webkit/chromium/src and therefore isn't exported).
> >
> > My thought had therefore been to take a WebDataSource::ExtraData*, the
> > parent
> > class of DocumentState. It's declared in blink, so it can be passed in from
> > blink, and it's easily constructable for tests, since the main
> > implementation
> > lives on content/.
> >
> > Sorry for not providing more context initially. Does that make at least
> > some
> > sense? :)
> >
> 
> I see - basically you want to keep the one that passes the
> WebDataSource::ExtraData* extraData, and you're trying to remove the one
> that doesn't, but you have to leave it for now because of a 3-way patch.
> So the static_cast is unavoidable, and we just cross our fingers that
> the WebDataSource::ExtraData is always going to be a DocumentState. Can we
> add a comment on the method to make that clear (i.e. it's not up to the
> caller to decide what type it is)?
> After that, LGTM for lack of better option. Maybe in some distant future we
> can share more typing information between blink and chromium and we can
> clean that up.

Yeah, it's definitely not perfect.

For what it's worth, the only other implementers of ExtraData live in DRT and
webkit/tools/test_shell, and shouldn't reach here. The static_cast isn't
actually new, it's just not hidden inside DocumentState::FromDataSource().

Will re-upload with comment added momentarily.

> 
> >
> >
> >
> >  >
> >> >
> >>
> >
> > https://codereview.chromium.****org/14188060/%253Chttps://codere**
> > view.chromium.org/14188060/ <http://codereview.chromium.org/14188060/>>
> >
> >> >
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/14188060/%3Chttps://codereview.chromium.org...>
> >

[commit-bot: I haz the power, 2013-04-29 13:56:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/14188060/57001

[commit-bot: I haz the power, 2013-04-29 15:07:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-29 17:26:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/14188060/77001

[commit-bot: I haz the power, 2013-04-29 20:05:35]

Change committed as 197118