OOPIFs: Deduplicating MHTML parts across frames.

OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in
presence of out-of-process iframes, but can potentially include the same
MHTML part twice.  MHTML with duplicated parts can be rendered by Chrome
and Internet Explorer without problems (at least according to the
limited ad-hoc testing I've done), but the unnecessary increase of the
file size is a performance regression.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).  This fixes the issue pointed
out above.

BUG=538766
Committed: https://crrev.com/ede9cc06d2550f0ee0dce0285cc88c872809c1bb
Cr-Commit-Position: refs/heads/master@{#367206}

[Łukasz Anforowicz, 2015-12-02 23:33:55]

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

[Łukasz Anforowicz, 2015-12-02 23:33:55]

nick@, could you please take a look?

[ncarter (slow), 2015-12-04 21:52:40]

lgtm

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

https://codereview.chromium.org/1417323006/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.cc:175: std::set<std::string>
digests_of_uris_to_skip;
This is unused.

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

https://codereview.chromium.org/1417323006/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.h:17: class GURL;
You can't use forward declaration here; std::set<Foo> requires a definition for
Foo.

https://codereview.chromium.org/1417323006/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPageSerializer.h (right):

https://codereview.chromium.org/1417323006/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPageSerializer.h:107: // sequentially call
WebPageSeriazlierClient once the data buffer is full.
"Seriazlier"

[Łukasz Anforowicz, 2015-12-04 22:55:43]

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

[Łukasz Anforowicz, 2015-12-04 22:55:43]

Thanks for reviewing.  dcheng@, could you please take a look?

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

https://codereview.chromium.org/1417323006/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.cc:175: std::set<std::string>
digests_of_uris_to_skip;
On 2015/12/04 21:52:39, ncarter wrote:
> This is unused.

Thanks for catching this.  I must have missed this when converting things to use
FrameMsg_SerializeAsMHTML_Params.  Removed.

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

https://codereview.chromium.org/1417323006/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.h:17: class GURL;
On 2015/12/04 21:52:39, ncarter wrote:
> You can't use forward declaration here; std::set<Foo> requires a definition
for
> Foo.

I thought that this is ok, because I don't need a std::set<GURL> *value* - I
just need a reference.  OTOH, to name the type of the reference, I need to tell
the compiler that GURL is a valid typename.  Still - I am not very confident in
my reasoning above / I shouldn't ask readers to jump through mental hoops to see
what is ok and what isn't.  So, let me just add the include.

https://codereview.chromium.org/1417323006/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPageSerializer.h (right):

https://codereview.chromium.org/1417323006/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPageSerializer.h:107: // sequentially call
WebPageSeriazlierClient once the data buffer is full.
On 2015/12/04 21:52:39, ncarter wrote:
> "Seriazlier"

Done.

[dcheng, 2015-12-11 07:36:06]

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.cc (right):

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:174: std::string digest =
crypto::SHA256HashString(ipc_params.salt + uri.spec());
Maybe already_serialized_uris should just be pre-hashed, rather than repeating
this calculation on each SendToNextRenderFrame?

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:230: bool success = true;
Remove this.

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:236: success =
SendToNextRenderFrame();
And return this directly.

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:238: return success;
And return true here. It might be useful in the future, but it doesn't add much
now. I would personally invert this as well, so it's:

if (!HasMoreFramesToProcess())
  return true;
return SendToNextRenderFrame();

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

https://codereview.chromium.org/1417323006/diff/120001/content/common/frame_m...
content/common/frame_messages.h:438: using FrameRoutingIdToContentIdMap =
std::map<int, std::string>;
Do we still need to typedef this?

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

https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4685: if (success && IsMainFrame()) {
Revert this.

https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4685: if (success && IsMainFrame()) {
Please revert this =)

https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4698:
WebPageSerializer::enumerateMHTMLResources(GetWebFrame());
I think I would prefer to see a new WebPageSerializerClient callback rather than
enumerating all the resources, hashing to figure it out, and then re-iterate
over all the resources.

[Łukasz Anforowicz, 2015-12-14 19:34:32]

On 2015/12/11 07:36:06, dcheng wrote:
>
https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
> File content/browser/download/mhtml_generation_manager.cc (right):
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
> content/browser/download/mhtml_generation_manager.cc:174: std::string digest =
> crypto::SHA256HashString(ipc_params.salt + uri.spec());
> Maybe already_serialized_uris should just be pre-hashed, rather than repeating
> this calculation on each SendToNextRenderFrame?
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
> content/browser/download/mhtml_generation_manager.cc:230: bool success = true;
> Remove this.
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
> content/browser/download/mhtml_generation_manager.cc:236: success =
> SendToNextRenderFrame();
> And return this directly.
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
> content/browser/download/mhtml_generation_manager.cc:238: return success;
> And return true here. It might be useful in the future, but it doesn't add
much
> now. I would personally invert this as well, so it's:
> 
> if (!HasMoreFramesToProcess())
>   return true;
> return SendToNextRenderFrame();
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/common/frame_m...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/common/frame_m...
> content/common/frame_messages.h:438: using FrameRoutingIdToContentIdMap =
> std::map<int, std::string>;
> Do we still need to typedef this?
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:4685: if (success && IsMainFrame()) {
> Revert this.
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:4685: if (success && IsMainFrame()) {
> Please revert this =)
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:4698:
> WebPageSerializer::enumerateMHTMLResources(GetWebFrame());
> I think I would prefer to see a new WebPageSerializerClient callback rather
than
> enumerating all the resources, hashing to figure it out, and then re-iterate
> over all the resources.

Let me think a little bit more about this.

Some random notes:

- WebPageSerializerClient doesn't quite fit (it reports binary blobs for
Complete-Html case and this is something we don't need/do for MHTML case).

- PageSerializer::Delegate is hidden below Blink's public API layer

- std::function is on a yet-to-be-discussed list at
https://chromium-cpp.appspot.com/

- I am not quite happy with the serialize-twice approach;  it just seemed like
the simplest (and still quite clean I think) thing that gets the job done.

[Łukasz Anforowicz, 2015-12-14 19:39:02]

dcheng@, thanks for reviewing.  I've addressed a subset of your feedback, but
still need to think a little bit more about the way how uri enumerating and
skipping is implemented.

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.cc (right):

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:174: std::string digest =
crypto::SHA256HashString(ipc_params.salt + uri.spec());
On 2015/12/11 07:36:05, dcheng wrote:
> Maybe already_serialized_uris should just be pre-hashed, rather than repeating
> this calculation on each SendToNextRenderFrame?

Yes, good point.  Done.

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:230: bool success = true;
On 2015/12/11 07:36:05, dcheng wrote:
> Remove this.

Done.

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:236: success =
SendToNextRenderFrame();
On 2015/12/11 07:36:05, dcheng wrote:
> And return this directly.

Done.

https://codereview.chromium.org/1417323006/diff/120001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:238: return success;
On 2015/12/11 07:36:05, dcheng wrote:
> And return true here. It might be useful in the future, but it doesn't add
much
> now. I would personally invert this as well, so it's:
> 
> if (!HasMoreFramesToProcess())
>   return true;
> return SendToNextRenderFrame();

Yes - this is fair feedback.  It does read better this way.  Done.

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

https://codereview.chromium.org/1417323006/diff/120001/content/common/frame_m...
content/common/frame_messages.h:438: using FrameRoutingIdToContentIdMap =
std::map<int, std::string>;
On 2015/12/11 07:36:05, dcheng wrote:
> Do we still need to typedef this?

1. We do need a typedef - otherwise preprocessor will get confused - see
http://stackoverflow.com/questions/13842468/comma-in-c-c-macro

2. In the earlier CL (crrev.com/1386873003) I moved the typedef into a
singly-included section of this header file.

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

https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4685: if (success && IsMainFrame()) {
On 2015/12/11 07:36:05, dcheng wrote:
> Please revert this =)

Ooops.  Done (here and for the DCHECK below).

https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4698:
WebPageSerializer::enumerateMHTMLResources(GetWebFrame());
On 2015/12/11 07:36:05, dcheng wrote:
> I think I would prefer to see a new WebPageSerializerClient callback rather
than
> enumerating all the resources, hashing to figure it out, and then re-iterate
> over all the resources.

Let me think a little bit more about this.

Some random notes:

- WebPageSerializerClient doesn't quite fit (it reports binary blobs for
Complete-Html case and this is something we don't need/do for MHTML case).

- PageSerializer::Delegate is hidden below Blink's public API layer

- std::function is on a yet-to-be-discussed list at
https://chromium-cpp.appspot.com/

- I am not quite happy with the serialize-twice approach;  it just seemed like
the simplest (and still quite clean I think) thing that gets the job done.

[Łukasz Anforowicz, 2015-12-18 17:38:49]

Some more ideas about enumeration and handling of resources to be skipped by
serialization...

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

https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4698:
WebPageSerializer::enumerateMHTMLResources(GetWebFrame());
On 2015/12/14 19:39:02, Łukasz Anforowicz wrote:
> On 2015/12/11 07:36:05, dcheng wrote:
> > I think I would prefer to see a new WebPageSerializerClient callback rather
> than
> > enumerating all the resources, hashing to figure it out, and then re-iterate
> > over all the resources.
> 
> Let me think a little bit more about this.
> 
> Some random notes:
> 
> - WebPageSerializerClient doesn't quite fit (it reports binary blobs for
> Complete-Html case and this is something we don't need/do for MHTML case).
> 
> - PageSerializer::Delegate is hidden below Blink's public API layer
> 
> - std::function is on a yet-to-be-discussed list at
> https://chromium-cpp.appspot.com/
> 
> - I am not quite happy with the serialize-twice approach;  it just seemed like
> the simplest (and still quite clean I think) thing that gets the job done.

Some more ideas:

- Stupid idea: Pass a "callback" (bound to a "lambda" in render_frame_impl.cc)? 
"Lambda" in quotes, because lambdas are not allowed, but I can write a
lambda-like object (closure that closes over the set of digests to skip)
manually.  "Callback" in quotes, because AFAIK base::Callback and std::function
are not allowed in Blink... :-/

- Maybe a good idea: Have a boolean enumerateOnly flag that
WebPageSerializer::enumerateMHTMLResources passes to the constructor of
PageSerializer and which causes PageSerializer to populate the vector of
SerializedResource, but without populating SerializedResource::data. 

  - Pros:
    - The first traversal doesn't descend into subresources at all (only
enumerates their urls).

  - Cons:
    - Still 2 traversals of the frame's html dom.
    - Second traversal still needlessly serializes some resources that will be
skipped at WebPageSerializer::generateMHTMLParts level.  This can be solved by
passing the set-of-urls-to-skip from WebPageSerializer::generateMHTMLParts into
PageSerializer.

WDYT?  Should I try coding any of these?  Any other ideas that I missed?  Or is
the current code "not just good, but good enough!" :-) ?

[dcheng, 2015-12-18 17:46:05]

On 2015/12/18 at 17:38:49, lukasza wrote:
> Some more ideas about enumeration and handling of resources to be skipped by
serialization...
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:4698:
WebPageSerializer::enumerateMHTMLResources(GetWebFrame());
> On 2015/12/14 19:39:02, Łukasz Anforowicz wrote:
> > On 2015/12/11 07:36:05, dcheng wrote:
> > > I think I would prefer to see a new WebPageSerializerClient callback
rather
> > than
> > > enumerating all the resources, hashing to figure it out, and then
re-iterate
> > > over all the resources.
> > 
> > Let me think a little bit more about this.
> > 
> > Some random notes:
> > 
> > - WebPageSerializerClient doesn't quite fit (it reports binary blobs for
> > Complete-Html case and this is something we don't need/do for MHTML case).
> > 
> > - PageSerializer::Delegate is hidden below Blink's public API layer
> > 
> > - std::function is on a yet-to-be-discussed list at
> > https://chromium-cpp.appspot.com/
> > 
> > - I am not quite happy with the serialize-twice approach;  it just seemed
like
> > the simplest (and still quite clean I think) thing that gets the job done.
> 
> Some more ideas:
> 
> - Stupid idea: Pass a "callback" (bound to a "lambda" in
render_frame_impl.cc)?  "Lambda" in quotes, because lambdas are not allowed, but
I can write a lambda-like object (closure that closes over the set of digests to
skip) manually.  "Callback" in quotes, because AFAIK base::Callback and
std::function are not allowed in Blink... :-/
> 
> - Maybe a good idea: Have a boolean enumerateOnly flag that
WebPageSerializer::enumerateMHTMLResources passes to the constructor of
PageSerializer and which causes PageSerializer to populate the vector of
SerializedResource, but without populating SerializedResource::data. 
> 
>   - Pros:
>     - The first traversal doesn't descend into subresources at all (only
enumerates their urls).
> 
>   - Cons:
>     - Still 2 traversals of the frame's html dom.
>     - Second traversal still needlessly serializes some resources that will be
skipped at WebPageSerializer::generateMHTMLParts level.  This can be solved by
passing the set-of-urls-to-skip from WebPageSerializer::generateMHTMLParts into
PageSerializer.
> 
> WDYT?  Should I try coding any of these?  Any other ideas that I missed?  Or
is the current code "not just good, but good enough!" :-) ?

Conceptually, what's wrong with adding a method to WebPageSerializerClient to do
this policy check? It's not a whole lot of plumbing (we're already in the web
layer), and MHTML is a page serialization method, so it doesn't seem
unreasonable to have something there. Is the objection because it makes
WebPageSerializerClient a collection of methods that's not closely related?

[Łukasz Anforowicz, 2015-12-18 18:08:46]

On 2015/12/18 17:46:05, dcheng wrote:
> On 2015/12/18 at 17:38:49, lukasza wrote:
> > Some more ideas about enumeration and handling of resources to be skipped by
> serialization...
> > 
> >
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1417323006/diff/120001/content/renderer/rende...
> > content/renderer/render_frame_impl.cc:4698:
> WebPageSerializer::enumerateMHTMLResources(GetWebFrame());
> > On 2015/12/14 19:39:02, Łukasz Anforowicz wrote:
> > > On 2015/12/11 07:36:05, dcheng wrote:
> > > > I think I would prefer to see a new WebPageSerializerClient callback
> rather
> > > than
> > > > enumerating all the resources, hashing to figure it out, and then
> re-iterate
> > > > over all the resources.
> > > 
> > > Let me think a little bit more about this.
> > > 
> > > Some random notes:
> > > 
> > > - WebPageSerializerClient doesn't quite fit (it reports binary blobs for
> > > Complete-Html case and this is something we don't need/do for MHTML case).
> > > 
> > > - PageSerializer::Delegate is hidden below Blink's public API layer
> > > 
> > > - std::function is on a yet-to-be-discussed list at
> > > https://chromium-cpp.appspot.com/
> > > 
> > > - I am not quite happy with the serialize-twice approach;  it just seemed
> like
> > > the simplest (and still quite clean I think) thing that gets the job done.
> > 
> > Some more ideas:
> > 
> > - Stupid idea: Pass a "callback" (bound to a "lambda" in
> render_frame_impl.cc)?  "Lambda" in quotes, because lambdas are not allowed,
but
> I can write a lambda-like object (closure that closes over the set of digests
to
> skip) manually.  "Callback" in quotes, because AFAIK base::Callback and
> std::function are not allowed in Blink... :-/
> > 
> > - Maybe a good idea: Have a boolean enumerateOnly flag that
> WebPageSerializer::enumerateMHTMLResources passes to the constructor of
> PageSerializer and which causes PageSerializer to populate the vector of
> SerializedResource, but without populating SerializedResource::data. 
> > 
> >   - Pros:
> >     - The first traversal doesn't descend into subresources at all (only
> enumerates their urls).
> > 
> >   - Cons:
> >     - Still 2 traversals of the frame's html dom.
> >     - Second traversal still needlessly serializes some resources that will
be
> skipped at WebPageSerializer::generateMHTMLParts level.  This can be solved by
> passing the set-of-urls-to-skip from WebPageSerializer::generateMHTMLParts
into
> PageSerializer.
> > 
> > WDYT?  Should I try coding any of these?  Any other ideas that I missed?  Or
> is the current code "not just good, but good enough!" :-) ?
> 
> Conceptually, what's wrong with adding a method to WebPageSerializerClient to
do
> this policy check? It's not a whole lot of plumbing (we're already in the web
> layer), and MHTML is a page serialization method, so it doesn't seem
> unreasonable to have something there. Is the objection because it makes
> WebPageSerializerClient a collection of methods that's not closely related?

Correct - this is my main concern.

- The old WebPageSerializerClient::didSerializeDataForFrame would only make
sense
  when passing the interface to WebPageSerializer::serialize.
  If WebPageSerializerClient::generateMHTMLParts would take the same interface,
  then it would be given a wrong impression that it might report results via
  WebPageSerializerClient::didSerializeDataForFrame.

- Similarily, if we've added a WebPageSerializerClient::shouldSkipResource(const
URL&),
  then readers of WebPageSerializer::serialize would expect that it is capable
of filtering resources,
  when in fact the complete-html code path doesn't and wouldn't call
shouldSkipResource.


So - maybe we just need a new interface - let's say
WebPageSerializerResourceFilter?  Some questions:

- I guess for symmetry we want to put WebPageSerializerResourceFilter into a
separate header file under WebKit/public/web?  An alternative (my preference if
WebPageSerializerClient didn't exist :-) would be to make it a nested class of
WebPageSerializer (this makes it possible to have a shorter class name for the
interface + aids in readability by putting the interface declaration closer to
the method that actually uses the interface).

- I assume it is ok to refer to WebKit/public/web from
WebKit/Source/core/page/PageSerializer.cpp.  Is that right?


Also - note that I plan to rename [Web]PageSerializer to [Web]FrameSerializer in
a separate, low-priority CL.

[Łukasz Anforowicz, 2015-12-18 23:05:08]

Daniel, could you please take another look?

In the latest patchset (#10) I have introduced a MHTMLPartsGenerationDelegate
interface (nested under WebPageSerializer) with shouldSkipResource and
getContentID methods.  I have also added a shouldSkipResource method to
PageSerializer::Delegate.  This change helps the code in a few nice ways:

- We avoid a separate WebPageSerializer::enumerateResources method (helping code
readability and maintainability and runtime performance)

- Going through getContentID method in the delegate seems nicer - it allows to
avoid lots of boilerplate conversion code in-between content (std::map),
WebPageSerializer (WebVector<pair>), PageSerializer (wtf::HashMap).

- It turned out that skipping resources in PageSerializer is not that difficult
- all supported resource kinds already go through PageSerializer::shouldAddURL
method.

[dcheng, 2015-12-22 01:01:46]

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

https://codereview.chromium.org/1417323006/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:604: if (0 !=
params_.digests_of_uris_to_skip.count(digest))
ContainsKey? Or find()? Either seems to be more commonly used than count(). I
don't feel strongly about this, it's just a more unusual pattern.

https://codereview.chromium.org/1417323006/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:607:
uris_of_serialized_resources_->insert(url);
Any particular reason not to just send the hashes back and forth?

Right now, the browser process has to recalculate.

https://codereview.chromium.org/1417323006/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:622: };
Nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageSerializer.cpp:406: && (!delegate() ||
!delegate()->shouldSkipResource(url));
Do we ever construct this with a null delegate? Maybe it should just be a
reference too?

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageSerializer.h (right):

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageSerializer.h:72: // Default
implementation always returns false.
Just inline the default implementations.

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:76:
MHTMLPageSerializerDelegate(WebPageSerializer::MHTMLPartsGenerationDelegate&);
explicit

[Łukasz Anforowicz, 2015-12-22 21:06:47]

Daniel, can you take another look please?

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

https://codereview.chromium.org/1417323006/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:604: if (0 !=
params_.digests_of_uris_to_skip.count(digest))
On 2015/12/22 01:01:45, dcheng wrote:
> ContainsKey? Or find()? Either seems to be more commonly used than count(). I
> don't feel strongly about this, it's just a more unusual pattern.

Thanks for pointing out ContainsKey.  Done.

Side-comment: I am a bit surprised that ContainsKey is not in the global, rather
than the base namespace.

Also - I've also used ContainsKey in 2 places in save_package.cc

https://codereview.chromium.org/1417323006/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:607:
uris_of_serialized_resources_->insert(url);
On 2015/12/22 01:01:45, dcheng wrote:
> Any particular reason not to just send the hashes back and forth?
> 
> Right now, the browser process has to recalculate.

Done - I changed the code to send digests (instead of uris) in the IPC response.

BTW: In my previous iteration I changed code to avoid recalculating digests
(i.e. to only calculate the digest once) in the browser process.  I guess I
misunderstood the suggestion :-)

https://codereview.chromium.org/1417323006/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:622: };
On 2015/12/22 01:01:45, dcheng wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Oh, right.  Done.

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageSerializer.cpp:406: && (!delegate() ||
!delegate()->shouldSkipResource(url));
On 2015/12/22 01:01:45, dcheng wrote:
> Do we ever construct this with a null delegate? Maybe it should just be a
> reference too?

Good point.  Done.

I also made similar change for SerializerMarkupAccumulator::m_serializer
(replacing it with a Delegate& m_delegate + getting rid of
PageSerializer::delegate() method).

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageSerializer.h (right):

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageSerializer.h:72: // Default
implementation always returns false.
On 2015/12/22 01:01:45, dcheng wrote:
> Just inline the default implementations.

Done.

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/1417323006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:76:
MHTMLPageSerializerDelegate(WebPageSerializer::MHTMLPartsGenerationDelegate&);
On 2015/12/22 01:01:45, dcheng wrote:
> explicit

Oops.  Done.

[dcheng, 2015-12-29 18:37:27]

lgtm

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

https://codereview.chromium.org/1417323006/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.cc:610: return false;
An alternative is return
!digests_of_uris_of_serialized_resources_->insert(digest)->second;

Which avoids a tree lookup followed by a tree insert. Up to you if you feel this
is better / worse.

https://codereview.chromium.org/1417323006/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPageSerializer.h (right):

https://codereview.chromium.org/1417323006/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializer.h:88:
MHTMLPartsGenerationDelegate&);
I'd suggest using a pointer here: while this can "never be null", it's also in
the blink public API, and mutable references in this layer are pretty rare.

[Łukasz Anforowicz, 2015-12-29 19:32:56]

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

[Łukasz Anforowicz, 2015-12-29 19:32:57]

Thanks for reviewing.  rdsmith@, could you please take a look?

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

https://codereview.chromium.org/1417323006/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.cc:610: return false;
On 2015/12/29 18:37:27, dcheng wrote:
> An alternative is return
> !digests_of_uris_of_serialized_resources_->insert(digest)->second;
> 
> Which avoids a tree lookup followed by a tree insert. Up to you if you feel
this
> is better / worse.

I think that doing this would change the semantics - note that there are 2
separate sets here (set to skip passed from the browser and covering urls from
previous frames + set to report passed from the serializer and covering urls
from current frame).  I'll keep the current code as-is.

https://codereview.chromium.org/1417323006/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPageSerializer.h (right):

https://codereview.chromium.org/1417323006/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializer.h:88:
MHTMLPartsGenerationDelegate&);
On 2015/12/29 18:37:27, dcheng wrote:
> I'd suggest using a pointer here: while this can "never be null", it's also in
> the blink public API, and mutable references in this layer are pretty rare.

Done.

[Łukasz Anforowicz, 2015-12-29 19:46:47]

Description was changed from

==========
OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in
presence of out-of-process iframes, but has introduced a performance
regression by potentially including the same MHTML part twice.  This
regression is addressed by the current CL.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).

BUG=538766
==========

to

==========
OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in presence of
out-of-process iframes, but can potentially include the same MHTML part twice.
MHTML with duplicated parts can be rendered by Chrome without problems, but
the unnecessary increase of the file size is a performance regression.
Additionally MHTML with duplicated parts will not render correctly in Internet
Explorer.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).  This fixes the issues pointed
out above.

BUG=538766
==========

[Łukasz Anforowicz, 2015-12-29 20:10:34]

Description was changed from

==========
OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in presence of
out-of-process iframes, but can potentially include the same MHTML part twice.
MHTML with duplicated parts can be rendered by Chrome without problems, but
the unnecessary increase of the file size is a performance regression.
Additionally MHTML with duplicated parts will not render correctly in Internet
Explorer.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).  This fixes the issues pointed
out above.

BUG=538766
==========

to

==========
OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in
presence of out-of-process iframes, but can potentially include the same
MHTML part twice.  MHTML with duplicated parts can be rendered by Chrome
and Internet Explorer without problems (at least according to the
limited ad-hoc testing I've done), but the unnecessary increase of the
file size is a performance regression.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).  This fixes the issue pointed
out above.

BUG=538766
==========

[Randy Smith (Not in Mondays), 2015-12-30 00:45:49]

content/browser/download LGTM; below are suggestions for you to do with as you
wish.

https://codereview.chromium.org/1417323006/diff/240001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.cc (right):

https://codereview.chromium.org/1417323006/diff/240001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:316: }
nit, suggestion: The current implementation of OnSerializeAsMHTMLResponse
doesn't need to be called on the last response, but it strikes me as less future
proof; the name says that it'll be called on all responses, but it's actually
only called on all but the last one (IIUC).  Willing to restructure the code so
that it's always called and handles being the last frame correctly?

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

https://codereview.chromium.org/1417323006/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:609:
digests_of_uris_of_serialized_resources_->insert(digest);
No claim to understand the WebKit code, so this is a random question, but: Is
there any chance that the frame serialization could produce a dup from within
the frame?  I'm noticing that this code won't catch dups between digest &
digests_of_urls_of_serialized_resources_, just dups with
digests_or_uris_to_skip.

[Łukasz Anforowicz, 2015-12-30 19:09:09]

Thanks for reviewing.

https://codereview.chromium.org/1417323006/diff/240001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.cc (right):

https://codereview.chromium.org/1417323006/diff/240001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:316: }
On 2015/12/30 00:45:49, Randy Smith - OOO until M 1-4 wrote:
> nit, suggestion: The current implementation of OnSerializeAsMHTMLResponse
> doesn't need to be called on the last response, but it strikes me as less
future
> proof; the name says that it'll be called on all responses, but it's actually
> only called on all but the last one (IIUC).  Willing to restructure the code
so
> that it's always called and handles being the last frame correctly?

Done.

MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse already checks and
short-circuits HasMoreFramesToProcess.  OTOH, HasMoreFramesToProcess != IsDone. 
So, I've split these 2 methods.  This in turn forced me to think about tracking
pending responses - we can tighten IPC security by only accepting responses from
expected frames.

I've also added a new DCHECK to
MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse:

  // Renderer should be deduping resources with the same uris.
  DCHECK_EQ(0, STLSetIntersection<std::set<std::string>>(
      digests_of_already_serialized_uris_,
      digests_of_uris_of_serialized_resources).size());

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

https://codereview.chromium.org/1417323006/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:609:
digests_of_uris_of_serialized_resources_->insert(digest);
On 2015/12/30 00:45:49, Randy Smith - OOO until M 1-4 wrote:
> No claim to understand the WebKit code, so this is a random question, but: Is
> there any chance that the frame serialization could produce a dup from within
> the frame?  I'm noticing that this code won't catch dups between digest &
> digests_of_urls_of_serialized_resources_, just dups with
> digests_or_uris_to_skip.

Blink should take care of deduping within a single frame.  I've added comments
and a DCHECK to point this out.

I've also added some comments that I hope help clarify Daniel's earlier
confusion about distinction between digests_of_uris_to_skip and
digests_of_uris_of_serialized_resources_.

Done.

[nasko, 2015-12-30 19:22:25]

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

[nasko, 2015-12-30 19:22:26]

Just a couple of drive-by nits.

https://codereview.chromium.org/1417323006/diff/260001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.cc (right):

https://codereview.chromium.org/1417323006/diff/260001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:58: bool done =
!waiting_for_response_from_renderer && no_more_requests_to_send;
Why not "return !b1 && b2;"?

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

https://codereview.chromium.org/1417323006/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:612: bool insertion_took_place =
pair.second;
nit: I'd skip this local variable and DCHECK the pair.second. We already use
this pattern in the constructor.

https://codereview.chromium.org/1417323006/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:614: // Blink should take care of deduping
within a single (current) frame.
nit: No need for "single (current)" part.

[Łukasz Anforowicz, 2015-12-30 19:47:10]

Thanks for reviewing.

https://codereview.chromium.org/1417323006/diff/260001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.cc (right):

https://codereview.chromium.org/1417323006/diff/260001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:58: bool done =
!waiting_for_response_from_renderer && no_more_requests_to_send;
On 2015/12/30 19:22:26, nasko wrote:
> Why not "return !b1 && b2;"?

Done.

RE: Why:

Desire to spell "done" one more time and hoping that this will improve
readability?

https://codereview.chromium.org/1417323006/diff/260001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:87: // The IDs of frames we
still need to process.
I tackled one of "we"-s in the comments, but didn't see an easy way out for the
remaining 2 occurrences below.  OTOH, this probably isn't a big enough deal to
precent CQ-ing this CL :-)

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

https://codereview.chromium.org/1417323006/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:612: bool insertion_took_place =
pair.second;
On 2015/12/30 19:22:26, nasko wrote:
> nit: I'd skip this local variable and DCHECK the pair.second. We already use
> this pattern in the constructor.

I don't know.  I always have to look-up std lib's docs to see what is pair.first
vs pair.second in this case + whether pair.second is insertion_took_place or
there_was_a_conflict.  Having an explicit name in the code makes it easier for
me to read and understand this code without having to look up the docs...

Leaving it as-is for now.

https://codereview.chromium.org/1417323006/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:614: // Blink should take care of deduping
within a single (current) frame.
On 2015/12/30 19:22:26, nasko wrote:
> nit: No need for "single (current)" part.

Done.

https://codereview.chromium.org/1417323006/diff/280001/content/browser/downlo...
File content/browser/download/mhtml_generation_manager.h (right):

https://codereview.chromium.org/1417323006/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.h:47: RenderFrameHostImpl*
sender,
Passing |RenderFrameHostImpl* sender| instead of |int
frame_tree_node_id_of_sender| to
MHTMLGenerationManager::OnSerializeAsMHTMLResponse.

[Łukasz Anforowicz, 2015-12-30 19:47:20]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-12-30 19:47:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
dcheng@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1417323006/#ps280001
(title: "Addressed CR feedback from nasko@.")

[commit-bot: I haz the power, 2015-12-30 19:47:55]

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

[commit-bot: I haz the power, 2015-12-30 19:50:12]

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

[commit-bot: I haz the power, 2015-12-30 19:50:21]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2015-12-30 20:16:54]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-12-30 20:16:54]

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

[commit-bot: I haz the power, 2015-12-30 20:17:20]

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

[commit-bot: I haz the power, 2015-12-30 23:48:39]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-12-30 23:49:52]

Description was changed from

==========
OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in
presence of out-of-process iframes, but can potentially include the same
MHTML part twice.  MHTML with duplicated parts can be rendered by Chrome
and Internet Explorer without problems (at least according to the
limited ad-hoc testing I've done), but the unnecessary increase of the
file size is a performance regression.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).  This fixes the issue pointed
out above.

BUG=538766
==========

to

==========
OOPIFs: Deduplicating MHTML parts across frames.

Previous CL (crrev.com/1386873003) made MHTML serialization work in
presence of out-of-process iframes, but can potentially include the same
MHTML part twice.  MHTML with duplicated parts can be rendered by Chrome
and Internet Explorer without problems (at least according to the
limited ad-hoc testing I've done), but the unnecessary increase of the
file size is a performance regression.

After the current CL, the browser keeps track of already serialized
MHTML parts and communicates them to the renderer/serializer for
each frame (so they can be deduped).  This fixes the issue pointed
out above.

BUG=538766

Committed: https://crrev.com/ede9cc06d2550f0ee0dce0285cc88c872809c1bb
Cr-Commit-Position: refs/heads/master@{#367206}
==========

[commit-bot: I haz the power, 2015-12-30 23:49:53]

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