OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.

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

[Łukasz Anforowicz, 2015-10-06 20:52:24]

Patchset #1 (id:1) has been deleted

[Łukasz Anforowicz, 2015-12-02 21:01:46]

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

[Łukasz Anforowicz, 2015-12-02 21:01:47]

nick@, could you please take a look?

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

Description was changed from

==========
OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.


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

to

==========
OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.


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

[ncarter (slow), 2015-12-04 22:54:48]

one possible simplification (which is optional), but looks good otherwise

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

https://codereview.chromium.org/1386873003/diff/160001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:102:
base::Unretained(this)));  // Safe because ForEachFrame is synchronus.
"synchronus" -> "synchronous"

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

https://codereview.chromium.org/1386873003/diff/160001/content/common/frame_m...
content/common/frame_messages.h:731: // FrameHostMsg_SerializeAsMHTMLResponse.
It seems like the back-and-forth for boundary-marker passing is a little more
complicated than we might actually need: from the code it looks like we just
need to use a consistent boundary marker across all frames, right?

Would it be better to generate the boundary marker (or ten bytes from
base::RandBytes() that could be turned into a boundary marker) in the browser
process MHTMLGenerationManager, and pass it to all frames? Or is that more
trouble than it's worth?

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

https://codereview.chromium.org/1386873003/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializer.h:51: // TODO(lukasza): Rename
this class to WebFrameSerializer?
This rename sounds like a good idea.

[Łukasz Anforowicz, 2015-12-05 00:18:39]

nick@, can you take another look please?

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

https://codereview.chromium.org/1386873003/diff/160001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:102:
base::Unretained(this)));  // Safe because ForEachFrame is synchronus.
On 2015/12/04 22:54:47, ncarter wrote:
> "synchronus" -> "synchronous"

Done.

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

https://codereview.chromium.org/1386873003/diff/160001/content/common/frame_m...
content/common/frame_messages.h:731: // FrameHostMsg_SerializeAsMHTMLResponse.
On 2015/12/04 22:54:47, ncarter wrote:
> It seems like the back-and-forth for boundary-marker passing is a little more
> complicated than we might actually need: from the code it looks like we just
> need to use a consistent boundary marker across all frames, right?
> 
> Would it be better to generate the boundary marker (or ten bytes from
> base::RandBytes() that could be turned into a boundary marker) in the browser
> process MHTMLGenerationManager, and pass it to all frames? Or is that more
> trouble than it's worth?

Thanks for bringing that up.  I am not quite happy with what I have here now. 
Initially I was simply hesitant to break-up the boundary generation code from
the rest of MHTML serialization/generation code in MHTMLArchive, but after
reading the specs I see that this is supposed to be a generic mime multipart
boundary and I don't see a problem with having such generic code in the browser,
far away from the MHTMLArchive code.

For now I'll have an ad-hoc code for generating the boundary inside
MHTMLGenerationManager.  I'll try to add a shared/generic utility function later
into net/base/mime_util.h.

Interestingly there is a restricted set of characters that can go here (guid
characters seem ok) - there is BNF in section "7.2.1 Multipart: The common
syntax" of rfc1341.  And I probably want to preserve some properties of what
MHTMLParser does (i.e. leading "----" substring) although this is silly CYA
approach that I am a bit ashamed of... (otoh, I don't entirely disagree with not
changing "it" if it ain't broken...).

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

https://codereview.chromium.org/1386873003/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializer.h:51: // TODO(lukasza): Rename
this class to WebFrameSerializer?
On 2015/12/04 22:54:48, ncarter wrote:
> This rename sounds like a good idea.

Acknowledged.

[ncarter (slow), 2015-12-07 18:39:57]

lgtm

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

https://codereview.chromium.org/1386873003/diff/160001/content/common/frame_m...
content/common/frame_messages.h:731: // FrameHostMsg_SerializeAsMHTMLResponse.
On 2015/12/05 00:18:39, Łukasz Anforowicz wrote:
> On 2015/12/04 22:54:47, ncarter wrote:
> > It seems like the back-and-forth for boundary-marker passing is a little
more
> > complicated than we might actually need: from the code it looks like we just
> > need to use a consistent boundary marker across all frames, right?
> > 
> > Would it be better to generate the boundary marker (or ten bytes from
> > base::RandBytes() that could be turned into a boundary marker) in the
browser
> > process MHTMLGenerationManager, and pass it to all frames? Or is that more
> > trouble than it's worth?
> 
> Thanks for bringing that up.  I am not quite happy with what I have here now. 
> Initially I was simply hesitant to break-up the boundary generation code from
> the rest of MHTML serialization/generation code in MHTMLArchive, but after
> reading the specs I see that this is supposed to be a generic mime multipart
> boundary and I don't see a problem with having such generic code in the
browser,
> far away from the MHTMLArchive code.
> 
> For now I'll have an ad-hoc code for generating the boundary inside
> MHTMLGenerationManager.  I'll try to add a shared/generic utility function
later
> into net/base/mime_util.h.
> 
> Interestingly there is a restricted set of characters that can go here (guid
> characters seem ok) - there is BNF in section "7.2.1 Multipart: The common
> syntax" of rfc1341.  And I probably want to preserve some properties of what
> MHTMLParser does (i.e. leading "----" substring) although this is silly CYA
> approach that I am a bit ashamed of... (otoh, I don't entirely disagree with
not
> changing "it" if it ain't broken...).

Yeah, my instinct is also to apply the "if it ain't broken" logic here.

[Łukasz Anforowicz, 2015-12-07 19:23:38]

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

[Łukasz Anforowicz, 2015-12-07 19:23:39]

dcheng@, could you please take a look?  (this is probably lower priority that
the CLs related to the site-isolation finch trial)

[dcheng, 2015-12-08 06:22:27]

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

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:109:
DCHECK(FrameTreeNode::GloballyFindByID(pending_frame_tree_node_ids_.front())
DCHECK_EQ works here

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:179: content_id_builder <<
"<frame" << frame_tree_node_id << "@save"
I was going to say that streams aren't permitted in Google style, but it seems
like that restriction has been relaxed.

That being said, I think using a stream typically results in larger generated
code, and it seems like most usages of streams in Chrome are still related to
logging. So maybe we should preserve the status quo? Switching this one to
base::StringPrintf() is pretty easy anyway.

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:234: ss << std::hex <<
static_cast<uint32_t>(random_values[i]);
https://google.github.io/styleguide/cppguide.html#Streams

If you do use streams, avoid the stateful parts of the streams API (other than
error state), such as imbue(), xalloc(), and register_callback(). Use explicit
formatting functions rather than stream manipulators or formatting flags to
control formatting details such as number base, precision, or padding.

This one is almost certainly easier to read as StringPrintf. There are some
helpers in
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
for hex<->string conversions.

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

https://codereview.chromium.org/1386873003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4734: if (success && IsMainFrame()) {
Isn't success always true here?

https://codereview.chromium.org/1386873003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4741: DCHECK(!mhtml_boundary.isEmpty());
Seems like this should probably go with where it's unpacked, for clarity
(otherwise, it looks like generateMHTMLHeader could mutate it). Also make it
const for good measure?

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

https://codereview.chromium.org/1386873003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:152: result.add(frame,
contentID);
Replace the ternary statement with webFrame->toImplBase()->frame()

https://codereview.chromium.org/1386873003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:168: return
PassRefPtr<SharedBuffer>(buffer);
Does buffer.release() work?

https://codereview.chromium.org/1386873003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:209: return
PassRefPtr<SharedBuffer>(buffer);
Ditto: does buffer.release() work?

[ncarter (slow), 2015-12-08 19:31:01]

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

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:234: ss << std::hex <<
static_cast<uint32_t>(random_values[i]);
On 2015/12/08 06:22:27, dcheng wrote:
> https://google.github.io/styleguide/cppguide.html#Streams
> 
> If you do use streams, avoid the stateful parts of the streams API (other than
> error state), such as imbue(), xalloc(), and register_callback(). Use explicit
> formatting functions rather than stream manipulators or formatting flags to
> control formatting details such as number base, precision, or padding.
> 
> This one is almost certainly easier to read as StringPrintf. There are some
> helpers in
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> for hex<->string conversions.

Agree with Daniel here. I'd use StringPrintf (which is more readable) or
string's operator+= (robust against \0's, though that's not an issue here).

[Łukasz Anforowicz, 2015-12-08 21:26:43]

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

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

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:109:
DCHECK(FrameTreeNode::GloballyFindByID(pending_frame_tree_node_ids_.front())
On 2015/12/08 06:22:27, dcheng wrote:
> DCHECK_EQ works here

I've tried this:
109   DCHECK_EQ(nullptr, FrameTreeNode::GloballyFindByID(
110                          pending_frame_tree_node_ids_.front())
111                          ->parent());
and it doesn't compile:
./../base/logging.h:523:23: error: use of overloaded operator '<<' is ambiguous
(with 
operand types 'basic_ostream<char, std::char_traits<char> >' and 'const
nullptr_t')
Candidate functions in include/c++/4.6/ostream included a void*, wchar_t* and
some other overloads.

So I think I'll just stick with the original.

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:179: content_id_builder <<
"<frame" << frame_tree_node_id << "@save"
On 2015/12/08 06:22:27, dcheng wrote:
> I was going to say that streams aren't permitted in Google style, but it seems
> like that restriction has been relaxed.
> 
> That being said, I think using a stream typically results in larger generated
> code, and it seems like most usages of streams in Chrome are still related to
> logging. So maybe we should preserve the status quo? Switching this one to
> base::StringPrintf() is pretty easy anyway.

Yeah, I guess StringPrintf will be clearer indeed.

I guess I try too hard to do thing via "standard C++ way" (sometimes overlooking
simple solutions like string's += vs ostringstream), rather than looking for the
"standard Chromium way".  I'll try to repent... :-)  In the latest patches I've
even tried to improve by reusing a whitespace collapsing function from base in
save_page_browsertest.cc :-)

And I guess .NET has tainted my mind with "strings are immutable and inefficient
to add or append to;  you need StringBuilders" and I keep forgetting that
std::string's += is effectively a nice string builder.

https://codereview.chromium.org/1386873003/diff/200001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:234: ss << std::hex <<
static_cast<uint32_t>(random_values[i]);
On 2015/12/08 19:31:01, ncarter wrote:
> On 2015/12/08 06:22:27, dcheng wrote:
> > https://google.github.io/styleguide/cppguide.html#Streams
> > 
> > If you do use streams, avoid the stateful parts of the streams API (other
than
> > error state), such as imbue(), xalloc(), and register_callback(). Use
explicit
> > formatting functions rather than stream manipulators or formatting flags to
> > control formatting details such as number base, precision, or padding.
> > 
> > This one is almost certainly easier to read as StringPrintf. There are some
> > helpers in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> > for hex<->string conversions.
> 
> Agree with Daniel here. I'd use StringPrintf (which is more readable) or
> string's operator+= (robust against \0's, though that's not an issue here).

Done.

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

https://codereview.chromium.org/1386873003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4734: if (success && IsMainFrame()) {
On 2015/12/08 06:22:27, dcheng wrote:
> Isn't success always true here?

Yes - it is always true.  I've written this way for symmetry between the 3
cases.  I can remove it.

https://codereview.chromium.org/1386873003/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4741: DCHECK(!mhtml_boundary.isEmpty());
On 2015/12/08 06:22:27, dcheng wrote:
> Seems like this should probably go with where it's unpacked, for clarity
> (otherwise, it looks like generateMHTMLHeader could mutate it). Also make it
> const for good measure?

Good point.  Done.

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

https://codereview.chromium.org/1386873003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:152: result.add(frame,
contentID);
On 2015/12/08 06:22:27, dcheng wrote:
> Replace the ternary statement with webFrame->toImplBase()->frame()

Nice.  Done.

https://codereview.chromium.org/1386873003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:168: return
PassRefPtr<SharedBuffer>(buffer);
On 2015/12/08 06:22:27, dcheng wrote:
> Does buffer.release() work?

Hmmm... I see that I've (inconsistently) used output.release() below and it
works.  I don't remember why I used the PassRefPtr constructor;  maybe I thought
that RefPtr.release() returns a raw pointer rather than PassRefPtr as it really
does.

Done.

https://codereview.chromium.org/1386873003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:209: return
PassRefPtr<SharedBuffer>(buffer);
On 2015/12/08 06:22:27, dcheng wrote:
> Ditto: does buffer.release() work?

Done.

[dcheng, 2015-12-09 08:02:37]

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

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:45: bool
GotMoreFramesToProcess() {
Nit: HasMoreFramesToProcess() const?

Got sounds like the code got something in response to trying to get something
(if that makes any sense).

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:217: std::string
MHTMLGenerationManager::Job::GenerateMhtmlBoundaryMarker() {
I think you should stick either with MHTML or Mhtml, but not both in the same
line.

(I know Mhtml is now the mandated style, but there's something to be said about
consistency)

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:224: uint8_t
random_values[kRandomValuesLength];
Just put 10 in here.

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:225:
base::RandBytes(&random_values, kRandomValuesLength);
And then use sizeof() here.

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

https://codereview.chromium.org/1386873003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:158: const WebString&
boundary, WebLocalFrame* topLevelFrame)
And just call this frame to match generateMHTMLParts.

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

https://codereview.chromium.org/1386873003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializer.h:63: const WebString& boundary,
WebLocalFrame* topLevelFrame);
Omit topLevelFrame here: I mostly find it confusing to mention it here.

[Łukasz Anforowicz, 2015-12-09 17:33:57]

dcheng@, could you please take another look?

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

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:45: bool
GotMoreFramesToProcess() {
On 2015/12/09 08:02:37, dcheng wrote:
> Nit: HasMoreFramesToProcess() const?
> 
> Got sounds like the code got something in response to trying to get something
> (if that makes any sense).

Makes sense.  Done.

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:217: std::string
MHTMLGenerationManager::Job::GenerateMhtmlBoundaryMarker() {
On 2015/12/09 08:02:37, dcheng wrote:
> I think you should stick either with MHTML or Mhtml, but not both in the same
> line.
> 
> (I know Mhtml is now the mandated style, but there's something to be said
about
> consistency)

Done.

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:224: uint8_t
random_values[kRandomValuesLength];
On 2015/12/09 08:02:37, dcheng wrote:
> Just put 10 in here.

Done.

https://codereview.chromium.org/1386873003/diff/220001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:225:
base::RandBytes(&random_values, kRandomValuesLength);
On 2015/12/09 08:02:37, dcheng wrote:
> And then use sizeof() here.

Done.

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

https://codereview.chromium.org/1386873003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPageSerializer.cpp:158: const WebString&
boundary, WebLocalFrame* topLevelFrame)
On 2015/12/09 08:02:37, dcheng wrote:
> And just call this frame to match generateMHTMLParts.

Done.

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

https://codereview.chromium.org/1386873003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPageSerializer.h:63: const WebString& boundary,
WebLocalFrame* topLevelFrame);
On 2015/12/09 08:02:37, dcheng wrote:
> Omit topLevelFrame here: I mostly find it confusing to mention it here.

Done.  OTOH, I've expanded the doc comment to explain that this typically should
be the main, top-level frame.  I hope this is ok.

When editing the comment I've also realized that I didn't remove remaining
references to generateMHTMLBoundary after removing this method.  So I've done
this as well.

[dcheng, 2015-12-09 18:12:38]

lgtm

[Łukasz Anforowicz, 2015-12-09 18:18:57]

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

[Łukasz Anforowicz, 2015-12-09 18:18:58]

rdsmith@, could you please take a look?

[Łukasz Anforowicz, 2015-12-10 01:09:04]

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

[Łukasz Anforowicz, 2015-12-10 01:09:05]

tkent@, could you please take a look (mostly at
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.* and
third_party/WebKit/Source/web/tests/MHTMLTest.cpp changes - I've moved the
generation of MHTML boundary [which is really a generic MIME multipart boundary]
from blink/MHTMLArchive into
content/browser/.../content/browser/download/mhtml_generation_manager.cc [and in
the future possibly into net/base/mime_util.h])

[tkent, 2015-12-10 02:21:18]

lgtm

[Randy Smith (Not in Mondays), 2015-12-10 23:07:56]

content/browser, content/common looks basically good to me; nits, questions, and
suggestions below.

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:65: // a
|site_instance|-specific, routing-id-based map.
nit: My impression from reading the code is the primary purpose of this function
is to filter frame_tree_node_to_content_id_ down to just the frames relevant to
a site instance, and then secondarily making it a routing-id based map instead
of a FTNID based map.  If you agree, would you be willing to call out the
filtering functionality in the comment?

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:120: SiteInstance*
site_instance) {
nit, suggestion: The only place in this file that appears to use SiteInstance is
in the interface to this function; elsewhere it's RenderFrameHost & similar at
that level.  Reasonable to make this take a RFH instead of a SiteInstance and
hide the need to access the site instance behind this interface?

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:175: auto* rfhi =
static_cast<RenderFrameHostImpl*>(render_frame_host);
I just want to confirm that someone familiar with the RenderFrameHost class
structure in content signed off on this code?  This makes me nervous, but it may
well be perfectly fine given content/ idioms.

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:217: std::string
MHTMLGenerationManager::Job::GenerateMHTMLBoundaryMarker() {
Do you need me to learn enough about MIME/MHTML to properly review this
function, or can I assume one of the other reviewers did so?

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:260: void
MHTMLGenerationManager::OnSavedPageAsMHTML(
Hmmm.  Given this change, I'm uncomfortable with this function name.  It's
really "OnSavedFrameAsMHTML" now; reasonable to change the name?

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:294: uint32 file_flags =
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
Can the renderer seek in the file?  If it can, that means it could overwrite
things written by other frames.  This is probably ok--I don't think revisiting
the MHTML would re-give those frames privileges to that origin--but I wanted to
raise the issue in case I was wrong.

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.h:9: #include <string>
Ok, I'll bite.  I don't see any use of std::string in the header file; why the
inclusion?

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

https://codereview.chromium.org/1386873003/diff/280001/content/common/frame_m...
content/common/frame_messages.h:740: // emit the MHTML footer.
nit, suggestion/question: Why not emit the header/footer from the browser
process and keep the IPC interface simpler?

[lukasza, 2015-12-11 00:29:02]

lukasza@google.com changed reviewers:
+ lukasza@google.com

[lukasza, 2015-12-11 00:29:03]

Randy, can you take another look please?

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:65: // a
|site_instance|-specific, routing-id-based map.
On 2015/12/10 23:07:55, rdsmith wrote:
> nit: My impression from reading the code is the primary purpose of this
function
> is to filter frame_tree_node_to_content_id_ down to just the frames relevant
to
> a site instance, and then secondarily making it a routing-id based map instead
> of a FTNID based map.  If you agree, would you be willing to call out the
> filtering functionality in the comment? 

I've tried rewording the comment to make it clearer what is happening:

  // Creates a new map with values (content ids) the same as in
  // |frame_tree_node_to_content_id_| map, but with the keys translated from
  // frame_tree_node_id into a |site_instance|-specific routing_id.

Let me try to explain further below, and if you think this is important, I can
add that to the comment (I didn't add it for now, because it drowns the main
message of the comment in technical details that are borderline implementation
details):

  // frame_tree_node_id uniquely identifies a frame.  The frame might need to
  // be represented in multiple renderer processes (as either [local]
  // RenderFrame or [remote] RenderFrameProxy) and routing ids only make
  // sense only in the context of a process / belong to process-specific
  // RenderFrame/RenderFrameProxy objects.  We can get the right routing_id,
  // by looking at a frame through the lenses/perspective of a |site_instance|.

See also my reply to the next comment.

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:120: SiteInstance*
site_instance) {
On 2015/12/10 23:07:55, rdsmith wrote:
> nit, suggestion: The only place in this file that appears to use SiteInstance
is
> in the interface to this function; elsewhere it's RenderFrameHost & similar at
> that level.  Reasonable to make this take a RFH instead of a SiteInstance and
> hide the need to access the site instance behind this interface?

There are 2 frames: frame_A and frame_B.  We want to get routing_id of frame_B
that will work in the process that will handle messages for frame_A (process
where frame_A is a local frame).  I feel it is ok to say "give me routing id of
frame B in the context of site instance foo.com", but it feels like a layering
violation to say "give me routing id of frame B in the context of frame A",
because it really means "give me routing id of frame B in the context of site
instance where frame A is a local frame" (i.e. I don't think "routing id in the
context of frame A" alone, has a clear, specific meaning).

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:175: auto* rfhi =
static_cast<RenderFrameHostImpl*>(render_frame_host);
On 2015/12/10 23:07:55, rdsmith wrote:
> I just want to confirm that someone familiar with the RenderFrameHost class
> structure in content signed off on this code?  This makes me nervous, but it
may
> well be perfectly fine given content/ idioms.

Yup - that would be nick@.  AFAIK the cast is ok within //content/browser

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:217: std::string
MHTMLGenerationManager::Job::GenerateMHTMLBoundaryMarker() {
On 2015/12/10 23:07:55, rdsmith wrote:
> Do you need me to learn enough about MIME/MHTML to properly review this
> function, or can I assume one of the other reviewers did so?

I think this is fine as-is:
1. I copied it from third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp
2. It seems that the MIME specs/rfcs don't really put any restrictions on the
string used as a mime multipart boundary (other than having 2 dashes in the
front + restricting the characterset somewhat).

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:260: void
MHTMLGenerationManager::OnSavedPageAsMHTML(
On 2015/12/10 23:07:55, rdsmith wrote:
> Hmmm.  Given this change, I'm uncomfortable with this function name.  It's
> really "OnSavedFrameAsMHTML" now; reasonable to change the name?

Good catch.  Yes - it should be "frame", not "page" (and I also have a todo to
rename WebPageSerializer and PageSerializer to "frame"-named-equivalents).

Done.  (there was also a stale comment with page-related name of an IPC message)

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:294: uint32 file_flags =
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
On 2015/12/10 23:07:55, rdsmith wrote:
> Can the renderer seek in the file?  If it can, that means it could overwrite
> things written by other frames.  This is probably ok--I don't think revisiting
> the MHTML would re-give those frames privileges to that origin--but I wanted
to
> raise the issue in case I was wrong.

Thank you for catching this.  It does seem that base::File::FLAG_APPEND is
better because on POSIX it gets translated to O_APPEND | O_WRONLY and for
O_APPEND docs say "if set, the file offset shall be set to the end of the file
prior to each write".  Note that FLAG_WRITE and FLAG_APPEND have to be used
together (otherwise we hit a DCHECK in file_posix.cc(438) - check failed: flags
& FLAG_WRITE.).

Done.  OTOH, note that I still need to retest on Windows... and I don't really
understand what file_posix.cc and file_win.cc try to do wrt requiring (posix) or
disallowing (win?) write+append together.  Maybe I'll have to revert this
particular change if it blows up on debug flavor of a windows build.

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.h:9: #include <string>
On 2015/12/10 23:07:55, rdsmith wrote:
> Ok, I'll bite.  I don't see any use of std::string in the header file; why the
> inclusion?

I don't know how that happened :-/  
Removed / done.

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

https://codereview.chromium.org/1386873003/diff/280001/content/common/frame_m...
content/common/frame_messages.h:740: // emit the MHTML footer.
On 2015/12/10 23:07:55, rdsmith wrote:
> nit, suggestion/question: Why not emit the header/footer from the browser
> process and keep the IPC interface simpler?

I wasn't sure about that, but it felt better to keep all the MHTML
parsing/generation code (and test code) close together in Blink.

[Łukasz Anforowicz, 2015-12-11 01:00:43]

lukasza@chromium.org changed reviewers:
- lukasza@google.com

[ncarter (slow), 2015-12-11 17:21:42]

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

https://codereview.chromium.org/1386873003/diff/280001/content/common/frame_m...
content/common/frame_messages.h:740: // emit the MHTML footer.
On 2015/12/11 00:29:03, lukasza wrote:
> On 2015/12/10 23:07:55, rdsmith wrote:
> > nit, suggestion/question: Why not emit the header/footer from the browser
> > process and keep the IPC interface simpler?
> 
> I wasn't sure about that, but it felt better to keep all the MHTML
> parsing/generation code (and test code) close together in Blink.

Also, you can't do file IO on the UI thread where the coordination code lives,
so you'd have to farm it off on the file thread or the blocking pool, and that's
more complicated than just getting the child process to do it.

[Randy Smith (Not in Mondays), 2015-12-14 02:47:04]

LGTM; thanks!

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:65: // a
|site_instance|-specific, routing-id-based map.
On 2015/12/11 00:29:03, lukasza wrote:
> On 2015/12/10 23:07:55, rdsmith wrote:
> > nit: My impression from reading the code is the primary purpose of this
> function
> > is to filter frame_tree_node_to_content_id_ down to just the frames relevant
> to
> > a site instance, and then secondarily making it a routing-id based map
instead
> > of a FTNID based map.  If you agree, would you be willing to call out the
> > filtering functionality in the comment? 
> 
> I've tried rewording the comment to make it clearer what is happening:
> 
>   // Creates a new map with values (content ids) the same as in
>   // |frame_tree_node_to_content_id_| map, but with the keys translated from
>   // frame_tree_node_id into a |site_instance|-specific routing_id.
> 
> Let me try to explain further below, and if you think this is important, I can
> add that to the comment (I didn't add it for now, because it drowns the main
> message of the comment in technical details that are borderline implementation
> details):
> 
>   // frame_tree_node_id uniquely identifies a frame.  The frame might need to
>   // be represented in multiple renderer processes (as either [local]
>   // RenderFrame or [remote] RenderFrameProxy) and routing ids only make
>   // sense only in the context of a process / belong to process-specific
>   // RenderFrame/RenderFrameProxy objects.  We can get the right routing_id,
>   // by looking at a frame through the lenses/perspective of a
|site_instance|.
> 
> See also my reply to the next comment.

Yeah, I think I was mis-understanding the functionality.  Sounds good.

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:120: SiteInstance*
site_instance) {
On 2015/12/11 00:29:03, lukasza wrote:
> On 2015/12/10 23:07:55, rdsmith wrote:
> > nit, suggestion: The only place in this file that appears to use
SiteInstance
> is
> > in the interface to this function; elsewhere it's RenderFrameHost & similar
at
> > that level.  Reasonable to make this take a RFH instead of a SiteInstance
and
> > hide the need to access the site instance behind this interface?
> 
> There are 2 frames: frame_A and frame_B.  We want to get routing_id of frame_B
> that will work in the process that will handle messages for frame_A (process
> where frame_A is a local frame).  I feel it is ok to say "give me routing id
of
> frame B in the context of site instance foo.com", but it feels like a layering
> violation to say "give me routing id of frame B in the context of frame A",
> because it really means "give me routing id of frame B in the context of site
> instance where frame A is a local frame" (i.e. I don't think "routing id in
the
> context of frame A" alone, has a clear, specific meaning).

Yeah, I think I didn't have a full appreciation of what this function was doing.
 I'm still not completely sure I do, but given what you're saying the name &
arguments make sense.

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:294: uint32 file_flags =
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
On 2015/12/11 00:29:02, lukasza wrote:
> On 2015/12/10 23:07:55, rdsmith wrote:
> > Can the renderer seek in the file?  If it can, that means it could overwrite
> > things written by other frames.  This is probably ok--I don't think
revisiting
> > the MHTML would re-give those frames privileges to that origin--but I wanted
> to
> > raise the issue in case I was wrong.
> 
> Thank you for catching this.  It does seem that base::File::FLAG_APPEND is
> better because on POSIX it gets translated to O_APPEND | O_WRONLY and for
> O_APPEND docs say "if set, the file offset shall be set to the end of the file
> prior to each write".  Note that FLAG_WRITE and FLAG_APPEND have to be used
> together (otherwise we hit a DCHECK in file_posix.cc(438) - check failed:
flags
> & FLAG_WRITE.).
> 
> Done.  OTOH, note that I still need to retest on Windows... and I don't really
> understand what file_posix.cc and file_win.cc try to do wrt requiring (posix)
or
> disallowing (win?) write+append together.  Maybe I'll have to revert this
> particular change if it blows up on debug flavor of a windows build.

Sounds good; I'm happy to leave that to you.  I don't think this is a security
issue as there are almost certainly more interesting things that a malicious
renderer could do with an MHTML output request, but if O_APPEND | O_WRONLY can
be made to work, that's probably better.

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

https://codereview.chromium.org/1386873003/diff/280001/content/common/frame_m...
content/common/frame_messages.h:740: // emit the MHTML footer.
On 2015/12/11 00:29:03, lukasza wrote:
> On 2015/12/10 23:07:55, rdsmith wrote:
> > nit, suggestion/question: Why not emit the header/footer from the browser
> > process and keep the IPC interface simpler?
> 
> I wasn't sure about that, but it felt better to keep all the MHTML
> parsing/generation code (and test code) close together in Blink.

Acknowledged.

https://codereview.chromium.org/1386873003/diff/280001/content/common/frame_m...
content/common/frame_messages.h:740: // emit the MHTML footer.
On 2015/12/11 17:21:42, ncarter wrote:
> On 2015/12/11 00:29:03, lukasza wrote:
> > On 2015/12/10 23:07:55, rdsmith wrote:
> > > nit, suggestion/question: Why not emit the header/footer from the browser
> > > process and keep the IPC interface simpler?
> > 
> > I wasn't sure about that, but it felt better to keep all the MHTML
> > parsing/generation code (and test code) close together in Blink.
> 
> Also, you can't do file IO on the UI thread where the coordination code lives,
> so you'd have to farm it off on the file thread or the blocking pool, and
that's
> more complicated than just getting the child process to do it.

Acknowledged.

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

Thanks for reviewing.

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:294: uint32 file_flags =
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
On 2015/12/14 02:47:04, rdsmith wrote:
> On 2015/12/11 00:29:02, lukasza wrote:
> > On 2015/12/10 23:07:55, rdsmith wrote:
> > > Can the renderer seek in the file?  If it can, that means it could
overwrite
> > > things written by other frames.  This is probably ok--I don't think
> revisiting
> > > the MHTML would re-give those frames privileges to that origin--but I
wanted
> > to
> > > raise the issue in case I was wrong.
> > 
> > Thank you for catching this.  It does seem that base::File::FLAG_APPEND is
> > better because on POSIX it gets translated to O_APPEND | O_WRONLY and for
> > O_APPEND docs say "if set, the file offset shall be set to the end of the
file
> > prior to each write".  Note that FLAG_WRITE and FLAG_APPEND have to be used
> > together (otherwise we hit a DCHECK in file_posix.cc(438) - check failed:
> flags
> > & FLAG_WRITE.).
> > 
> > Done.  OTOH, note that I still need to retest on Windows... and I don't
really
> > understand what file_posix.cc and file_win.cc try to do wrt requiring
(posix)
> or
> > disallowing (win?) write+append together.  Maybe I'll have to revert this
> > particular change if it blows up on debug flavor of a windows build.
> 
> Sounds good; I'm happy to leave that to you.  I don't think this is a security
> issue as there are almost certainly more interesting things that a malicious
> renderer could do with an MHTML output request, but if O_APPEND | O_WRONLY can
> be made to work, that's probably better.

I've removed FLAG_APPEND in the latest patchset.  AFAICT using this flag (with
FLAG_CREATE_ALWAYS) in a way that doesn't DCHECK on either Posix or Windows
would requires changes under base/files/... (i.e. I've run tests with FLAG_WRITE
| FLAG_APPEND on Windows and they did hit the DCHECK I saw earlier in the code).
 I've discussed this with Nasko and he is also ok with landing as-is.

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

https://codereview.chromium.org/1386873003/diff/280001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.h:9: #include <string>
On 2015/12/11 00:29:03, lukasza wrote:
> On 2015/12/10 23:07:55, rdsmith wrote:
> > Ok, I'll bite.  I don't see any use of std::string in the header file; why
the
> > inclusion?
> 
> I don't know how that happened :-/  
> Removed / done.

Ah, I think I need this include in the next CL and when splitting the CLs into
smaller ones it accidentally got and remained here, instead of the next CL.

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

The CQ bit was checked by lukasza@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
dcheng@chromium.org, tkent@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1386873003/#ps340001
(title: "Removing base::File::FLAG_APPEND.")

[commit-bot: I haz the power, 2015-12-14 18:10:16]

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

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

The CQ bit was unchecked by lukasza@chromium.org

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

The CQ bit was checked by lukasza@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
nick@chromium.org, rdsmith@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1386873003/#ps360001
(title: "Moving "using" declaration under a singly-included section of
frame_messges.h")

[commit-bot: I haz the power, 2015-12-14 19:07:13]

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

[commit-bot: I haz the power, 2015-12-14 20:24:20]

Description was changed from

==========
OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.


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

to

==========
OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.


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

[commit-bot: I haz the power, 2015-12-14 20:24:21]

Committed patchset #18 (id:360001)

[commit-bot: I haz the power, 2015-12-14 20:25:24]

Description was changed from

==========
OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.


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

to

==========
OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

IPC Messages
============

This CL replaces the following view-oriented IPC messages:

  IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                      int /* job_id */,
                      IPC::PlatformFileForTransit /* file handle */)

  IPC_MESSAGE_CONTROL2(ViewHostMsg_SavedPageAsMHTML,
                       int /* job_id */,
                       int64 /* size of the MHTML file, -1 if error */)

with the following frame-oriented IPC messages:

  IPC_MESSAGE_ROUTED5(FrameMsg_SerializeAsMHTML,
                      int /* job_id (used to match responses to requests) */,
                      IPC::PlatformFileForTransit /* destination file */,
                      std::string /* mhtml boundary marker */,
                      FrameRoutingIdToContentIdMap,
                      bool /* is last frame */)

  IPC_MESSAGE_ROUTED3(FrameHostMsg_SerializeAsMHTMLResponse,
                      int /* job_id (used to match responses to requests) */,
                      bool /* true if success, false if error */,
                      std::string /* mhtml boundary marker that was used */)

This allows MHTML generation to work, even when some frames are in separate
processes.

Supporting changes
==================

This CL builds on top of recent CLs below:
crrev.com/1436683002 - Making PageSerializer operate on single frames only.
crrev.com/1441553002 - Generating CIDs in Blink during MHTML serialization.
crrev.com/1415463007 - Exposing more granular methods from MHTMLArchive.
crrev.com/1380963006 - MHTMLGenerator preparation for per-frame serialization.

Handling of renderer IPC messages was moved from MHTMLGenerator into
RenderFrameImpl.  Handling of browser IPC responses was moved from
RenderProcessHostImpl into RenderFrameHostImpl.

MHTMLGenerationManager now handles walking over all the frames and
asking the renderer process to serialize them into MHTML (sequentially -
one frame at a time).  MHTMLGenerationManager also handles generating
ContentIDs for all frames.

BUG=538766

Committed: https://crrev.com/70b79c8c617e3e73e4df9e7e548ff675a9886a6b
Cr-Commit-Position: refs/heads/master@{#365082}
==========

[commit-bot: I haz the power, 2015-12-14 20:25:26]

Patchset 18 (id:??) landed as
https://crrev.com/70b79c8c617e3e73e4df9e7e548ff675a9886a6b
Cr-Commit-Position: refs/heads/master@{#365082}