Save-Page-As-Complete-Html: Fixing how links are rewritten in subframes.

Save-Page-As-Complete-Html: Fixing how links are rewritten in subframes.

This CL fixes how links are rewritten in subframes, by tweaking
save_package.cc to exclude the "directory" part from links rewritten
for subframes (i.e. only links rewritten for the main frame should
include the "directory" part).

Rewriting of links in subframes has regressed in crrev.com/1320453004 [1],
which stopped using "recursive" serialization mode of WebPageSerializerImpl.
In "recursive" serialization mode only the main frame would use a
non-empty directory name in WebPageSerializerImpl::serialize [2].
Note that crrev.com/1320453004 has initially landed in 47.0.2522.0.

This regression meant that we used local links of the form
"./foo_files/img.png" in all frames (i.e. in ./foo_files/subframe.htm)
rather than only in the main frame (i.e. in foo.htm).

Note that saving subframes had some issues also before the regression
(i.e. 46.0.2490.80 has trouble saving
https://anforowicz.github.io/nested-frames/index.htm as noted in
crbug.com/554666), but the regression in crrev.com/1320453004 made
things even worse... :-/

[1] See in crrev.com/1320453004/patch/180001/190008 how
    WebPageSerializer::serialize is called.

[2] https://chromium.googlesource.com/chromium/src/+/11d41626fbe8e88fb507371df3beda99fce268a1/third_party/WebKit/Source/web/WebPageSerializerImpl.cpp#504

BUG=554666
Committed: https://crrev.com/0296979a51bf3e6347371c9b3432d7c90f73cec1
Cr-Commit-Position: refs/heads/master@{#359705}

[Łukasz Anforowicz, 2015-11-12 18:14:31]

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

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

Asanka, can you take a look please?

[Łukasz Anforowicz, 2015-11-13 00:25:46]

Asanka / Randy - ping :-) ?  My apologies for pinging before the expected 24h
turnaround time, but (since this CL fixes a recent regression) it would be great
if this CL could land before the M48 branchpoint - I would really appreciate if
you could take a look today or early on Friday.

[asanka, 2015-11-13 22:00:31]

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

[asanka, 2015-11-13 22:00:31]

I'm tagging in rdsmith@ who I hope isn't on a plane. Otherwise I'll familiarize
myself with WebPageSerializer enough to review this later tonight.

[Randy Smith (Not in Mondays), 2015-11-13 22:18:32]

On 2015/11/13 22:00:31, asanka wrote:
> I'm tagging in rdsmith@ who I hope isn't on a plane. Otherwise I'll
familiarize
> myself with WebPageSerializer enough to review this later tonight.

Reviewing (still in CA and working for the rest of the afternoon).

[Łukasz Anforowicz, 2015-11-13 22:20:13]

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

[commit-bot: I haz the power, 2015-11-13 22:20:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442733002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442733002/40001

[Randy Smith (Not in Mondays), 2015-11-13 22:39:07]

LGTM with nits below.

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

https://codereview.chromium.org/1442733002/diff/40001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:982: // as MHTML
(crbug.com/539936).
I find this comment a bit confusing "as well as for" suggests to me that it's
got multiple parts targeted at different areas.  Maybe restructure it to be of
the format "Test for Z ....  Regression test for crbug.com/X & crbug.com/Y."?

https://codereview.chromium.org/1442733002/diff/40001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:1007: // Test for
crbug.com/553478.
Could you include a line about what the test does as well as the bug regression
reference?  (Also below; I find a sentence comment before a test as to what it's
supposed to test very useful when reading tests.)

https://codereview.chromium.org/1442733002/diff/40001/chrome/browser/download...
chrome/browser/download/save_page_browsertest.cc:1061: ,
nit, suggestion: I'm no expert in the use of INSTANTIATE_TEST_CASE_P, but I'd be
inclined to include a prefix that describes the test variation space, i.e.
something like "SaveType".

[Łukasz Anforowicz, 2015-11-13 22:55:05]

Thanks for reviewing Randy.  I hope you won't mind if I push this out to CQ
as-is, and address the nits (around test comments and naming of the tests) in a
follow-up CL.  The CQ dry-run looks good so far.

[Randy Smith (Not in Mondays), 2015-11-13 23:02:13]

On 2015/11/13 22:55:05, Łukasz Anforowicz wrote:
> Thanks for reviewing Randy.  I hope you won't mind if I push this out to CQ
> as-is, and address the nits (around test comments and naming of the tests) in
a
> follow-up CL.  The CQ dry-run looks good so far.

Sure, that's fine.

[commit-bot: I haz the power, 2015-11-14 00:05:56]

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

[commit-bot: I haz the power, 2015-11-14 00:05:57]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2015-11-14 00:06:08]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-11-14 00:08:58]

The CQ bit was unchecked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-11-14 00:09:02]

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

[Łukasz Anforowicz, 2015-11-14 00:10:39]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-11-14 00:11:16]

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

[Łukasz Anforowicz, 2015-11-14 00:30:55]

The CQ bit was unchecked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-11-14 00:33:55]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-11-14 00:36:00]

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

[commit-bot: I haz the power, 2015-11-14 00:48:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-14 00:49:27]

Patchset 3 (id:??) landed as
https://crrev.com/0296979a51bf3e6347371c9b3432d7c90f73cec1
Cr-Commit-Position: refs/heads/master@{#359705}