Move MHTML file writing out of the renderer main thread.

Move MHTML file writing out of the renderer main thread.

This change moves the step of writing MHTML serialized and encoded data
to file out of the renderer main thread, into its FILE thread. The less
we block the main one, the more responsive the renderer remains which is
good.

Note: I mande a correction to the logic tracked by the UMA histogram
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame to make sure
it includes the file close operation that might involve a buffer flush
that could be significant.

BUG=651525, 645686
Committed: https://crrev.com/27fa065af52591c67adba826584ff854beaa0a35
Cr-Commit-Position: refs/heads/master@{#423331}

[carlosk, 2016-09-29 18:30:06]

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

[commit-bot: I haz the power, 2016-09-29 18:30:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 18:33:22]

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

[commit-bot: I haz the power, 2016-09-29 18:33:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[carlosk, 2016-09-29 18:46:53]

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

[commit-bot: I haz the power, 2016-09-29 18:47:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2016-09-29 19:10:39]

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

[carlosk, 2016-09-29 19:10:40]

nasko@chromium.org, lukasza@chromium.org: PTAL.

[carlosk, 2016-09-29 19:14:25]

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

[commit-bot: I haz the power, 2016-09-29 19:14:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-09-29 19:56:18]

Everything looks good, but we need to figure out how to close the file after a
failure (and how that might affect perf measurements).

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

https://codereview.chromium.org/2379823003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:5276: } else {
I think we need to call |file.Close()| here?

Alternatively, we could try to simplify and always go through
PostTaskAndReplyWithResult (rather than special-casing success + 3 empty results
here), because it seems to me that 1) the else branch should happen rarely + 2)
even if the else branch does happen, the performance impact should be relatively
small (yes, 2 thread hops, but no file IO, and there is always the IPC response
later).

WDYT?

[commit-bot: I haz the power, 2016-09-29 20:45:11]

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

[commit-bot: I haz the power, 2016-09-29 20:45:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[carlosk, 2016-09-30 00:16:51]

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

[commit-bot: I haz the power, 2016-09-30 00:17:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2016-09-30 00:20:18]

Thanks. It seems that the change you suggested also fixed the Android test
failure. The Windows one I think is unrelated but we'll see what the dry run
says.

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

https://codereview.chromium.org/2379823003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:5276: } else {
On 2016/09/29 19:56:18, Łukasz Anforowicz wrote:
> I think we need to call |file.Close()| here?
> 
> Alternatively, we could try to simplify and always go through
> PostTaskAndReplyWithResult (rather than special-casing success + 3 empty
results
> here), because it seems to me that 1) the else branch should happen rarely +
2)
> even if the else branch does happen, the performance impact should be
relatively
> small (yes, 2 thread hops, but no file IO, and there is always the IPC
response
> later).
> 
> WDYT?

Done.

I agree that the else should happen way less often and the cost of 2 thread hops
it minimal when compared to the IPC. So I unified the code paths there.

[commit-bot: I haz the power, 2016-09-30 00:21:08]

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

[commit-bot: I haz the power, 2016-09-30 00:21:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[carlosk, 2016-09-30 00:59:14]

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

[commit-bot: I haz the power, 2016-09-30 00:59:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 02:03:40]

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

[commit-bot: I haz the power, 2016-09-30 02:03:41]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[carlosk, 2016-09-30 03:23:10]

These same trybot failing tests I see here are passing when I run them locally
on my N5X. I'll have to figure out tomorrow what's going on... :/

[carlosk, 2016-09-30 22:47:16]

dewittj@ just showed me what seems to be the issue with my patch: WebData
instances *cannot* be passed between threads [1]. So I'll have to see what other
way can we pass serialized data from the main to the file thread for this to
work.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebDa...

[carlosk, 2016-10-03 22:47:15]

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

[commit-bot: I haz the power, 2016-10-03 22:47:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 22:52:22]

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

[commit-bot: I haz the power, 2016-10-03 22:52:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[carlosk, 2016-10-03 23:11:45]

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

[commit-bot: I haz the power, 2016-10-03 23:12:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 23:16:01]

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

[commit-bot: I haz the power, 2016-10-03 23:16:02]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[carlosk, 2016-10-03 23:51:04]

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

[carlosk, 2016-10-03 23:51:05]

dcheng@: could you PTAL on my changes to replace the usage of WebData for MHTML
generation? Does this looks reasonable as far as Blink standards go?

https://codereview.chromium.org/2379823003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/2379823003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:240: DCHECK(encoding
== Encoding::Base64);
I replaced ASSERT with DCHECK here as I understand this is the way forward.
Should I replace all other occurrences in this file?

https://codereview.chromium.org/2379823003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h (right):

https://codereview.chromium.org/2379823003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h:65: std::vector<char>&
outputBuffer);
Is it really OK to pass non const references in Blink? Or should I change this
to a pointer instead?

[carlosk, 2016-10-03 23:59:34]

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

[commit-bot: I haz the power, 2016-10-04 00:00:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 00:19:09]

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

[commit-bot: I haz the power, 2016-10-04 00:19:10]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[carlosk, 2016-10-04 00:41:05]

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

[commit-bot: I haz the power, 2016-10-04 00:41:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2016-10-04 00:42:52]

Patchset #11 (id:190008) has been deleted

[carlosk, 2016-10-04 00:45:17]

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

[commit-bot: I haz the power, 2016-10-04 00:45:26]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2016-10-04 00:45:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 02:11:58]

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

[commit-bot: I haz the power, 2016-10-04 02:11:59]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-04 04:34:48]

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

https://codereview.chromium.org/2379823003/diff/210001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:81: BLINK_EXPORT static
std::vector<char> generateMHTMLHeader(
We don't allow STL containers (string, vector, map, etc) in the public headers.
Similarly, we also don't use STL inside Blink currently.

Historical background can be found at
https://lists.webkit.org/pipermail/webkit-dev/2008-January/003100.html

In order to use STL containers in Blink, at least several things need
investigation:
- Performance: someone needs to measure if the STL equivalent to the WTF version
is just as performant. There are anecdotes that the WTF versions are a perf win,
but IIRC, no one has measured recently.
- Consistency: if there are perf issues, it's easier to have consistent rules
and just not allow STL containers
- Codegen: On some platforms, std::string is actually COW and inlines a ton of
code. Someone needs to demonstrate that the STL versions are no worse than the
WTF versions (and ideally better)

So unfortunately, this will have to use WebData or WebThreadSafeData, if we need
to pass between threads.

* You'll see some usage in tests, but that's about it.

[carlosk, 2016-10-04 21:34:06]

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

[commit-bot: I haz the power, 2016-10-04 21:34:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2016-10-04 21:37:02]

Description was changed from

==========
Move MHTML file writing out of the renderer main thread.

This change moves the step of writing MHTML serialized and encoded data
to file out of the renderer main thread, into its FILE thread. The less
we block the main one, the more responsive the renderer remains which is
good.

BUG=651525
==========

to

==========
Move MHTML file writing out of the renderer main thread.

This change moves the step of writing MHTML serialized and encoded data
to file out of the renderer main thread, into its FILE thread. The less
we block the main one, the more responsive the renderer remains which is
good.

Note: I mande a correction to the logic tracked by the UMA histogram
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame to make sure
it includes the file close operation that might involve a buffer flush
that could be significant.

BUG=651525,645686
==========

[carlosk, 2016-10-04 21:49:54]

Thanks.

I reverted back this last set of changes but switched the raw data container to
WebThreadSafeData instances. Could you all PTAL?

https://codereview.chromium.org/2379823003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h (right):

https://codereview.chromium.org/2379823003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h:65: std::vector<char>&
outputBuffer);
On 2016/10/03 23:51:05, carlosk wrote:
> Is it really OK to pass non const references in Blink? Or should I change this
> to a pointer instead?

Just for the record: I did ask dcheng@ directly about this and passing non-const
references is acceptable in Blink as it implies a safe, never null "pointer" to
an output argument.

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

https://codereview.chromium.org/2379823003/diff/230001/content/renderer/rende...
content/renderer/render_frame_impl.cc:795:
"PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame");
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame is a little
changed now as it includes the file.Close() operation as it should have from the
start. I'm not concerned with the effect on the histogram as a) I don't expect
this to be a huge change and b) this has at most been release in the Canary
channel.

https://codereview.chromium.org/2379823003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/2379823003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/MHTMLTest.cpp:248:
SharedBuffer::create(rawData->data(), rawData->length());
Changing the parsing of MHTML to work with RawData insetad of SharedBuffer would
be a much greater task. As this direct data flow from generation to parsing only
happens here in tests I deemed this copy operation was good enough.

[dcheng, 2016-10-04 22:05:32]

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

[dcheng, 2016-10-04 22:05:33]

+dmurph as the blobs expert, just to confirm that this isn't a super surprising
way of using RawData / WebThreadSafeData.

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

https://codereview.chromium.org/2379823003/diff/230001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5280:
base::Passed(std::move(mhtml_contents)),
This can be shortened to base::Passed(&mhtml_contents)

https://codereview.chromium.org/2379823003/diff/230001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5283: base::Unretained(this),
params.job_id,
I think this needs to be a WeakPtr

[dmurph, 2016-10-05 00:08:27]

The use of WebThreadSafeData lgtm

[carlosk, 2016-10-05 00:24:14]

Thanks.

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

https://codereview.chromium.org/2379823003/diff/230001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5280:
base::Passed(std::move(mhtml_contents)),
On 2016/10/04 22:05:33, dcheng wrote:
> This can be shortened to base::Passed(&mhtml_contents)

Done. I was unsure about this as I found both versions in codesearch but now
that I read bind_helpers.h I see they are exchangeable.

https://codereview.chromium.org/2379823003/diff/230001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5283: base::Unretained(this),
params.job_id,
On 2016/10/04 22:05:33, dcheng wrote:
> I think this needs to be a WeakPtr

Done.

[dcheng, 2016-10-05 00:58:16]

LGTM with a comment for the tests

https://codereview.chromium.org/2379823003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/2379823003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/MHTMLTest.cpp:248:
SharedBuffer::create(rawData->data(), rawData->length());
Can the code below use rawData.data()? Then we don't have to convert at all.

[carlosk, 2016-10-05 01:03:10]

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

[carlosk, 2016-10-05 01:03:11]

Thanks.

tkent@: PTAL at MHTMLArchive.*

lucasza@: do you have more comments?

https://codereview.chromium.org/2379823003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/2379823003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/MHTMLTest.cpp:248:
SharedBuffer::create(rawData->data(), rawData->length());
On 2016/10/05 00:58:16, dcheng wrote:
> Can the code below use rawData.data()? Then we don't have to convert at all.

That would require a much larger change. See my previous comment:
https://codereview.chromium.org/2379823003/diff/230001/third_party/WebKit/Sou...

[tkent, 2016-10-05 02:04:02]

> tkent@: PTAL at MHTMLArchive.*

lgtm

[Łukasz Anforowicz, 2016-10-05 18:34:37]

LGTM

[nasko, 2016-10-05 18:56:54]

Just a few small things.

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

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:787: bool WriteMHTMLToDisk(bool success,
This should go in the anonymous namespace at the top of the file.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:791: "WriteMHTMLToDisk
(RenderFrameImpl)");
Since WriteMHTMLToDisk is already RenderFrameImpl internal, why do we need to
include that in the string? It will just add unneeded detail making trace files
bigger.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: void
RenderFrameImpl::OnWriteMHTMLToDiskComplete(
This should be ordered the same way it is ordered in the header file.
Alternatively, move the declaration in the header.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5294: // Note: this method must be short
enough to not need to be accounted for in
nit: "must be short enough" implies something that needs to be ensured by code
touching this method. There is nothing one can do here for Send(). Probably
reword it to say that Send is likely small enough to not need accounting. I like
your other comment that has "assume", which was more clear.

[carlosk, 2016-10-05 20:23:37]

Thanks!

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

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:787: bool WriteMHTMLToDisk(bool success,
On 2016/10/05 18:56:54, nasko (slow) wrote:
> This should go in the anonymous namespace at the top of the file.

This is already in that namespace, whose scope ends in line 873.

I anyway moved it down so to be right before the if-define scoped section
begins.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:791: "WriteMHTMLToDisk
(RenderFrameImpl)");
On 2016/10/05 18:56:54, nasko (slow) wrote:
> Since WriteMHTMLToDisk is already RenderFrameImpl internal, why do we need to
> include that in the string? It will just add unneeded detail making trace
files
> bigger.

IMO when I look at trace reports this kinds of thing helps me understand "where"
events happen. This is basically to cope with this function not being named
RenderFrameImpl::WriteMHTMLToDisk. WDYT?

As for the concern in regards of trace file size, this trace is infrequent
enough to be insignificant in that sense.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: void
RenderFrameImpl::OnWriteMHTMLToDiskComplete(
On 2016/10/05 18:56:54, nasko (slow) wrote:
> This should be ordered the same way it is ordered in the header file.
> Alternatively, move the declaration in the header.

Done, 2nd option.

I was split between following the same order from the header and keeping things
in context in the .cpp. In this case it seemed better to follow the latter.

From your comment I understand it is OK to mix a method that is not directly and
IPC handler into the "IPC message handlers" section. Am I understanding
correctly?

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5294: // Note: this method must be short
enough to not need to be accounted for in
On 2016/10/05 18:56:54, nasko (slow) wrote:
> nit: "must be short enough" implies something that needs to be ensured by code
> touching this method. There is nothing one can do here for Send(). Probably
> reword it to say that Send is likely small enough to not need accounting. I
like
> your other comment that has "assume", which was more clear.

Done.

[nasko, 2016-10-05 20:41:10]

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

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:791: "WriteMHTMLToDisk
(RenderFrameImpl)");
On 2016/10/05 20:23:37, carlosk wrote:
> On 2016/10/05 18:56:54, nasko (slow) wrote:
> > Since WriteMHTMLToDisk is already RenderFrameImpl internal, why do we need
to
> > include that in the string? It will just add unneeded detail making trace
> files
> > bigger.
> 
> IMO when I look at trace reports this kinds of thing helps me understand
"where"
> events happen. This is basically to cope with this function not being named
> RenderFrameImpl::WriteMHTMLToDisk. WDYT?
> 
> As for the concern in regards of trace file size, this trace is infrequent
> enough to be insignificant in that sense.

If it is about context, then let's just have it be the method
"RenderFrameImpl::WriteMHTMLToDisk" :). If someone renames the method, they can
change the string too.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: void
RenderFrameImpl::OnWriteMHTMLToDiskComplete(
On 2016/10/05 20:23:37, carlosk wrote:
> On 2016/10/05 18:56:54, nasko (slow) wrote:
> > This should be ordered the same way it is ordered in the header file.
> > Alternatively, move the declaration in the header.
> 
> Done, 2nd option.
> 
> I was split between following the same order from the header and keeping
things
> in context in the .cpp. In this case it seemed better to follow the latter.
> 
> From your comment I understand it is OK to mix a method that is not directly
and
> IPC handler into the "IPC message handlers" section. Am I understanding
> correctly?

Let's not put non-IPC methods in the middle of IPC handlers.

[carlosk, 2016-10-05 21:18:01]

Thanks.

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

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:791: "WriteMHTMLToDisk
(RenderFrameImpl)");
On 2016/10/05 20:41:10, nasko (slow) wrote:
> On 2016/10/05 20:23:37, carlosk wrote:
> > On 2016/10/05 18:56:54, nasko (slow) wrote:
> > > Since WriteMHTMLToDisk is already RenderFrameImpl internal, why do we need
> to
> > > include that in the string? It will just add unneeded detail making trace
> > files
> > > bigger.
> > 
> > IMO when I look at trace reports this kinds of thing helps me understand
> "where"
> > events happen. This is basically to cope with this function not being named
> > RenderFrameImpl::WriteMHTMLToDisk. WDYT?
> > 
> > As for the concern in regards of trace file size, this trace is infrequent
> > enough to be insignificant in that sense.
> 
> If it is about context, then let's just have it be the method
> "RenderFrameImpl::WriteMHTMLToDisk" :). If someone renames the method, they
can
> change the string too.

As explained offline traces are generally named after the method they trace and
as this is in an anonymous namespace that name wouldn't match and might cause
confusion. Leaving as is.

https://codereview.chromium.org/2379823003/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: void
RenderFrameImpl::OnWriteMHTMLToDiskComplete(
On 2016/10/05 20:41:10, nasko (slow) wrote:
> On 2016/10/05 20:23:37, carlosk wrote:
> > On 2016/10/05 18:56:54, nasko (slow) wrote:
> > > This should be ordered the same way it is ordered in the header file.
> > > Alternatively, move the declaration in the header.
> > 
> > Done, 2nd option.
> > 
> > I was split between following the same order from the header and keeping
> things
> > in context in the .cpp. In this case it seemed better to follow the latter.
> > 
> > From your comment I understand it is OK to mix a method that is not directly
> and
> > IPC handler into the "IPC message handlers" section. Am I understanding
> > correctly?
> 
> Let's not put non-IPC methods in the middle of IPC handlers.

Done.

[nasko, 2016-10-05 21:29:40]

LGTM

[carlosk, 2016-10-05 21:34:29]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-10-05 21:34:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org,
tkent@chromium.org, lukasza@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2379823003/#ps290001
(title: "Address reviewer comments.")

[commit-bot: I haz the power, 2016-10-05 21:34:58]

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

[commit-bot: I haz the power, 2016-10-05 23:11:02]

Committed patchset #15 (id:290001)

[commit-bot: I haz the power, 2016-10-05 23:13:39]

Description was changed from

==========
Move MHTML file writing out of the renderer main thread.

This change moves the step of writing MHTML serialized and encoded data
to file out of the renderer main thread, into its FILE thread. The less
we block the main one, the more responsive the renderer remains which is
good.

Note: I mande a correction to the logic tracked by the UMA histogram
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame to make sure
it includes the file close operation that might involve a buffer flush
that could be significant.

BUG=651525,645686
==========

to

==========
Move MHTML file writing out of the renderer main thread.

This change moves the step of writing MHTML serialized and encoded data
to file out of the renderer main thread, into its FILE thread. The less
we block the main one, the more responsive the renderer remains which is
good.

Note: I mande a correction to the logic tracked by the UMA histogram
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame to make sure
it includes the file close operation that might involve a buffer flush
that could be significant.

BUG=651525,645686

Committed: https://crrev.com/27fa065af52591c67adba826584ff854beaa0a35
Cr-Commit-Position: refs/heads/master@{#423331}
==========

[commit-bot: I haz the power, 2016-10-05 23:13:40]

Patchset 15 (id:??) landed as
https://crrev.com/27fa065af52591c67adba826584ff854beaa0a35
Cr-Commit-Position: refs/heads/master@{#423331}