OOPIFs: Moving GetSerializedHtmlData...WithLocalLinks away from RenderViewHost.

OOPIFs: Moving GetSerializedHtmlData...WithLocalLinks away from RenderViewHost.

This CL replaces ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks and
ViewHostMsg_SendSerializedHtmlData IPC messages by their
frame-oriented equivalents: FrameMsg_GetSerializedHtmlDataWithLocalLinks
and FrameHostMsg_SerializedHtmlDataWithLocalLinksResponse.

BUG=526786
Committed: https://crrev.com/2e296c32ef14fc32c998e634663943b8d39cca1f
Cr-Commit-Position: refs/heads/master@{#351200}

[Łukasz Anforowicz, 2015-09-21 21:54:17]

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

[Łukasz Anforowicz, 2015-09-21 21:54:18]

Nasko, could you please take a look?

[nasko, 2015-09-21 23:05:47]

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

[nasko, 2015-09-21 23:05:48]

Hey Alex, can you take a first stab at this one?

[alexmos, 2015-09-22 23:03:50]

Looks good overall, a few questions and nits below.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1065:
web_contents()->ForEachFrame(base::Bind(
Just to make sure I understand, is one SavePackage created per one page save
job, and are we not worried about starting a new job with an old one still in
progress?

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1078: target->Send(new
FrameMsg_GetSerializedHtmlDataWithLocalLinks(
Just wondering, what would happen if one of the renderer processes has crashed? 
(I.e., what would happen if you kill a subframe process in Chrome's task manager
and try to save the resulting page?)  Since this failure mode wasn't possible
before, I'm just curious if our behavior is going to be sane.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1083: // Process the serialized HTML
content data of a specified web page
nit: s/web page/frame/

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1084: // retrieved from render process.
nit: s/render/the renderer/

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1092: if (flag ==
WebPageSerializerClient::AllFramesAreFinished)
It looks like each frame will send back a CurrentFrameIsFinished followed by a
AllFramesAreFinished -- is that correct?  Maybe put a comment explaining why you
return here?  It doesn't seem like AllFramesAreFinished will be needed after
your refactor, and if that's the case, it might be worth adding a TODO to remove
it.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1153: // If the all frames are finished
saving, we need to close the
nit: s/the all/all/

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.h:191: // Set of methods to get html data
by serializing all frames of current page
nit: s/of/in the/

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.h:193: void
GetSerializedHtmlDataWithLocalLinksForCurrentPage();
These function names are really hard to parse, as is the preceding comment. 
Perhaps we could remove "WithLocalLinks" from the names, and instead explain
this part in more detail in the comments, along with what the parameters mean?

https://codereview.chromium.org/1320453004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4428: // Only DCHECK, since the message
comes from a trusted process (browser).
nit: s/a trusted process (browser)/the trusted browser process/

https://codereview.chromium.org/1320453004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4439:
WebPageSerializer::serialize(GetWebFrame(),
nit: maybe replace the "recursive? nope" with a more detailed comment here about
why this needs to be called non-recursively?

[Łukasz Anforowicz, 2015-09-23 16:29:28]

Alex, can you take another look please?

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1065:
web_contents()->ForEachFrame(base::Bind(
On 2015/09/22 23:03:49, alexmos wrote:
> Just to make sure I understand, is one SavePackage created per one page save
> job, and are we not worried about starting a new job with an old one still in
> progress?

Correct.  I base this assertion on presence of saved_main_file_path, page_url
and wait_state_ fields in SavePackage - all of these were present before my
changes and are associated with a single save page action.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1078: target->Send(new
FrameMsg_GetSerializedHtmlDataWithLocalLinks(
On 2015/09/22 23:03:49, alexmos wrote:
> Just wondering, what would happen if one of the renderer processes has
crashed? 
> (I.e., what would happen if you kill a subframe process in Chrome's task
manager
> and try to save the resulting page?)  Since this failure mode wasn't possible
> before, I'm just curious if our behavior is going to be sane.

The save will never finish (i.e. the download items keeps spinning the progress
circle and saying "Starting..." forever).  The user can cancel the "download"
the save-page-as is represented as.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1083: // Process the serialized HTML
content data of a specified web page
On 2015/09/22 23:03:49, alexmos wrote:
> nit: s/web page/frame/

Done.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1084: // retrieved from render process.
On 2015/09/22 23:03:49, alexmos wrote:
> nit: s/render/the renderer/

Done.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1092: if (flag ==
WebPageSerializerClient::AllFramesAreFinished)
On 2015/09/22 23:03:49, alexmos wrote:
> It looks like each frame will send back a CurrentFrameIsFinished followed by a
> AllFramesAreFinished -- is that correct?  Maybe put a comment explaining why
you
> return here?  It doesn't seem like AllFramesAreFinished will be needed after
> your refactor, and if that's the case, it might be worth adding a TODO to
remove
> it.

Correct - AllFramesAreFinished won't be needed after I am done.

I'll add a comment explaining what is happening with the code the current CL
leaves here.

I think I won't add a TODO comment, because this whole method will go away
eventually (see
https://docs.google.com/document/d/1OMRoBCBuLV-HOcL8ZGcstYdocq0kHmFbZhWwJC7GeGE
[the single IPC message with serialized data will be replaced with more granular
messages]), but the sequence of CLs to do this doesn't really start / doesn't
require a TODO until crrev.com/1346993005 lands.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1153: // If the all frames are finished
saving, we need to close the
On 2015/09/22 23:03:49, alexmos wrote:
> nit: s/the all/all/

Done.  I was just copy & pasting an old comment? :-)

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.h:191: // Set of methods to get html data
by serializing all frames of current page
On 2015/09/22 23:03:50, alexmos wrote:
> nit: s/of/in the/

Done.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.h:193: void
GetSerializedHtmlDataWithLocalLinksForCurrentPage();
On 2015/09/22 23:03:50, alexmos wrote:
> These function names are really hard to parse, as is the preceding comment. 
> Perhaps we could remove "WithLocalLinks" from the names, and instead explain
> this part in more detail in the comments, along with what the parameters mean?

Done:
- removed "data"
- removed ForCurrentPage, but kept ForFrame prefix
  (without the prefix getting a pointer to an *overloaded* method
   [to pass it to Bind] is rather clunky and ugly).

Also note that I've applied similar changes to the methods touched in the
previous CL as well (i.e. GetSavableResourceLinksForCurrentPage).

https://codereview.chromium.org/1320453004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4428: // Only DCHECK, since the message
comes from a trusted process (browser).
On 2015/09/22 23:03:50, alexmos wrote:
> nit: s/a trusted process (browser)/the trusted browser process/

Done.

https://codereview.chromium.org/1320453004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4439:
WebPageSerializer::serialize(GetWebFrame(),
On 2015/09/22 23:03:50, alexmos wrote:
> nit: maybe replace the "recursive? nope" with a more detailed comment here
about
> why this needs to be called non-recursively?

Done.  I've left this comment, but also added a "top-level" comment about why
non-recursive mode is used here.

[alexmos, 2015-09-23 19:53:30]

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1065:
web_contents()->ForEachFrame(base::Bind(
On 2015/09/23 16:29:28, Łukasz Anforowicz wrote:
> On 2015/09/22 23:03:49, alexmos wrote:
> > Just to make sure I understand, is one SavePackage created per one page save
> > job, and are we not worried about starting a new job with an old one still
in
> > progress?
> 
> Correct.  I base this assertion on presence of saved_main_file_path, page_url
> and wait_state_ fields in SavePackage - all of these were present before my
> changes and are associated with a single save page action.

Acknowledged.

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1078: target->Send(new
FrameMsg_GetSerializedHtmlDataWithLocalLinks(
On 2015/09/23 16:29:28, Łukasz Anforowicz wrote:
> On 2015/09/22 23:03:49, alexmos wrote:
> > Just wondering, what would happen if one of the renderer processes has
> crashed? 
> > (I.e., what would happen if you kill a subframe process in Chrome's task
> manager
> > and try to save the resulting page?)  Since this failure mode wasn't
possible
> > before, I'm just curious if our behavior is going to be sane.
> 
> The save will never finish (i.e. the download items keeps spinning the
progress
> circle and saying "Starting..." forever).  The user can cancel the "download"
> the save-page-as is represented as.

Acknowledged.  This is probably good enough for now -- I'm happy that we don't
crash. :)

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1092: if (flag ==
WebPageSerializerClient::AllFramesAreFinished)
On 2015/09/23 16:29:28, Łukasz Anforowicz wrote:
> On 2015/09/22 23:03:49, alexmos wrote:
> > It looks like each frame will send back a CurrentFrameIsFinished followed by
a
> > AllFramesAreFinished -- is that correct?  Maybe put a comment explaining why
> you
> > return here?  It doesn't seem like AllFramesAreFinished will be needed after
> > your refactor, and if that's the case, it might be worth adding a TODO to
> remove
> > it.
> 
> Correct - AllFramesAreFinished won't be needed after I am done.
> 
> I'll add a comment explaining what is happening with the code the current CL
> leaves here.
> 
> I think I won't add a TODO comment, because this whole method will go away
> eventually (see
>
https://docs.google.com/document/d/1OMRoBCBuLV-HOcL8ZGcstYdocq0kHmFbZhWwJC7GeGE
> [the single IPC message with serialized data will be replaced with more
granular
> messages]), but the sequence of CLs to do this doesn't really start / doesn't
> require a TODO until crrev.com/1346993005 lands.  

Acknowledged.

https://codereview.chromium.org/1320453004/diff/40001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/40001/content/browser/downloa...
content/browser/download/save_package.cc:1157: // If all frames are finished
saving, we need to close the
nit: "remaining" fits on this line now.

https://codereview.chromium.org/1320453004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (left):

https://codereview.chromium.org/1320453004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:258: void
GetSerializedHtmlDataForCurrentPageWithLocalLinks(
These comments explaining what the arguments mean seem useful.  Even though this
function isn't used anywhere (hence its removal), I'm wondering if the comments
would apply to GetSerializedHtmlWithLocalLinksForFrame?  Should we move them
there (and fix their grammar while at it)?

[Łukasz Anforowicz, 2015-09-23 20:24:03]

Alex, can you take one more look please?

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1078: target->Send(new
FrameMsg_GetSerializedHtmlDataWithLocalLinks(
On 2015/09/23 19:53:30, alexmos wrote:
> On 2015/09/23 16:29:28, Łukasz Anforowicz wrote:
> > On 2015/09/22 23:03:49, alexmos wrote:
> > > Just wondering, what would happen if one of the renderer processes has
> > crashed? 
> > > (I.e., what would happen if you kill a subframe process in Chrome's task
> > manager
> > > and try to save the resulting page?)  Since this failure mode wasn't
> possible
> > > before, I'm just curious if our behavior is going to be sane.
> > 
> > The save will never finish (i.e. the download items keeps spinning the
> progress
> > circle and saying "Starting..." forever).  The user can cancel the
"download"
> > the save-page-as is represented as.
> 
> Acknowledged.  This is probably good enough for now -- I'm happy that we don't
> crash. :)

Clarification:
- "saving forever" when renderer process is gone
  happens after *and* before OOPIF-related CLs
- we *will* crash when saving a page with cross-site frames
  when running with site isolation cmdline flag
  This CL (and previous CL) simply moves the point of
  crashing further and further away (and the next set
  of CLs moving path stitching to the browser will make
  the crash go away).

https://codereview.chromium.org/1320453004/diff/40001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/40001/content/browser/downloa...
content/browser/download/save_package.cc:1157: // If all frames are finished
saving, we need to close the
On 2015/09/23 19:53:30, alexmos wrote:
> nit: "remaining" fits on this line now.

Done.

https://codereview.chromium.org/1320453004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (left):

https://codereview.chromium.org/1320453004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:258: void
GetSerializedHtmlDataForCurrentPageWithLocalLinks(
On 2015/09/23 19:53:30, alexmos wrote:
> These comments explaining what the arguments mean seem useful.  Even though
this
> function isn't used anywhere (hence its removal), I'm wondering if the
comments
> would apply to GetSerializedHtmlWithLocalLinksForFrame?  Should we move them
> there (and fix their grammar while at it)?

These arguments will disappear in a CL or 2, but I guess copying these argument
description comments won't hurt :-)  OTOH, I am not sure if it is worth spending
much time on the grammar (since the arguments+comments  will go away soon).  

Also note that the current comment format is blink-based / almost directly
copied from WebPageSerializer::serialize comment
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

[alexmos, 2015-09-23 20:43:50]

Thanks, LGTM.  Nasko, mind doing an OWNERS review?

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1078: target->Send(new
FrameMsg_GetSerializedHtmlDataWithLocalLinks(
On 2015/09/23 20:24:03, Łukasz Anforowicz wrote:
> On 2015/09/23 19:53:30, alexmos wrote:
> > On 2015/09/23 16:29:28, Łukasz Anforowicz wrote:
> > > On 2015/09/22 23:03:49, alexmos wrote:
> > > > Just wondering, what would happen if one of the renderer processes has
> > > crashed? 
> > > > (I.e., what would happen if you kill a subframe process in Chrome's task
> > > manager
> > > > and try to save the resulting page?)  Since this failure mode wasn't
> > possible
> > > > before, I'm just curious if our behavior is going to be sane.
> > > 
> > > The save will never finish (i.e. the download items keeps spinning the
> > progress
> > > circle and saying "Starting..." forever).  The user can cancel the
> "download"
> > > the save-page-as is represented as.
> > 
> > Acknowledged.  This is probably good enough for now -- I'm happy that we
don't
> > crash. :)
> 
> Clarification:
> - "saving forever" when renderer process is gone
>   happens after *and* before OOPIF-related CLs
> - we *will* crash when saving a page with cross-site frames
>   when running with site isolation cmdline flag
>   This CL (and previous CL) simply moves the point of
>   crashing further and further away (and the next set
>   of CLs moving path stitching to the browser will make
>   the crash go away).

Right.  I guess my question was more about the final state of things once you're
done with your remaining CLs. :)

https://codereview.chromium.org/1320453004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (left):

https://codereview.chromium.org/1320453004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:258: void
GetSerializedHtmlDataForCurrentPageWithLocalLinks(
> Also note that the current comment format is blink-based / almost directly
> copied from WebPageSerializer::serialize comment
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 

Ah, ok.  I just wanted to make sure we don't lose them.

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
content/browser/download/save_package.h:194: // Get html data by serializing
|target| frame with lists
nit: maybe separate these methods with blank lines since the second one has
comments now.  Might be worth putting a short comment on the third one as well
(or is that going to go away too?)

[nasko, 2015-09-23 21:49:27]

Mostly nits.

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
content/browser/download/save_package.cc:1170: return;
nit: This return isn't needed.

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
content/browser/download/save_package.h:194: // Get html data by serializing
|target| frame with lists
On 2015/09/23 20:43:50, alexmos wrote:
> nit: maybe separate these methods with blank lines since the second one has
> comments now.  Might be worth putting a short comment on the third one as well
> (or is that going to go away too?)

+1

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:140: #include
"third_party/WebKit/public/web/WebPageSerializerClient.h"
This one is already included in the header file. Actually, it should probably be
forward declared in the header without an include and this include to stay.

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4448: void
RenderFrameImpl::didSerializeDataForFrame(
Ordering of method implementations should match the ordering in the header file.

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:499: ~RenderViewImpl() override;
Why does the destructor change in this CL?

[Łukasz Anforowicz, 2015-09-23 23:01:52]

Nasko, can you take another look please?

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
content/browser/download/save_package.cc:1170: return;
On 2015/09/23 21:49:27, nasko (slow to review) wrote:
> nit: This return isn't needed.

Done.

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/1320453004/diff/60001/content/browser/downloa...
content/browser/download/save_package.h:194: // Get html data by serializing
|target| frame with lists
On 2015/09/23 21:49:27, nasko (slow to review) wrote:
> On 2015/09/23 20:43:50, alexmos wrote:
> > nit: maybe separate these methods with blank lines since the second one has
> > comments now.  Might be worth putting a short comment on the third one as
well
> > (or is that going to go away too?)
> 
> +1

Done.

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:140: #include
"third_party/WebKit/public/web/WebPageSerializerClient.h"
On 2015/09/23 21:49:27, nasko (slow to review) wrote:
> This one is already included in the header file. Actually, it should probably
be
> forward declared in the header without an include and this include to stay.

I think we cannot do a forward declaraion in the header - the full definition of
WPSC needs to be included in render_frame_impl.h, because otherwise RenderFrame
in render_frame_impl.h can't *override* virtual methods declared by WPSC.

I've removed the include in render_frame_impl.cc

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4448: void
RenderFrameImpl::didSerializeDataForFrame(
On 2015/09/23 21:49:27, nasko (slow to review) wrote:
> Ordering of method implementations should match the ordering in the header
file.

Done.  (i.e. I edited to make sure that didSerializedDataForFrame is right after
PlayerGone + OnGetSerializedHtmlWithLocalLinks is right after
OnGetSavableResourceLinks;  OTOH I am leaving other methods here with
inconsistent ordering between the header and the implementation)

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1320453004/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:499: ~RenderViewImpl() override;
On 2015/09/23 21:49:27, nasko (slow to review) wrote:
> Why does the destructor change in this CL?

Previously there was no virtual destructor in any of the base classes.  Now
(when we derive from WebPageSerializerClient) there is a virtual destructor that
we are overriding.

[nasko, 2015-09-23 23:22:47]

(all but save page) LGTM with a nit.

https://codereview.chromium.org/1320453004/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1320453004/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3876: // blink::WebPageSerializerClient
implementation
nit: This comment isn't needed. We only use those in the header.

[Łukasz Anforowicz, 2015-09-24 16:06:24]

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

[Łukasz Anforowicz, 2015-09-24 16:06:25]

Randy, could you please take a look?

https://codereview.chromium.org/1320453004/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1320453004/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3876: // blink::WebPageSerializerClient
implementation
On 2015/09/23 23:22:47, nasko (slow to review) wrote:
> nit: This comment isn't needed. We only use those in the header.

Done.

[Łukasz Anforowicz, 2015-09-25 18:13:47]

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

[Łukasz Anforowicz, 2015-09-25 18:13:48]

Asanka, could you please take a look?  (I believe Randy is out of office for a
few days).

[Randy Smith (Not in Mondays), 2015-09-28 17:11:18]

Looks fairly good; just a few nits.

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

https://codereview.chromium.org/1320453004/diff/140001/content/browser/downlo...
content/browser/download/save_package.h:191: // Ask renderer processes to
serialize all frames in the current page.
nit: "Ask renderer processes" to me reads as if the renderer or renderers are
splitting up the requests across the frames.  Maybe instead "Ask the renderer
process associated with each frame to serialize that frame."?

https://codereview.chromium.org/1320453004/diff/140001/content/browser/downlo...
content/browser/download/save_package.h:196: // The parameter |saved_links|
contains original URLs of all saved links.
Worthwhile mentioning that this parameter may have URLs that aren't in the
frame?

https://codereview.chromium.org/1320453004/diff/140001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1320453004/diff/140001/content/common/frame_m...
content/common/frame_messages.h:678: // links with a path to the local copy
passed in the message payload.
Worthwhile mentioning that the GURL vector may include URLs that aren't in the
frame?

[Łukasz Anforowicz, 2015-09-28 17:30:47]

Thanks Randy.  Could you take another look please?

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

https://codereview.chromium.org/1320453004/diff/140001/content/browser/downlo...
content/browser/download/save_package.h:191: // Ask renderer processes to
serialize all frames in the current page.
On 2015/09/28 17:11:17, rdsmith wrote:
> nit: "Ask renderer processes" to me reads as if the renderer or renderers are
> splitting up the requests across the frames.  Maybe instead "Ask the renderer
> process associated with each frame to serialize that frame."?

Thanks - I tried to reword the comment.

https://codereview.chromium.org/1320453004/diff/140001/content/browser/downlo...
content/browser/download/save_package.h:196: // The parameter |saved_links|
contains original URLs of all saved links.
On 2015/09/28 17:11:17, rdsmith wrote:
> Worthwhile mentioning that this parameter may have URLs that aren't in the
> frame?

Thanks - I've added more detailed discussion of the |saved_links| parameter.

https://codereview.chromium.org/1320453004/diff/140001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1320453004/diff/140001/content/common/frame_m...
content/common/frame_messages.h:678: // links with a path to the local copy
passed in the message payload.
On 2015/09/28 17:11:18, rdsmith wrote:
> Worthwhile mentioning that the GURL vector may include URLs that aren't in the
> frame?

This argument will go away in crrev.com/1373573002 (I planned to send it for CR
from you after getting sign-off from WebKit and OOPIF side, but feel free to
provide early drive-by feedback).

I could add a comment that you are suggesting, but 1) there is no good place for
a more detailed argument description [well, I guess I could carve out some space
in the message-level comment] and 2) this argument will go away soon.  Therefore
I think the comment for SavePackage::GetSerializedHtmlWithLocalLinksForFrame
should be sufficient.

[Randy Smith (Not in Mondays), 2015-09-28 19:18:14]

LGTM.  I'll wait for your heads up before reviewing the other CL.

[Łukasz Anforowicz, 2015-09-28 22:09:11]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-09-28 22:09:11]

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

[commit-bot: I haz the power, 2015-09-28 22:10:37]

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

[commit-bot: I haz the power, 2015-09-28 23:30:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-28 23:30:56]

Patchset 10 (id:??) landed as
https://crrev.com/2e296c32ef14fc32c998e634663943b8d39cca1f
Cr-Commit-Position: refs/heads/master@{#351200}