Tests comparing original-vs-saved multi-frame pages.

Tests comparing original-vs-saved for multi-frame pages.

Extra tests navigate to the saved file and verify if it "looks" the same
as the original page (i.e. if it has the same number of frames, if it
has the expected substrings from all subframes, if it has no "err"
substrings).

The extra verification done by the new test catches some regressions
that older tests would not catch (i.e. without the new tests verifying
about:blank regressions is tricky, as substrings for subframes can
appear in the saved file but end up not being rendered because of
incorrect cross-referencing).  Additionally the extra verification adds
coverage for newly discovered bugs (i.e. for handling of <object>
elements).

More details about the changes:

- The new resources got created manually from scratch:
  pdf.pdf and svg.svg using Inkscape,
  png.png using Gimp.

- iframes.htm got renamed to frames-xsite.htm
  (to be named consistently with the other 2 test files in this CL).

- local-about-blank-subframes.htm got renamed to frames-about-blank.htm
  and moved from content/test/data/download into
  chrome/test/data/save_page/

  The test using this file in mhtml_generation_browsertest.cc got
  removed - the coverage offered by one of the new tests
  (SavePageMultiFrameBrowserTest.AboutBlank_MHTML) is a superset of the
  coverage from the old test.

- Content verification markers got tweaked so the tests
  are easier to read (i.e. it is more apparent which htm test file
  the given guid comes from).

- The SavePageMultiFrameBrowserTest to some extent overlap coverage of
  existing SavePageSitePerProcessBrowserTest.  This is ok, as the old and
  new tests still verify some things unique to a specific test.

- In theory we could reuse SavePageSitePerProcessBrowserTest class.
  I've put the new tests into a separate SavePageMultiFrameBrowserTest
  class, because all new tests perform the same steps and verification
  and I plan to parametrize SavePageMultiFrameBrowserTest in the future
  by having it inherit from ::testing::WithParamInterface<SavePageType>.

BUG=553478, 539936, 538766, 526786
Committed: https://crrev.com/4771dd55cdc2ca26c955776056d540ec62da40e6
Cr-Commit-Position: refs/heads/master@{#359351}

[Łukasz Anforowicz, 2015-11-11 19:59:11]

lukasza@chromium.org changed reviewers:
+ asanka@chromium.org

[Łukasz Anforowicz, 2015-11-11 19:59:11]

Asanka, could you please take a look?

[asanka, 2015-11-11 21:37:37]

What's the entropy like for MHTML? Can we generate a reference file and have the
test compare the generated file with the reference file?

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:310: void
GetDestinationPaths(const std::string& prefix,
The directory is not used for the MHTML case, right? Shall we have a separate
function for computing the path for MHTML?

This function, as written, does the wrong thing for the
SAVE_PAGE_TYPE_AS_ONLY_HTML case.

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:875: << "Verifing if content
from 1.css is present";
Verifying

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:914: LOG(INFO) << "Verifying
test expectations for original page... : "
DLOG(INFO)

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:934: // [the original version
of the page].
Use net::FilePathToFileURL().

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:942: LOG(INFO) << "Verifying
test expectations for saved page... : "
DLOG(INFO)

[Łukasz Anforowicz, 2015-11-11 23:55:40]

Asanka, could you please take another look?

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:310: void
GetDestinationPaths(const std::string& prefix,
On 2015/11/11 21:37:37, asanka wrote:
> The directory is not used for the MHTML case, right? 

Right.

> Shall we have a separate function for computing the path for MHTML?

I'd rather keep the status quo:

1. Calculating the directory string doesn't hurt
   - it just gets ignored later on
   - see how SaveHTMLOnly test enforces that the directory *didn't* get created

2. This enables having a single function that saves a page and waits for save to
complete
   (i.e. SaveCurrentTab is shared by html-only / complete-html / mhtml tests)

> This function, as written, does the wrong thing for the
> SAVE_PAGE_TYPE_AS_ONLY_HTML case.

Does it?  SaveHTMLOnly test seems to be even assert that the directory *didn't*
get created.
(BTW: this test was not touched by my recent set of changes - I just folder
calculating of the directory path/string into a single shared function).

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:875: << "Verifing if content
from 1.css is present";
On 2015/11/11 21:37:37, asanka wrote:
> Verifying

Done.

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:914: LOG(INFO) << "Verifying
test expectations for original page... : "
On 2015/11/11 21:37:37, asanka wrote:
> DLOG(INFO)

Done.

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:934: // [the original version
of the page].
On 2015/11/11 21:37:37, asanka wrote:
> Use net::FilePathToFileURL().

Thanks!  I knew that I saw this some time ago, but couldn't find it again (and
incorrectly concluded that maybe I saw that in another project).

Done.

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:942: LOG(INFO) << "Verifying
test expectations for saved page... : "
On 2015/11/11 21:37:37, asanka wrote:
> DLOG(INFO)

Done.

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:949: void
AssertExpectationsAboutCurrentTab(
RE: top-level question about entropy in mhtml files.

> What's the entropy like for MHTML? Can we generate a reference file and have
the
> test compare the generated file with the reference file?

1. MHTML files will soon be nondeterministic - I plan to soon land a CL that
will refer to subframes via guid-based content-id.

2. I'd rather avoid whole-sale file content comparison - the essence of these
tests is that the original and saved page "look" the same.  I wouldn't want to
update 6+ reference files (+ 2 existing b.saved1.htm and b.saved2.htm in
chrome/test/data/save_page) every time some insignificant change is made to the
serialization output.

[asanka, 2015-11-12 03:14:37]

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:310: void
GetDestinationPaths(const std::string& prefix,
On 2015/11/11 at 23:55:40, Łukasz Anforowicz wrote:
> On 2015/11/11 21:37:37, asanka wrote:
> > The directory is not used for the MHTML case, right? 
> 
> Right.
> 
> > Shall we have a separate function for computing the path for MHTML?
> 
> I'd rather keep the status quo:
> 
> 1. Calculating the directory string doesn't hurt
>    - it just gets ignored later on
>    - see how SaveHTMLOnly test enforces that the directory *didn't* get
created
> 
> 2. This enables having a single function that saves a page and waits for save
to complete
>    (i.e. SaveCurrentTab is shared by html-only / complete-html / mhtml tests)
> 
> > This function, as written, does the wrong thing for the
> > SAVE_PAGE_TYPE_AS_ONLY_HTML case.
> 
> Does it?  SaveHTMLOnly test seems to be even assert that the directory
*didn't* get created.
> (BTW: this test was not touched by my recent set of changes - I just folder
calculating of the directory path/string into a single shared function).

For ONLY_HTML, it now returns an ".mht" extension.

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:949: void
AssertExpectationsAboutCurrentTab(
On 2015/11/11 at 23:55:40, Łukasz Anforowicz wrote:
> RE: top-level question about entropy in mhtml files.
> 
> > What's the entropy like for MHTML? Can we generate a reference file and have
the
> > test compare the generated file with the reference file?
> 
> 1. MHTML files will soon be nondeterministic - I plan to soon land a CL that
will refer to subframes via guid-based content-id.
> 
> 2. I'd rather avoid whole-sale file content comparison - the essence of these
tests is that the original and saved page "look" the same.  I wouldn't want to
update 6+ reference files (+ 2 existing b.saved1.htm and b.saved2.htm in
chrome/test/data/save_page) every time some insignificant change is made to the
serialization output.

Got it.

[Łukasz Anforowicz, 2015-11-12 06:30:27]

Asanka, could you please take another look?

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1412733017/diff/20001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:310: void
GetDestinationPaths(const std::string& prefix,
On 2015/11/12 03:14:36, asanka wrote:
> On 2015/11/11 at 23:55:40, Łukasz Anforowicz wrote:
> > On 2015/11/11 21:37:37, asanka wrote:
> > > The directory is not used for the MHTML case, right? 
> > 
> > Right.
> > 
> > > Shall we have a separate function for computing the path for MHTML?
> > 
> > I'd rather keep the status quo:
> > 
> > 1. Calculating the directory string doesn't hurt
> >    - it just gets ignored later on
> >    - see how SaveHTMLOnly test enforces that the directory *didn't* get
> created
> > 
> > 2. This enables having a single function that saves a page and waits for
save
> to complete
> >    (i.e. SaveCurrentTab is shared by html-only / complete-html / mhtml
tests)
> > 
> > > This function, as written, does the wrong thing for the
> > > SAVE_PAGE_TYPE_AS_ONLY_HTML case.
> > 
> > Does it?  SaveHTMLOnly test seems to be even assert that the directory
> *didn't* get created.
> > (BTW: this test was not touched by my recent set of changes - I just folder
> calculating of the directory path/string into a single shared function).
> 
> For ONLY_HTML, it now returns an ".mht" extension.
> 

Doh, you're right.  I don't know how I could have missed that :-(

Done.
(for a moment I thought whether to use a switch statement for this, but it felt
like an overkill)

[asanka, 2015-11-12 16:03:03]

lgtm

What's the story with Document.open() on an iframe? I recall from somewhere that
that scenario is commonplace for some ad networks, and mess with "Save as".

[Łukasz Anforowicz, 2015-11-12 17:13:28]

On 2015/11/12 16:03:03, asanka wrote:
> lgtm
> 
Thanks for reviewing.

> What's the story with Document.open() on an iframe? I recall from somewhere
that
> that scenario is commonplace for some ad networks, and mess with "Save as".

Thanks for pointing this out.  If you would have any specifics (uris / repro
steps) then it would greatly help.

I am not sure what is the significance of Document.open.  I am guessing that it
causes a frame to navigate to a different URI and this can cause a difference
between the "original URI" (what was in the parent's html document) and the
frame's current URI (last committed URI visible via
blink::WebDocument/WebFrame/etc).  This is known to confuse MHTML and I am
working on a fix (see crrev.com/1441553002 and crbug.com/539936).  OTOH, we
don't expose MHTML via Save-As UI/dialog (at least not by default / not without
a special command line flag).

There is also a tricky situation with frames without a specific URI (i.e. iframe
without src attribute but with content populated via srcdoc or by parent's
javascript/DOM manipulation).  These are require special handling for MHTML
cross-references to work + they behave slightly differently in complete-html
(because they have no src attribute, we don't rewrite their links to point to
locally saved files).  I tried to get *some* coverage via the new
frames-about-blank.htm tests, but I have a feeling that coverage here could be
improved (but I don't want to do it blindly and at this point I don't
know/understand exactly which testcases might expose potential problems).

[Łukasz Anforowicz, 2015-11-12 17:14:32]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-11-12 17:14:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412733017/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412733017/60001

[asanka, 2015-11-12 17:24:18]

On 2015/11/12 at 17:13:28, lukasza wrote:
> On 2015/11/12 16:03:03, asanka wrote:
> > lgtm
> > 
> Thanks for reviewing.
> 
> > What's the story with Document.open() on an iframe? I recall from somewhere
that
> > that scenario is commonplace for some ad networks, and mess with "Save as".
> 
> Thanks for pointing this out.  If you would have any specifics (uris / repro
steps) then it would greatly help.
> 
> I am not sure what is the significance of Document.open.  I am guessing that
it causes a frame to navigate to a different URI and this can cause a difference
between the "original URI" (what was in the parent's html document) and the
frame's current URI (last committed URI visible via
blink::WebDocument/WebFrame/etc).  This is known to confuse MHTML and I am
working on a fix (see crrev.com/1441553002 and crbug.com/539936).  OTOH, we
don't expose MHTML via Save-As UI/dialog (at least not by default / not without
a special command line flag).
> 
> There is also a tricky situation with frames without a specific URI (i.e.
iframe without src attribute but with content populated via srcdoc or by
parent's javascript/DOM manipulation).  These are require special handling for
MHTML cross-references to work + they behave slightly differently in
complete-html (because they have no src attribute, we don't rewrite their links
to point to locally saved files).  I tried to get *some* coverage via the new
frames-about-blank.htm tests, but I have a feeling that coverage here could be
improved (but I don't want to do it blindly and at this point I don't
know/understand exactly which testcases might expose potential problems).

Sorry, the latter was what I was referring to. It used to be that an iframe
without a src attribute or one where the parent document calls Document.open()
on ends up with the document URI of the parent. This used to cause problems
since now you don't have a 1-1 mapping from a URI to a persisted resource. See
https://code.google.com/p/chromium/issues/detail?id=106364

[commit-bot: I haz the power, 2015-11-12 19:00:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-12 20:08:44]

Patchset 4 (id:??) landed as
https://crrev.com/4771dd55cdc2ca26c955776056d540ec62da40e6
Cr-Commit-Position: refs/heads/master@{#359351}