Give the RequestFetcher access to cross-origin URLs when allowed

Give the RequestFetcher access to cross-origin URLs when allowed

Previously even if the URL loader was configured to have cross origin access
this bit was not sent through to the RequestFetcher so that access could be
granted. This CL adds that information to the FetchRequest so that
RequestFetcher::canRequest gives the correct result in these cases.

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

[raymes, 2014-06-20 04:59:38]

Starting with japhet@ - please point me to the right place! Thanks!

[Nate Chapin, 2014-06-20 15:17:57]

LGTM. Test?

[raymes, 2014-06-23 03:35:03]

Can you give me a suggestion as to where to add a test? Ideally we could just
unit-test this change...but I'm guessing it's not easy to do.

Thanks!

[Nate Chapin, 2014-06-23 17:39:32]

On 2014/06/23 03:35:03, raymes wrote:
> Can you give me a suggestion as to where to add a test? Ideally we could just
> unit-test this change...but I'm guessing it's not easy to do.
> 
> Thanks!

I'd guess this could be done via Source/web/tests/WebFrameTest using
WebFrame::createAssociatedURLLoader?

[raymes, 2014-06-24 05:18:57]

On 2014/06/23 17:39:32, Nate Chapin wrote:
> On 2014/06/23 03:35:03, raymes wrote:
> > Can you give me a suggestion as to where to add a test? Ideally we could
just
> > unit-test this change...but I'm guessing it's not easy to do.
> > 
> > Thanks!
> 
> I'd guess this could be done via Source/web/tests/WebFrameTest using
> WebFrame::createAssociatedURLLoader?

Using createAssociatedURLLoader was going to be harder because it only supports
async requests. It was much easier to test the DocumentThreadableLoader
directly. As such this could actually be a unittest for DocumentThreadableLoader
but I couldn't work out how to spin up enough of the frame to get it to work.

[Nate Chapin, 2014-06-24 18:00:36]

On 2014/06/24 05:18:57, raymes wrote:
> On 2014/06/23 17:39:32, Nate Chapin wrote:
> > On 2014/06/23 03:35:03, raymes wrote:
> > > Can you give me a suggestion as to where to add a test? Ideally we could
> just
> > > unit-test this change...but I'm guessing it's not easy to do.
> > > 
> > > Thanks!
> > 
> > I'd guess this could be done via Source/web/tests/WebFrameTest using
> > WebFrame::createAssociatedURLLoader?
> 
> Using createAssociatedURLLoader was going to be harder because it only
supports
> async requests. It was much easier to test the DocumentThreadableLoader
> directly. As such this could actually be a unittest for
DocumentThreadableLoader
> but I couldn't work out how to spin up enough of the frame to get it to work.

Unfortunately, we don't have good ways to create tests that can do real loads in
Source/core, so I usually dump them into Source/web/tests (WebFrameTest or
WebViewTest). Those can mock out loads in ways that look completely real to
blink, and you should be able to reach in and grab the Document to create a
ThreadableLoader.

You could also check whether Source/web/tests/AssociatedURLLoaderTest is close
enough to your purposes. I forgot that existed.

[raymes, 2014-06-25 00:26:54]

Thanks - I did upload a new patchset with the last comment (not sure
if you saw it) but it might be along the lines of what you were
suggesting :)

On Wed, Jun 25, 2014 at 4:00 AM,  <japhet@chromium.org> wrote:
> On 2014/06/24 05:18:57, raymes wrote:
>>
>> On 2014/06/23 17:39:32, Nate Chapin wrote:
>> > On 2014/06/23 03:35:03, raymes wrote:
>> > > Can you give me a suggestion as to where to add a test? Ideally we
>> > > could
>> just
>> > > unit-test this change...but I'm guessing it's not easy to do.
>> > >
>> > > Thanks!
>> >
>> > I'd guess this could be done via Source/web/tests/WebFrameTest using
>> > WebFrame::createAssociatedURLLoader?
>
>
>> Using createAssociatedURLLoader was going to be harder because it only
>
> supports
>>
>> async requests. It was much easier to test the DocumentThreadableLoader
>> directly. As such this could actually be a unittest for
>
> DocumentThreadableLoader
>>
>> but I couldn't work out how to spin up enough of the frame to get it to
>> work.
>
>
> Unfortunately, we don't have good ways to create tests that can do real
> loads in
> Source/core, so I usually dump them into Source/web/tests (WebFrameTest or
> WebViewTest). Those can mock out loads in ways that look completely real to
> blink, and you should be able to reach in and grab the Document to create a
> ThreadableLoader.
>
> You could also check whether Source/web/tests/AssociatedURLLoaderTest is
> close
> enough to your purposes. I forgot that existed.
>
> https://codereview.chromium.org/344883006/

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

[raymes, 2014-06-30 00:25:28]

ping

[Nate Chapin, 2014-07-01 02:17:27]

On 2014/06/30 00:25:28, raymes wrote:
> ping

LGTM

[raymes, 2014-07-02 04:51:37]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-07-02 04:52:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/344883006/60001

[commit-bot: I haz the power, 2014-07-02 06:00:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13887)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14984)

[commit-bot: I haz the power, 2014-07-02 06:25:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-02 06:25:37]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[raymes, 2014-07-02 06:27:31]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-07-02 06:28:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/344883006/60001

[commit-bot: I haz the power, 2014-07-02 07:36:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13897)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14984)

[commit-bot: I haz the power, 2014-07-02 08:03:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-02 08:03:01]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[raymes, 2014-07-03 07:40:54]

Hey Nate,

Could you please take a look at the patch one more time. One of the layout tests
failed. It's in web inspector. The test assumes that loading a local file should
fail from the inspector. I'm not quite sure why that is assumed to be the case?
Is there anyone who would know more about this.

Note that I verified in this case that the DocumentThreadableLoader has its
crossOriginRequestPolicy set to be AllowCrossOriginRequests. This should allow
it to load local files, correct? I don't want to get this wrong :)

Note that in Pepper it assumed that AllowCrossOriginRequests grants universal
permissions to the URL loader (which is what this change permits). This may not
be the case though.

Thanks!

[Nate Chapin, 2014-07-07 17:24:52]

LGTM

Sorry for the perpetual delays, I've been on leave and only checking reviews
sporadically.

https://codereview.chromium.org/344883006/diff/80001/Source/web/tests/WebFram...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/344883006/diff/80001/Source/web/tests/WebFram...
Source/web/tests/WebFrameTest.cpp:5634: // requires spin-up of a frame. It may
be possible to remove that requirement
FWIW, this is a nearly universal problem with anything in core/loader/ or
core/fetch/.

[raymes, 2014-07-08 00:08:43]

On 2014/07/07 17:24:52, Nate Chapin wrote:
> LGTM
> 
> Sorry for the perpetual delays, I've been on leave and only checking reviews
> sporadically.
> 
>
https://codereview.chromium.org/344883006/diff/80001/Source/web/tests/WebFram...
> File Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/344883006/diff/80001/Source/web/tests/WebFram...
> Source/web/tests/WebFrameTest.cpp:5634: // requires spin-up of a frame. It may
> be possible to remove that requirement
> FWIW, this is a nearly universal problem with anything in core/loader/ or
> core/fetch/.

No worries, thanks!

[raymes, 2014-07-08 00:09:11]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-07-08 00:09:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/344883006/80001

[commit-bot: I haz the power, 2014-07-08 01:02:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/9523)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/14519)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15781)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25650)

[commit-bot: I haz the power, 2014-07-08 01:04:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 01:04:04]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25681)

[raymes, 2014-07-14 04:01:56]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-07-14 04:02:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/344883006/100001

[commit-bot: I haz the power, 2014-07-14 05:03:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16664)

[commit-bot: I haz the power, 2014-07-14 05:36:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-14 05:36:45]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16670)

[raymes, 2014-07-14 06:52:32]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-07-14 06:52:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/344883006/100001

[commit-bot: I haz the power, 2014-07-14 07:20:49]

Change committed as 178029