predictors: Add browsertests that check fetching through javascript.

predictors: Add browsertests that check fetching through javascript.

Make sure that ResourcePrefetchPredictor awares about javascript initiated
requests + test for iframe.

BUG=650253
Committed: https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8
Cr-Commit-Position: refs/heads/master@{#436566}

[alexilin, 2016-11-29 19:30:56]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org, pasko@chromium.org

[alexilin, 2016-11-29 19:30:56]

Hi,

PTAL at another bunch of browser tests for ResourcePrefetchPredictor.

[pasko, 2016-11-30 13:52:48]

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:29: static
const char kStylePath2[] = "/predictors/style2.css";
it would be nice (i.e. more readable with less surprises) to rename these to be
like: "/handled-by-test/style2.css" and explain in the comment that they return
empty responses with controllable response headers.

Then kHtmlSubresourcesPath (plus html_{script,iframe} would be defined
separately, as it is loaded from a file by the test server.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:51:
ResourceSummary* WithContent(const std::string& i_content) {
we do not make prefixed names in chromium codebase, so it should be just
"content"

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:58: return
this;
while I appreciate the added elegance, returning |this| like builders commonly
do in Java, I did not observe this as a common pattern in chromium code, and
also was advised against it on codereviews (cannot remember the arguments they
used - perhaps consistency?).

Also using |this| approaches the boundary of trivial and suggests using a class
instead of a struct.

Can we "just" have one method to register a list of not-recorded
URLs/paths/files in the fixture? Call it once per test. This would save lines of
code as well.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:172: if
(kv.second.is_no_store || !kv.second.should_be_recorded)
what does "should_be_recorded" mean? Can we EXPECT that after all requests are
being made this resource is not in the database? If that would make a reliable
test, let's do it, more easy checks are better.

Also please explain what this field is responsible for in the struct definition.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:396:
"document.write(\'<img src=\"image.png\">\');"
Code in one language containing code in another language containing code in yet
another language, mixed with escapes, is unnecessarily hard to read.

There is even a name for it: https://puroh.it/turducken-code/

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:397:
"document.write(\'<link rel=\"stylesheet\" href=\"style.css\">\');"
there are other ways for JS to request a resource: innerHtml, appendChild.

Can we try them as well to be extra confident that we are not missing these
cases?

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:398:
"document.write(\'<script src=\"script2.js\"></script>\');");
creating a file for this would be preferable. Is there any flexibility required
here that the test server loading from a file does not provide? If it is for
setting priorities from the test, I would argue that it is a mis-feature of an
integration test, covering the code setting the priorities in Blink would be
good here (unless it is flaky - which we could workaround in the test with more
complicated expectations).

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:407: //
because they have unsupported resource type content::RESOURCE_TYPE_XHR.
what's our status on fixing it? is there a bug? If there is, please reference
the bug in the comment.

[alexilin, 2016-11-30 18:43:50]

Manipulations with innerHTML behave interesting. I didn't investigate it yet.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:29: static
const char kStylePath2[] = "/predictors/style2.css";
On 2016/11/30 13:52:48, pasko wrote:
> it would be nice (i.e. more readable with less surprises) to rename these to
be
> like: "/handled-by-test/style2.css" and explain in the comment that they
return
> empty responses with controllable response headers.
> 
> Then kHtmlSubresourcesPath (plus html_{script,iframe} would be defined
> separately, as it is loaded from a file by the test server.

Done.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:51:
ResourceSummary* WithContent(const std::string& i_content) {
On 2016/11/30 13:52:47, pasko wrote:
> we do not make prefixed names in chromium codebase, so it should be just
> "content"

Not quite true:
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...
This is an attempt to avoid the collision between parameter name and field name.
Anyway, I deleted these setters at all.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:58: return
this;
On 2016/11/30 13:52:48, pasko wrote:
> while I appreciate the added elegance, returning |this| like builders commonly
> do in Java, I did not observe this as a common pattern in chromium code, and
> also was advised against it on codereviews (cannot remember the arguments they
> used - perhaps consistency?).
> 
> Also using |this| approaches the boundary of trivial and suggests using a
class
> instead of a struct.
> 
> Can we "just" have one method to register a list of not-recorded
> URLs/paths/files in the fixture? Call it once per test. This would save lines
of
> code as well.

Done.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:172: if
(kv.second.is_no_store || !kv.second.should_be_recorded)
On 2016/11/30 13:52:47, pasko wrote:
> what does "should_be_recorded" mean? Can we EXPECT that after all requests are
> being made this resource is not in the database? If that would make a reliable
> test, let's do it, more easy checks are better.
> 
> Also please explain what this field is responsible for in the struct
definition.

It means that this resource should or should not be recorded in
ResourcePrefetchPredictor database. But we still want to declare this resource
in test to not return 404 response code. There is no need of explicitly checking
the absence of these resources in database because we already have the check in
ResourcePrefetchPredictorTestObserver.
There is possibly confusion because I have a single list of resources for
serving in request handler and for checking expectations in TestObserver.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:396:
"document.write(\'<img src=\"image.png\">\');"
On 2016/11/30 13:52:48, pasko wrote:
> Code in one language containing code in another language containing code in
yet
> another language, mixed with escapes, is unnecessarily hard to read.
> 
> There is even a name for it: https://puroh.it/turducken-code/

Acknowledged.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:397:
"document.write(\'<link rel=\"stylesheet\" href=\"style.css\">\');"
On 2016/11/30 13:52:48, pasko wrote:
> there are other ways for JS to request a resource: innerHtml, appendChild.
> 
> Can we try them as well to be extra confident that we are not missing these
> cases?

I'm not sure that it's secure to perform DOM manipulations before onload event.
Is it guaranteed that all elements before script are ready before script
execution?

We can see interesting things about tests with appendChild and innerHTML.
Script requested by appendChild has lower priority and adding script tag by
innerHTML doesn't cause fetching of script at all!

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:398:
"document.write(\'<script src=\"script2.js\"></script>\');");
On 2016/11/30 13:52:48, pasko wrote:
> creating a file for this would be preferable. Is there any flexibility
required
> here that the test server loading from a file does not provide? If it is for
> setting priorities from the test, I would argue that it is a mis-feature of an
> integration test, covering the code setting the priorities in Blink would be
> good here (unless it is flaky - which we could workaround in the test with
more
> complicated expectations).
> 

Done.

https://codereview.chromium.org/2540803002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:407: //
because they have unsupported resource type content::RESOURCE_TYPE_XHR.
On 2016/11/30 13:52:47, pasko wrote:
> what's our status on fixing it? is there a bug? If there is, please reference
> the bug in the comment.

Well, I just discovered it by writing this test so it's new.
I don't know how often xhr requests are used to load scripts and images. I
suspect that it is rather used for dynamically load some json or xml data than
something from {css,script,image,font}.

Some data from HTTP archive. I filtered all requests with "X-Requested-With:
XMLHttpRequest" header and grouped by "type" column. It's a relatively small
percent of all requests (40M).
type    count
total   232939
script	113159	 
html	97751	 
text	7521	 
xml	6133	 
other	4485	 
image	3431	 
css	251	 
audio	151	 
font	35	 
video	22	 
I'm not sure how accurate this "type" column and what percent of xhr requests
actually set this header. Also notice that "script" type isn't javascript only.
It encloses files with various types (php, html etc.)

[alexilin, 2016-11-30 19:48:39]

I've found following reference in W3C html5 Working Draft: "script elements
inserted using innerHTML do not execute when they are inserted." 
https://www.w3.org/TR/2008/WD-html5-20080610/dom.html

However, it's the draft dated 2008 and I can't find this instructions in newer
drafts.

There is also relevant layout test that should pass:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/...

[alexilin, 2016-11-30 19:54:16]

Wow, it seems like the test
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/...
works only for xhtml and doesn't work if we have plain html.
However I don't think that it's our problem at all. Do not load script after
innerHTML change is expected browser behavior.

[alexilin, 2016-12-01 09:23:35]

My previous comment was wrong.
LayoutTest
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/...
works fine for html file extension if delete CDATA from page.
However this test is differ because it modifies innerHTML of existing script tag
(inserted on previous step) and definition of script tag is inside innerHTML
parameter in my test.

[pasko, 2016-12-01 15:38:01]

ok, innerHTML is good to have, but let's *not* block this change behind it. Feel
free to remove it, leave a TODO and ask to take another look.

As long as it works, I would be happy to approve this, with innerHTML or
without.

Please launch the git cl try.

[alexilin, 2016-12-01 15:43:26]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 15:43:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 15:49:33]

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

[commit-bot: I haz the power, 2016-12-01 15:49:34]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alexilin, 2016-12-01 18:22:10]

I've changed some comments in code. Now I'm pretty sure that innerHTML behaves
correctly so I'll keep it in this CL.
What to do with presubmit errors? I don't think that we have to add copyright to
these .js files. Other test scripts don't have it.

[pasko, 2016-12-02 18:27:04]

On 2016/12/01 18:22:10, alexilin wrote:
> I've changed some comments in code. Now I'm pretty sure that innerHTML behaves
> correctly so I'll keep it in this CL.
> What to do with presubmit errors? I don't think that we have to add copyright
to
> these .js files. Other test scripts don't have it.

just add the copyright headers. Plenty of other .js files have it. These ones
could have landed before the presubmit check existed, or were pushed with
NOPRESUBMIT=true. You can fix the old test/data files, if you want, for
consistency, but I would not insist on that.

handy command:

git ls-files -- '*.js' | xargs head -3 | less

[alexilin, 2016-12-05 08:11:40]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[alexilin, 2016-12-05 08:12:01]

Still waiting for your LGTMs :)

[commit-bot: I haz the power, 2016-12-05 08:12:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 09:13:36]

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

[commit-bot: I haz the power, 2016-12-05 09:13:37]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-12-05 14:03:31]

lgtm with a few minor questions and comments.

https://codereview.chromium.org/2540803002/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2540803002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:450: // The
script isn't requested. Need furhter investigation.
nit: further.

https://codereview.chromium.org/2540803002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:456: //
because they have unsupported resource type content::RESOURCE_TYPE_XHR.
Oh, that's a good find!
I wonder what we should do about that.

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:28: static
const char kImagePath[] = "/handled-by-test/image.png";
nit: Why static here? (and below).

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:420:
externalScript->request.mime_type = "application/javascript";
nit: Define kJavascriptMime?

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:456: //
Requests originated by XMLHttpRequest have content::RESOURCE_TYPE_XHR.
This is no longer true, right?

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:467: //
load completes. Find the way to delay loading until fetch completes.
This can be done in Javascript.
Sync XHR?
https://developer.mozilla.org/fr/docs/Web/API/XMLHttpRequest/Synchronous_and_...

[alexilin, 2016-12-06 09:10:44]

I've switched xhr test from fetch to sync XHR + added mime types to subresources
to make them considerable for ResourcePrefetchPredictor.

https://codereview.chromium.org/2540803002/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2540803002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:450: // The
script isn't requested. Need furhter investigation.
On 2016/12/05 14:03:30, Benoit L wrote:
> nit: further.

Done.

https://codereview.chromium.org/2540803002/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:456: //
because they have unsupported resource type content::RESOURCE_TYPE_XHR.
On 2016/12/05 14:03:30, Benoit L wrote:
> Oh, that's a good find!
> I wonder what we should do about that.

Already done:
https://codereview.chromium.org/2539263004/

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:28: static
const char kImagePath[] = "/handled-by-test/image.png";
On 2016/12/05 14:03:30, Benoit L wrote:
> nit: Why static here? (and below).

Initially, to force internal linkage.
https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat...
But now I realized that anonymous namespace does the same thing so static is
really unnecessary. However, we still should prefer to use static when declaring
const locally. (As in
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...)
Done.

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:420:
externalScript->request.mime_type = "application/javascript";
On 2016/12/05 14:03:30, Benoit L wrote:
> nit: Define kJavascriptMime?

Done.

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:456: //
Requests originated by XMLHttpRequest have content::RESOURCE_TYPE_XHR.
On 2016/12/05 14:03:30, Benoit L wrote:
> This is no longer true, right?

Why not? Notice that this comment has been changed since previous diff.
However I think it would be better to change this test so that resources have
mime-type.

https://codereview.chromium.org/2540803002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:467: //
load completes. Find the way to delay loading until fetch completes.
On 2016/12/05 14:03:30, Benoit L wrote:
> This can be done in Javascript.
> Sync XHR?
>
https://developer.mozilla.org/fr/docs/Web/API/XMLHttpRequest/Synchronous_and_...

Done.

[Benoit L, 2016-12-06 09:22:31]

still lgtm, thanks.

[alexilin, 2016-12-06 09:41:48]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2016-12-06 09:42:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 10:44:02]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481017308199240,
"parent_rev": "79b4bd51d23583397819618732fa8b72f3fd2f30", "commit_rev":
"ea37d817a119c3b34519b1455af49650c9a87e54"}

[commit-bot: I haz the power, 2016-12-06 10:44:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-06 10:46:10]

Description was changed from

==========
predictors: Add browsertests that check fetching through javascript.

Make sure that ResourcePrefetchPredictor awares about javascript initiated
requests + test for iframe.

BUG=650253
==========

to

==========
predictors: Add browsertests that check fetching through javascript.

Make sure that ResourcePrefetchPredictor awares about javascript initiated
requests + test for iframe.

BUG=650253

Committed: https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8
Cr-Commit-Position: refs/heads/master@{#436566}
==========

[commit-bot: I haz the power, 2016-12-06 10:46:12]

Patchset 7 (id:??) landed as
https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8
Cr-Commit-Position: refs/heads/master@{#436566}