Adds tracing to MHTML generation call chain.

Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

A new trace category was created: page-serialization. I don't expect it
to generate much noise so I left is as default-enabled. I also kept the
name not MHTML specific as part of the renderer code seems to be intended
to be more generic.

BUG=645686
Committed: https://crrev.com/f86fb54ec35e00f6b5b744ddd35dc19049caae7c
Cr-Commit-Position: refs/heads/master@{#420104}

[carlosk, 2016-09-16 22:35:11]

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

[commit-bot: I haz the power, 2016-09-16 22:35: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-16 22:39:11]

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

[commit-bot: I haz the power, 2016-09-16 22:39:12]

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...)
  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-09-16 22:40:58]

Description was changed from

==========
Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

BUG=645686
==========

to

==========
Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

A new trace category was created: page-serialization. I don't expect it
to generate much noise hence hence why I left is as default-enabled. I
also kept the name not MHTML specific as part of the renderer code seems
to be intended to be more generic.

BUG=645686
==========

[carlosk, 2016-09-16 22:45:17]

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

[carlosk, 2016-09-16 22:45:24]

Patchset #1 (id:1) has been deleted

[carlosk, 2016-09-16 22:45:36]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-16 22:45:49]

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-16 22:52:54]

carlosk@chromium.org changed reviewers:
+ esprehn@chromium.org, rdsmith@chromium.org

[carlosk, 2016-09-16 22:52:55]

rdsmith@chromium.org: PTAL at mhtml file.

esprehn@chromium.org: PTAL at renderer and Blink files.

[commit-bot: I haz the power, 2016-09-16 22:57:59]

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

[commit-bot: I haz the power, 2016-09-16 22:58:00]

Dry run: Try jobs failed on following builders:
  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-...)

[carlosk, 2016-09-17 00:48:56]

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

[commit-bot: I haz the power, 2016-09-17 00:49:14]

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-17 00:49:52]

Patchset #2 (id:60001) has been deleted

[carlosk, 2016-09-17 00:50:30]

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

[commit-bot: I haz the power, 2016-09-17 00:50:47]

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-17 01:24:37]

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

[commit-bot: I haz the power, 2016-09-17 01:24:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[carlosk, 2016-09-17 02:28:13]

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

[commit-bot: I haz the power, 2016-09-17 02:28:27]

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-17 03:46:42]

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

[commit-bot: I haz the power, 2016-09-17 03:46:43]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-20 22:44:59]

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

[Łukasz Anforowicz, 2016-09-20 22:44:59]

Drive-by comments from somebody who happened to touch
mhtml_generation_manager.cc some time ago.

nit for CL description: s/hence hence why/so/

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

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:228: this, "tree node id",
frame_tree_node_id);
nit: Would it be worth spelling out "frame tree node id" to avoid any confusion?

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:339: "page-serialization",
"SavingMhtmlJob", FindJob(job_id), "url",
FindJob above looks weird (maybe just to me :-).  Would it be possible to change
NewJob to return a Job pointer and then replace |FindJob(...)| with |job->id()|?

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:340:
web_contents->GetLastCommittedURL().spec().c_str(), "file",
I don't know if GetLastCommittedURL is guaranteed to always return a valid URL. 
Maybe change this to  possibly_invalid_spec?

[esprehn, 2016-09-21 00:14:58]

lgtm w/ nits

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

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:226: rfh->Send(new
FrameMsg_SerializeAsMHTML(rfh->GetRoutingID(), ipc_params));
we really need to rewrite this with mojo and move it into blink

https://codereview.chromium.org/2344153003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2344153003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:226: "resource count",
static_cast<unsigned long long>(resources.size()));
size_t ? unsigned long long is very unusual

[Randy Smith (Not in Mondays), 2016-09-21 16:15:58]

mhtml_generation_manager.cc LGTM, but I encourage you to address Lukasz'
comments (he's dealt with the code more recently than I).

[carlosk, 2016-09-21 16:45:03]

Description was changed from

==========
Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

A new trace category was created: page-serialization. I don't expect it
to generate much noise hence hence why I left is as default-enabled. I
also kept the name not MHTML specific as part of the renderer code seems
to be intended to be more generic.

BUG=645686
==========

to

==========
Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

A new trace category was created: page-serialization. I don't expect it
to generate much noise so I left is as default-enabled. I also kept the
name not MHTML specific as part of the renderer code seems to be intended
to be more generic.

BUG=645686
==========

[carlosk, 2016-09-21 16:47:20]

Thanks for all reviews.

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

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:226: rfh->Send(new
FrameMsg_SerializeAsMHTML(rfh->GetRoutingID(), ipc_params));
On 2016/09/21 00:14:57, esprehn wrote:
> we really need to rewrite this with mojo and move it into blink

Acknowledged.

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:228: this, "tree node id",
frame_tree_node_id);
On 2016/09/20 22:44:59, Łukasz Anforowicz wrote:
> nit: Would it be worth spelling out "frame tree node id" to avoid any
confusion?

Done.

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:339: "page-serialization",
"SavingMhtmlJob", FindJob(job_id), "url",
On 2016/09/20 22:44:59, Łukasz Anforowicz wrote:
> FindJob above looks weird (maybe just to me :-).  Would it be possible to
change
> NewJob to return a Job pointer and then replace |FindJob(...)| with
|job->id()|?

Sounds better to me too. Done.

This mapping should also be updated to use smart pointers. I wlill try to do
that eventually as I should visit this file a few more times.

https://codereview.chromium.org/2344153003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:340:
web_contents->GetLastCommittedURL().spec().c_str(), "file",
On 2016/09/20 22:44:59, Łukasz Anforowicz wrote:
> I don't know if GetLastCommittedURL is guaranteed to always return a valid
URL. 
> Maybe change this to  possibly_invalid_spec?

If the URL was committed it must be valid otherwise the navigation leading to it
would have been stopped early on.

BUT as there's no significant difference and the returned value is what's
referred to as a "virtual URL" (it can contain some extra snippets) I will make
that change anyway. 

So done!

https://codereview.chromium.org/2344153003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2344153003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:226: "resource count",
static_cast<unsigned long long>(resources.size()));
On 2016/09/21 00:14:58, esprehn wrote:
> size_t ? unsigned long long is very unusual

Yes, this is unusual but this solves a compilation error in patch set #2. I
explained the issue in a chromium-dev thread [1] that  I started because of
tracing issues in Blink.

[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CyMqAd0Kld4/w8NYs...

[carlosk, 2016-09-21 16:47:48]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-09-21 16:47:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2344153003/#ps120001
(title: "Address code review comments")

[commit-bot: I haz the power, 2016-09-21 16:48:40]

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-21 18:12:52]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-09-21 18:14:23]

Description was changed from

==========
Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

A new trace category was created: page-serialization. I don't expect it
to generate much noise so I left is as default-enabled. I also kept the
name not MHTML specific as part of the renderer code seems to be intended
to be more generic.

BUG=645686
==========

to

==========
Adds tracing to MHTML generation call chain.

This change adds tracing macro calls to many points of the MHTML
generation code over browser and renderer code, both in content and
Blink. These traces should help with futher performance investigations.

A new trace category was created: page-serialization. I don't expect it
to generate much noise so I left is as default-enabled. I also kept the
name not MHTML specific as part of the renderer code seems to be intended
to be more generic.

BUG=645686

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

[commit-bot: I haz the power, 2016-09-21 18:14:24]

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