[Offline Pages] Generate MHTML header in the browser process.

[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

Paquete #fixit
BUG=672292

[dewittj, 2017-04-24 23:25:30]

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

[commit-bot: I haz the power, 2017-04-24 23:25:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-04-25 00:00:47]

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

[commit-bot: I haz the power, 2017-04-25 00:01:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-04-25 00:10:15]

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

[commit-bot: I haz the power, 2017-04-25 00:11:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-04-25 00:12:15]

dewittj@chromium.org changed reviewers:
+ carlosk@chromium.org

[dewittj, 2017-04-25 00:12:16]

+carlosk: please take an early look.  It's a bit ugly because I'm duplicating
logic in the browser that exists in the renderer (you did this before too :) )

Does this look like the right direction?

[commit-bot: I haz the power, 2017-04-25 00:42:05]

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

[commit-bot: I haz the power, 2017-04-25 00:42:06]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[carlosk, 2017-04-25 01:28:07]

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

[carlosk, 2017-04-25 01:28:08]

Thanks for working on this dewittj@ and this looks like the correct direction.

I agree with what you mentioned offline that this feels like too much code added
and/or duplicated for the (small?) consistency improvement of having both the
footer and header now being written by the browser process. 

+lukasza@ for MHTML code review and because he expressed interest in having the
header code also moved to the browser. WDYT?

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

https://codereview.chromium.org/2842653002/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.cc:699: result.header_size =
-1;
Set default to -1 so not to have to re-set it here.

https://codereview.chromium.org/2842653002/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.cc:708: return result;
Could you differentiate here between an error creating the file
(MhtmlSaveStatus::FILE_CREATION_ERROR) and another for trying to write the
header (MhtmlSaveStatus::FILE_WRITTING_ERROR)? As header_size is only user for
error checking you could replace it with a MhtmlSaveStatus member.

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

https://codereview.chromium.org/2842653002/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.h:72: // Called on the file
thread to create |file|.
Append "and write the MHTML header into it".

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

https://codereview.chromium.org/2842653002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5631: "RenderFrameImpl::OnSerializeAsMHTML
header");
Could you make this header serialization tracing survive this change like it was
done for the footer change? (see
MHTMLGenerationManager::Job::FinalizeAndCloseFileOnFileThread)

Ideally it would encompass the header generation/serialization too but that
might not be ideal for this case given all the data needed to generate it.

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

https://codereview.chromium.org/2842653002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5640: // frame is serialized by a renderer
process.
nit: we should either remove this comment above or add a similar one for the
MTHML header. I'm slightly favoring the former as this comment only makes sense
because the footer used to be generated here and that doesn't matter anymore.

https://codereview.chromium.org/2842653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h (right):

https://codereview.chromium.org/2842653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h:56: // Generates an
MHTML header and appends it to |outputBuffer|.
Append "for testing purposes".

[Łukasz Anforowicz, 2017-04-25 17:20:51]

Description was changed from

==========
[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

BUG=672292
==========

to

==========
[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

BUG=672292
==========

[Łukasz Anforowicz, 2017-04-25 17:21:53]

<+tkent@ for Blink / platform-architecture-dev@ advice>

Hmmm... I don't like that after this CL, we will have quite a bit of code
duplication between Blink and //content:
- MHTMLArchive::GenerateMHTMLHeader[ForTesting]
- WTF::MakeRFC2822DateString (the only caller was [is]
MHTMLArchive::GenerateMHTMLHeader[ForTesting])
The duplication seems undesirable.

Are we sure that the disadvantage of the code duplication is outweighed by
advantages brought by this CL?  I am not convinced if writing the header from
the browser is a huge performance win;  I also think that the aesthetics win of
writing both the header and the footer from the same place is smaller than the
loss brought by the extra code duplication.

tkent@, is there a precedent for having a piece of code that both the 1)
//content/browser and 2) third_party/WebKit/Sources depend on?  I think CSP
parsing+handling might be similar in this respect (with some policies being
enforced from the browser and most policies enforced from within Blink).  Would
it make sense to similarly carve out MHTML generation and/or parsing and reuse
it from both //content/browser and from third_party/WebKit/Sources?

[dewittj, 2017-04-25 18:36:05]

Description was changed from

==========
[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

BUG=672292
==========

to

==========
[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

Paquete #fixit

BUG=672292
==========

[dewittj, 2017-04-25 18:36:17]

Description was changed from

==========
[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

Paquete #fixit

BUG=672292
==========

to

==========
[Offline Pages] Generate MHTML header in the browser process.

This completes the work moving header and footer to the browser process,
reducing load in the renderer.

Paquete #fixit
BUG=672292
==========

[tkent, 2017-04-26 00:29:24]

> Hmmm... I don't like that after this CL, we will have quite a bit of code
duplication between Blink and //content:
> - MHTMLArchive::GenerateMHTMLHeader[ForTesting]
> - WTF::MakeRFC2822DateString (the only caller was [is]
MHTMLArchive::GenerateMHTMLHeader[ForTesting])
> The duplication seems undesirable.
> 
> Are we sure that the disadvantage of the code duplication is outweighed by
advantages brought by this CL?  I am not convinced if writing the header from
the browser is a huge performance win;  I also think that the aesthetics win of
writing both the header and the footer from the same place is smaller than the
loss brought by the extra code duplication.
> 
> tkent@, is there a precedent for having a piece of code that both the 1)
//content/browser and 2) third_party/WebKit/Sources depend on?  I think CSP
parsing+handling might be similar in this respect (with some policies being
enforced from the browser and most policies enforced from within Blink).  Would
it make sense to similarly carve out MHTML generation and/or parsing and reuse
it from both //content/browser and from third_party/WebKit/Sources?

In this case, we should try to remove WTF::MakeRFC2822DateString().
GenerateMHTMLHeaderForTesting() may generate a fixed string for Date: field, or
we should not generate headers for testing because Blink doesn't have code to
produce headers.

If we need to share the code, I guess we should add it to base/time/, and wrap
it in platform/wtf/.

[dewittj, 2017-04-27 17:25:09]

It seems to me that this is not worth the complexity/duplication. Thanks for
your feedback!