Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- WebFrameSerializerImpl now rewrites link uris based on 2 maps
  (url -> local path map as well as frame -> local path map).

BUG=106364, 106158, 538188
Committed: https://crrev.com/777a7dd63a55b297f0c9543f1892cecb54505a1f
Cr-Commit-Position: refs/heads/master@{#371349}

[Łukasz Anforowicz, 2015-12-16 21:23:12]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame into a separate file.

TODO: Expand the CL description.

BUG=106364
==========

to

==========
Save-Page-As-Complete-Html: Each frame into a separate file.

Previous CL (crrev.com/1484093002) made it possible to save contents of all
frames - even if some of them have the same URI, they still get saved into
separate files / SaveItems.  This CL makes sure that we rewrite links in a html
document, based on frame identity if possible (rather than always doing the
rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id to track

BUG=106364
==========

[Łukasz Anforowicz, 2015-12-16 21:28:55]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame into a separate file.

Previous CL (crrev.com/1484093002) made it possible to save contents of all
frames - even if some of them have the same URI, they still get saved into
separate files / SaveItems.  This CL makes sure that we rewrite links in a html
document, based on frame identity if possible (rather than always doing the
rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id to track

BUG=106364
==========

to

==========
Save-Page-As-Complete-Html: Each frame into a separate file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364
==========

[Łukasz Anforowicz, 2015-12-16 21:38:27]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame into a separate file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364
==========

to

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364
==========

[Łukasz Anforowicz, 2015-12-16 21:38:33]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364
==========

to

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364
==========

[Łukasz Anforowicz, 2015-12-16 21:39:01]

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

[Łukasz Anforowicz, 2015-12-16 21:39:02]

Daniel, could you please take a look?

https://codereview.chromium.org/1502563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPageSerializerImpl.cpp (right):

https://codereview.chromium.org/1502563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPageSerializerImpl.cpp:314: frame =
toHTMLFrameOwnerElement(element)->contentFrame();
There is some risk that this toHTMLFrameOwnerElement might blow-up in a way
similar to crbug.com/570073...  I can't repro this bug locally, but I am not
sure about if the predicate below is really correct (as my usage of
HTMLFrameOwnerElement is agnostic to local-vs-remote distinction):

  HTMLFrameOwnerElement.h:
    DEFINE_TYPE_CASTS(HTMLFrameOwnerElement, ..., owner->isLocal()...

https://codereview.chromium.org/1502563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPageSerializerImpl.h (right):

https://codereview.chromium.org/1502563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPageSerializerImpl.h:47: #include <utility>
I think I should have added this include earlier - for std::pair

[dcheng, 2015-12-29 18:39:20]

Can we change this to use a delegate (similar to the MHTML deduplication patch)
to avoid this awkward map -> WebVector plumbing?

[Łukasz Anforowicz, 2015-12-29 22:07:03]

On 2015/12/29 18:39:20, dcheng wrote:
> Can we change this to use a delegate (similar to the MHTML deduplication
patch)
> to avoid this awkward map -> WebVector plumbing?

Good idea - done.  I introduced WebPageSerializer::LinkRewritingDelegate.  I
think it is best to have this as a separate interface / not bundle this with
either WebPageSerializerClient (other interface used by complete-html) and/or
PageSerializer::Delegate (element-based link rewriting + skipping of attributes)
and/or WebPageSerializer::MHTMLPartGenerationDelegate (content id for frames
[link rewriting not directly exposed] + attribute skipping).

[Łukasz Anforowicz, 2016-01-04 19:53:03]

+rdsmith@ (see 1st paragraph below)

Daniel,

I thought a bit more about the newly introduced SaveItem::frame_tree_node_id_
field - I agree that this field smells a bit bad (because it is not applicable
to all SaveItems).  OTOH, after thinking about it more, I think that I don't
like the alternative of effectively simulating the field by a map in SavePackage
that maps from SaveItem(Id) into frame_tree_node_id (FWIW, SaveItem/SaveItemId
is not at this point used as a key in any of maps held by SavePackage - there is
no precedent). I've added Randy to CC in case he wants to chime in here (as an
OWNER of content/browser/downloads).

I also wanted to point out one area that I am slightly unhappy about -
duplication of SingleLinkRewritingDelegate test class in
content/renderer/dom_serializer_browsertest.cc and in
third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp.  I think the
duplication is bad, but acceptable for the relatively simple test code.  I
assume that exposing SingleLinkRewritingDelegate through Blink's public API (so
that it can be accessed by dom_serializer_browsertest.cc) would be even uglier. 
WDYT?

[Randy Smith (Not in Mondays), 2016-01-05 19:06:05]

On 2016/01/04 19:53:03, Łukasz Anforowicz wrote:
> +rdsmith@ (see 1st paragraph below)
> 
> Daniel,
> 
> I thought a bit more about the newly introduced SaveItem::frame_tree_node_id_
> field - I agree that this field smells a bit bad (because it is not applicable
> to all SaveItems).  OTOH, after thinking about it more, I think that I don't
> like the alternative of effectively simulating the field by a map in
SavePackage
> that maps from SaveItem(Id) into frame_tree_node_id (FWIW, SaveItem/SaveItemId
> is not at this point used as a key in any of maps held by SavePackage - there
is
> no precedent). I've added Randy to CC in case he wants to chime in here (as an
> OWNER of content/browser/downloads).

Shooting from the hip without refreshing my knowledge of  the code: Naively,
this feels like the right thing is to split SaveItems into SaveFrames and
SaveResources (no attachment to those names); there have been a couple of ways
in which the two diverge now (the other one that I remember is around whether or
not we deduplicate and hence need them in the map used for deduplication).  I'm
not sure how much work that would be, and I don't feel like it's a requirement
for this CL or anything.  If you don't want to do that, I'm neutral between a
separate sid->ftnid map and including the ftnid in the structure.  I don't like
either for the same reasons as you, but not enough to object strongly.

> 
> I also wanted to point out one area that I am slightly unhappy about -
> duplication of SingleLinkRewritingDelegate test class in
> content/renderer/dom_serializer_browsertest.cc and in
> third_party/WebKit/Source/web/tests/WebFrameSerializerTest.cpp.  I think the
> duplication is bad, but acceptable for the relatively simple test code.  I
> assume that exposing SingleLinkRewritingDelegate through Blink's public API
(so
> that it can be accessed by dom_serializer_browsertest.cc) would be even
uglier. 
> WDYT?

[Łukasz Anforowicz, 2016-01-11 17:15:47]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364
==========

to

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364, 106158, 538188
==========

[Łukasz Anforowicz, 2016-01-11 23:40:40]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- In addition to a url->local_path map, we plumb through a second map
  for frame->local_path mapping.  This goes through all the layers
  (generating the map in save_package.cc, passing the map through IPC,
  unpacking the map in render_frame_impl.cc and passing the map through
  WebPageSerializer into WebPageSerializeImpl.

- WebPageSerializerImpl now rewrites link uris based on 2 maps.

BUG=106364, 106158, 538188
==========

to

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- WebFrameSerializerImpl now rewrites link uris based on 2 maps
  (url -> local path map as well as frame -> local path map).

BUG=106364, 106158, 538188
==========

[dcheng, 2016-01-12 00:10:47]

https://codereview.chromium.org/1502563004/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1502563004/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:598: int routing_id =
GetRoutingIdForFrameOrProxy(const_cast<WebFrame*>(&frame));
I wonder if we'd be better off just passing these as WebFrame* in the public
API.

The const casts are pretty icky, and it seems a natural extension of //content
being const hostile.

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameSerializerImpl.cpp (right):

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializerImpl.cpp:435: // Make sure we
have non 0 client and delegate.
Nit: non null

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameSerializer.h (right):

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:109: virtual bool
rewriteLink(const WebFrame&, WebString* rewrittenLink) = 0;
Let's distinguish the method names: how about rewriteFrameSource() and
rewriteLink()?

[Łukasz Anforowicz, 2016-01-13 22:32:23]

Daniel, can you take another look please?

https://codereview.chromium.org/1502563004/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1502563004/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:598: int routing_id =
GetRoutingIdForFrameOrProxy(const_cast<WebFrame*>(&frame));
On 2016/01/12 00:10:47, dcheng wrote:
> I wonder if we'd be better off just passing these as WebFrame* in the public
> API.
> 
> The const casts are pretty icky, and it seems a natural extension of //content
> being const hostile.

Done (= s/const WebFrame&/WebFrame*/ in both LinkRewritingDelegate and
MHTMLPartsGenerationDelegate).

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameSerializerImpl.cpp (right):

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializerImpl.cpp:435: // Make sure we
have non 0 client and delegate.
On 2016/01/12 00:10:47, dcheng wrote:
> Nit: non null

Done.

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameSerializer.h (right):

https://codereview.chromium.org/1502563004/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:109: virtual bool
rewriteLink(const WebFrame&, WebString* rewrittenLink) = 0;
On 2016/01/12 00:10:47, dcheng wrote:
> Let's distinguish the method names: how about rewriteFrameSource() and
> rewriteLink()?

Done.

[dcheng, 2016-01-13 22:40:42]

lgtm

[Łukasz Anforowicz, 2016-01-13 23:09:27]

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

[Łukasz Anforowicz, 2016-01-13 23:09:28]

Randy, could you please take a look?

[Randy Smith (Not in Mondays), 2016-01-22 15:13:10]

Lukasz: Detailed review still in process, but I didn't see a response from you
to my question earlier in the review (about splitting out SaveItem into
SaveFrame and SaveResource) and I'm curious as to what you think of it, since my
more detailed read-through of the CL reinforces my pull in that direction.  Not
a requirement for this CL, but I want to start the discussion.

[Randy Smith (Not in Mondays), 2016-01-22 15:28:39]

Sorry I took so long--didn't actually take very long when I sat down and
focussed on it.  Below is all minor stuff, and I won't block the CL on my
question above.  Having said that, I would like to hear your perspective on my
question :-}.

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
chrome/browser/download/save_page_browsertest.cc:1118: // test frames.  In
particular, the |complete_html_arr| below should be the
I don't understand what is missing from this CL to enable this functionality? 
Are we dependent in the code somewhere on there being an associated URL for
frames even after this change?

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
chrome/browser/download/save_page_browsertest.cc:1134: "a.com",
"/save_page/frames-runtime-changes.htm?do_runtime_changes=1"));
nit, not this CL: This usage implies that do_runtime_changes=0 would inhibit the
runtime changes, but if I understand the test JS correctly, it's just the
existence of the "do_runtime_changes" string that initiates the runtime changes.

https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
content/browser/download/save_package.cc:1037: if (routing_id ==
MSG_ROUTING_NONE)
Is this also due to 541354?  If so, could you duplicate the comment or combine
the if ... continue tests in one place?

[Łukasz Anforowicz, 2016-01-22 17:39:39]

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

[Łukasz Anforowicz, 2016-01-22 17:39:40]

Randy, I've replied to your feedback (thanks!) but haven't actually made changes
in the CL yet (this will have to wait until Monday), so this is not quite ready
for re-review or l-g-t-m.

Nasko, could you please double-check my statement in the CR reply here:
https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...

On 2016/01/22 15:13:10, Randy Smith - Not in Fridays wrote:
> Lukasz: Detailed review still in process, but I didn't see a response from you
> to my question earlier in the review (about splitting out SaveItem into
> SaveFrame and SaveResource) and I'm curious as to what you think of it, since
my
> more detailed read-through of the CL reinforces my pull in that direction. 
Not
> a requirement for this CL, but I want to start the discussion.

Oh, yes, I should have replied via Rietveld, sorry about that.  I talked with
Daniel about this and I think what is currently in the CL is the best of the
available options.

I don't like splitting SaveItem into 2 classes, because I don't see how the
differences between frame-vs-resource handling can be handled via virtual
dispatch / class inheritance (i.e. the difference in this CL when building
slightly different payload for FrameMsg_GetSerializedHtmlWithLocalLinks OR the
existing differences special-cased by SAVE_FILE_FROM_DOM in save_packace.cc).  I
think I would be more comfortable with the split-SaveItem-approach if I could
convince myself that all if statements based on SaveFileSource (or
frame_tree_node_id) could be replaced by virtual dispatch.  Still - even then I
am not sure if the end result would be cleaner/simpler or more
bizzantine/complex (than what is currently in the existing code + in the CL).

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
chrome/browser/download/save_page_browsertest.cc:1118: // test frames.  In
particular, the |complete_html_arr| below should be the
On 2016/01/22 15:28:39, Randy Smith - Not in Fridays wrote:
> I don't understand what is missing from this CL to enable this functionality? 
> Are we dependent in the code somewhere on there being an associated URL for
> frames even after this change?

This CL almost gets us there - the missing thing is handling of frames without
src attribute - these won't get rewritten to point to a locally saved file.  The
CL to handle src/srcdoc will come next, after the current CL lands.  A draft is
at crrev.com/1500103002.

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
chrome/browser/download/save_page_browsertest.cc:1134: "a.com",
"/save_page/frames-runtime-changes.htm?do_runtime_changes=1"));
On 2016/01/22 15:28:39, Randy Smith - Not in Fridays wrote:
> nit, not this CL: This usage implies that do_runtime_changes=0 would inhibit
the
> runtime changes, but if I understand the test JS correctly, it's just the
> existence of the "do_runtime_changes" string that initiates the runtime
changes.

Oh, right.  Yes - I am not quite happy with how this is done.  Basically, I want
to make sure that original page (the one being saved) gets its content populated
at runtime by javascript, while the saved page doesn't (i.e. the saved page
should have the final content already).

I wish javascript had a better way of handling query parameters.  Still - it
shouldn't be too difficult to add "=1" to the javascript in the test file.  I'll
try to remember to do it on Monday (when I get back my desktop PC after the move
to a new building).

https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
content/browser/download/save_package.cc:1037: if (routing_id ==
MSG_ROUTING_NONE)
On 2016/01/22 15:28:39, Randy Smith - Not in Fridays wrote:
> Is this also due to 541354?  If so, could you duplicate the comment or combine
> the if ... continue tests in one place?

Good point.  Right now I think that if |save_item_frame_tree_node| exists and
|target| exists, then we should never get a MSG_ROUTING_NONE here.  So I think
I'll change this if statement into a DCHECK.  And even if I am wrong, then
having MSG_ROUTING_NONE as a key in the map shouldn't hurt - there should be no
lookups using this key.

[Randy Smith (Not in Mondays), 2016-01-22 22:00:17]

>
https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
> content/browser/download/save_package.cc:1037: if (routing_id ==
> MSG_ROUTING_NONE)
> On 2016/01/22 15:28:39, Randy Smith - Not in Fridays wrote:
> > Is this also due to 541354?  If so, could you duplicate the comment or
combine
> > the if ... continue tests in one place?
> 
> Good point.  Right now I think that if |save_item_frame_tree_node| exists and
> |target| exists, then we should never get a MSG_ROUTING_NONE here.  So I think
> I'll change this if statement into a DCHECK.  And even if I am wrong, then
> having MSG_ROUTING_NONE as a key in the map shouldn't hurt - there should be
no
> lookups using this key.

LGTM conditional on this change, just so I minimize blocking you (the JS test
change is up to you--not really fair to make it part of this CL anyway).

[Łukasz Anforowicz, 2016-01-25 19:04:05]

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/1502563004/diff/140001/chrome/browser/downloa...
chrome/browser/download/save_page_browsertest.cc:1134: "a.com",
"/save_page/frames-runtime-changes.htm?do_runtime_changes=1"));
On 2016/01/22 17:39:39, Łukasz Anforowicz wrote:
> On 2016/01/22 15:28:39, Randy Smith - Not in Fridays wrote:
> > nit, not this CL: This usage implies that do_runtime_changes=0 would inhibit
> the
> > runtime changes, but if I understand the test JS correctly, it's just the
> > existence of the "do_runtime_changes" string that initiates the runtime
> changes.
> 
> Oh, right.  Yes - I am not quite happy with how this is done.  Basically, I
want
> to make sure that original page (the one being saved) gets its content
populated
> at runtime by javascript, while the saved page doesn't (i.e. the saved page
> should have the final content already).
> 
> I wish javascript had a better way of handling query parameters.  Still - it
> shouldn't be too difficult to add "=1" to the javascript in the test file. 
I'll
> try to remember to do it on Monday (when I get back my desktop PC after the
move
> to a new building).

Done (tweaked the test html / javascript so that do_runtime_changes will only
take effect if its set to 1).

https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1502563004/diff/140001/content/browser/downlo...
content/browser/download/save_package.cc:1037: if (routing_id ==
MSG_ROUTING_NONE)
On 2016/01/22 17:39:40, Łukasz Anforowicz wrote:
> On 2016/01/22 15:28:39, Randy Smith - Not in Fridays wrote:
> > Is this also due to 541354?  If so, could you duplicate the comment or
combine
> > the if ... continue tests in one place?
> 
> Good point.  Right now I think that if |save_item_frame_tree_node| exists and
> |target| exists, then we should never get a MSG_ROUTING_NONE here.  So I think
> I'll change this if statement into a DCHECK.  And even if I am wrong, then
> having MSG_ROUTING_NONE as a key in the map shouldn't hurt - there should be
no
> lookups using this key.

Done (talked with Nasko and used a DCHECK [instead of an if statement +
continue] to verify that we are not getting MSG_ROUTING_NONE).

[Łukasz Anforowicz, 2016-01-25 19:04:14]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-01-25 19:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1502563004/#ps160001
(title: "Addressed feedback from Randy (and apparently accidentally rebased as
well...).")

[commit-bot: I haz the power, 2016-01-25 19:06:31]

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

[commit-bot: I haz the power, 2016-01-25 19:19:01]

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

[commit-bot: I haz the power, 2016-01-25 19:19:02]

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

[Łukasz Anforowicz, 2016-01-25 19:30:45]

Nasko - could you please do an owners review for:
    content/renderer/dom_serializer_browsertest.cc
    content/renderer/render_frame_impl.cc
    content/renderer/render_frame_impl.h
?

[nasko, 2016-01-25 21:26:43]

content/renderer/dom_serializer_browsertest.cc
    content/renderer/render_frame_impl.cc
    content/renderer/render_frame_impl.h

LGTM with a couple of nits.

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/dom_s...
File content/renderer/dom_serializer_browsertest.cc (right):

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/dom_s...
content/renderer/dom_serializer_browsertest.cc:168: WebString* rewritten_link)
override {
The indentation on this looks wrong. Have you done "git cl format " on this CL?

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:604: return false;  // This can happen
because of crbug.com/541354.
nit: https://

[Łukasz Anforowicz, 2016-01-25 22:10:31]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-01-25 22:10:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, dcheng@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1502563004/#ps200001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-01-25 22:11:54]

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

[Łukasz Anforowicz, 2016-01-25 23:11:37]

Ooops, I forgot to publish my comments before pushing to CQ.  Better late than
never? :-/

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/dom_s...
File content/renderer/dom_serializer_browsertest.cc (right):

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/dom_s...
content/renderer/dom_serializer_browsertest.cc:168: WebString* rewritten_link)
override {
On 2016/01/25 21:26:42, nasko wrote:
> The indentation on this looks wrong. Have you done "git cl format " on this
CL?

Done (I must have missed this step after changing the method name).

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1502563004/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:604: return false;  // This can happen
because of crbug.com/541354.
On 2016/01/25 21:26:42, nasko wrote:
> nit: https://

Done.

[commit-bot: I haz the power, 2016-01-25 23:55:59]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-01-25 23:59:03]

Description was changed from

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- WebFrameSerializerImpl now rewrites link uris based on 2 maps
  (url -> local path map as well as frame -> local path map).

BUG=106364, 106158, 538188
==========

to

==========
Save-Page-As-Complete-Html: Each frame links to a distinct local file.

Previous CL (crrev.com/1484093002) made it possible to save contents of
all frames - even if some of them have the same URI, they still get
saved into separate files / SaveItems.  This CL makes sure that we
rewrite links in a html document, based on frame identity if possible
(rather than always doing the rewriting based on the uri):

- SaveItem now stores a frame_tree_node_id that can be potentially used
  (if the SaveItem is for a frame rather than for a savable resource) as
  a key in the map into the local paths.

- WebFrameSerializerImpl now rewrites link uris based on 2 maps
  (url -> local path map as well as frame -> local path map).

BUG=106364, 106158, 538188

Committed: https://crrev.com/777a7dd63a55b297f0c9543f1892cecb54505a1f
Cr-Commit-Position: refs/heads/master@{#371349}
==========

[commit-bot: I haz the power, 2016-01-25 23:59:04]

Patchset 11 (id:??) landed as
https://crrev.com/777a7dd63a55b297f0c9543f1892cecb54505a1f
Cr-Commit-Position: refs/heads/master@{#371349}