CORS errors print error twice on preload.

CORS errors print error twice on preload.

Error messages on CORS checks in the ResourceFetcher did not
discriminate between preload scans and normal resource requests. Thus,
if there was a CORS failure and a preload for the resource was done, two
error messages would be printed to the console for the same failure.
This fix adds a check to canAccessResource in ResourceFetcher to make
sure that Blink is not in a preload scan when an error message is
printed.

BUG=464876

[jww, 2015-03-06 22:58:03]

jww@chromium.org changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-03-06 23:06:11]

On 2015/03/06 22:58:27, jww wrote:

Awesome! :)

Can you add a test for that making sure that the console log is still working
when we're not preloading and not triggering when we are?

I'm especially interested in making sure that the case of a preload request that
is later discovered by the parser is handled well. Since canAccessResource is
triggered at response time, might be worth while to test that inside http/tests
and add some delay to the response.

[Yoav Weiss, 2015-03-07 15:28:04]

On 2015/03/07 08:07:16, jww wrote:

Almost :) Can you add some delay to cors-script.php as well, just to make sure
that the case of preloaded request that then gets picked up by the parser is
also covered in a non-flaky way? (I'm afraid that in the current test the parser
can discover cors-script both before and after the resource finished
downloading, depending on the machine's speed)

Also - I think there's a general preference to move to testharness rather than
js-test, but I don't think that's critical for now. Just to keep in mind for
next time.

[jww, 2015-03-07 16:25:30]

I'll add that to cors-script.php.

I actually did write the test initially using testharness, but the
presubmit was complaining that it was a "passing test" can shouldn't have
am expected.txt file, but since what we're testing for is console errors,
we need the expected.txt. Any thoughts on how to get that to work with
testharnes?

On Sat, Mar 7, 2015, 7:28 AM null <yoav@yoav.ws> wrote:

> On 2015/03/07 08:07:16, jww wrote:
>
> Almost :) Can you add some delay to cors-script.php as well, just to make
> sure
> that the case of preloaded request that then gets picked up by the parser
> is
> also covered in a non-flaky way? (I'm afraid that in the current test the
> parser
> can discover cors-script both before and after the resource finished
> downloading, depending on the machine's speed)
>
> Also - I think there's a general preference to move to testharness rather
> than
> js-test, but I don't think that's critical for now. Just to keep in mind
> for
> next time.
>
> https://codereview.chromium.org/984213002/
>

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

[jww, 2015-03-07 18:32:48]

On 2015/03/07 16:25:30, jww wrote:
> I'll add that to cors-script.php.
> 
> I actually did write the test initially using testharness, but the
> presubmit was complaining that it was a "passing test" can shouldn't have
> am expected.txt file, but since what we're testing for is console errors,
> we need the expected.txt. Any thoughts on how to get that to work with
> testharnes?
> 
> On Sat, Mar 7, 2015, 7:28 AM null <mailto:yoav@yoav.ws> wrote:
> 
> > On 2015/03/07 08:07:16, jww wrote:
> >
> > Almost :) Can you add some delay to cors-script.php as well, just to make
> > sure
> > that the case of preloaded request that then gets picked up by the parser
> > is
> > also covered in a non-flaky way? (I'm afraid that in the current test the
> > parser
> > can discover cors-script both before and after the resource finished
> > downloading, depending on the machine's speed)
> >
> > Also - I think there's a general preference to move to testharness rather
> > than
> > js-test, but I don't think that's critical for now. Just to keep in mind
> > for
> > next time.
> >
> > https://codereview.chromium.org/984213002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

I believe I've added delay to cors-script.php as you suggested, but definitely
double check that I didn't misunderstand your request :-)

And as I mentioned before, if you have suggestions on how to check console
errors even with a "passing" testharness test, I'd love to change it back to
testharness. Thanks!

[Yoav Weiss, 2015-03-07 18:38:09]

On 2015/03/07 18:32:48, jww wrote:
> On 2015/03/07 16:25:30, jww wrote:
> > I'll add that to cors-script.php.
> > 
> > I actually did write the test initially using testharness, but the
> > presubmit was complaining that it was a "passing test" can shouldn't have
> > am expected.txt file, but since what we're testing for is console errors,
> > we need the expected.txt. Any thoughts on how to get that to work with
> > testharnes?
> > 
> > On Sat, Mar 7, 2015, 7:28 AM null <mailto:yoav@yoav.ws> wrote:
> > 
> > > On 2015/03/07 08:07:16, jww wrote:
> > >
> > > Almost :) Can you add some delay to cors-script.php as well, just to make
> > > sure
> > > that the case of preloaded request that then gets picked up by the parser
> > > is
> > > also covered in a non-flaky way? (I'm afraid that in the current test the
> > > parser
> > > can discover cors-script both before and after the resource finished
> > > downloading, depending on the machine's speed)
> > >
> > > Also - I think there's a general preference to move to testharness rather
> > > than
> > > js-test, but I don't think that's critical for now. Just to keep in mind
> > > for
> > > next time.
> > >
> > > https://codereview.chromium.org/984213002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> I believe I've added delay to cors-script.php as you suggested, but definitely
> double check that I didn't misunderstand your request :-)

Awesome! LGTM :)

> 
> And as I mentioned before, if you have suggestions on how to check console
> errors even with a "passing" testharness test, I'd love to change it back to
> testharness. Thanks!

Yeah, I don't know how to do that. Might be worth an email to blink-dev, where
someone might have a better answer than *shrug* :D

[jww, 2015-03-07 18:40:36]

Cool, will do. If I get a good answer there I'll rewrite the test. Thanks!
--Joel

On Sat, Mar 7, 2015 at 10:38 AM <yoav@yoav.ws> wrote:

> On 2015/03/07 18:32:48, jww wrote:
> > On 2015/03/07 16:25:30, jww wrote:
> > > I'll add that to cors-script.php.
> > >
> > > I actually did write the test initially using testharness, but the
> > > presubmit was complaining that it was a "passing test" can shouldn't
> > have
> > > am expected.txt file, but since what we're testing for is console
> > errors,
> > > we need the expected.txt. Any thoughts on how to get that to work with
> > > testharnes?
> > >
> > > On Sat, Mar 7, 2015, 7:28 AM null <mailto:yoav@yoav.ws> wrote:
> > >
> > > > On 2015/03/07 08:07:16, jww wrote:
> > > >
> > > > Almost :) Can you add some delay to cors-script.php as well, just to
> > make
> > > > sure
> > > > that the case of preloaded request that then gets picked up by the
> > parser
> > > > is
> > > > also covered in a non-flaky way? (I'm afraid that in the current test
> > the
> > > > parser
> > > > can discover cors-script both before and after the resource finished
> > > > downloading, depending on the machine's speed)
> > > >
> > > > Also - I think there's a general preference to move to testharness
> > rather
> > > > than
> > > > js-test, but I don't think that's critical for now. Just to keep in
> > mind
> > > > for
> > > > next time.
> > > >
> > > > https://codereview.chromium.org/984213002/
> > > >
> > >
> > > To unsubscribe from this group and stop receiving emails from it, send
> > an
> > email
> > > to mailto:blink-reviews+unsubscribe@chromium.org.
>
> > I believe I've added delay to cors-script.php as you suggested, but
> > definitely
> > double check that I didn't misunderstand your request :-)
>
> Awesome! LGTM :)
>
>
> > And as I mentioned before, if you have suggestions on how to check
> console
> > errors even with a "passing" testharness test, I'd love to change it back
> > to
> > testharness. Thanks!
>
> Yeah, I don't know how to do that. Might be worth an email to blink-dev,
> where
> someone might have a better answer than *shrug* :D
>
>
>
> https://codereview.chromium.org/984213002/
>

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

[jww, 2015-03-07 18:40:39]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-03-07 18:41:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/984213002/40001

[commit-bot: I haz the power, 2015-03-08 22:43:52]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191498