Fail when saving page as MHTML provides information about the cause.

Fail when saving page as MHTML provides information about the cause.

This change adds more detailed information to tracing and UMA histograms about
the reason why a MHTML save operation has failed. This will help us understand
why some specific pages consistently fail and better handle these situations.

It introduces a new enum, MhtmlSaveStatus, that lists all tracked causes. Part
of the reasons are tracked by the browser and the others by the render process
but the reporting is centralized in the former. A few tests from
MHTMLGenerationTest were also changed to verify the MHTML failures they test for
are correctly reported.

BUG=645686, 655723, 655708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/29da4fe0a2b453e474e6cd530332a2ed02f70576
Cr-Commit-Position: refs/heads/master@{#434783}

[carlosk, 2016-11-22 02:19:38]

Description was changed from

==========
Fail when saving page as MHTML provides information about the cause.

This change adds more detailed information to tracing and UMA histograms about
the reason why a MHTML save operation has failed. This will help us understand
why some specific pages consistently fail and better handle these situations.

It introduces a new enum, MhtmlSaveStatus, that lists all tracked causes. Part
of the reasons are tracked by the browser and the others by the render process
but the reporting is centralized in the former. A few tests from
MHTMLGenerationTest were also changed to verify the MHTML failures they test for
are correctly reported.

BUG=645686,655723,655708
==========

to

==========
Fail when saving page as MHTML provides information about the cause.

This change adds more detailed information to tracing and UMA histograms about
the reason why a MHTML save operation has failed. This will help us understand
why some specific pages consistently fail and better handle these situations.

It introduces a new enum, MhtmlSaveStatus, that lists all tracked causes. Part
of the reasons are tracked by the browser and the others by the render process
but the reporting is centralized in the former. A few tests from
MHTMLGenerationTest were also changed to verify the MHTML failures they test for
are correctly reported.

BUG=645686,655723,655708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[carlosk, 2016-11-22 02:19:40]

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

[commit-bot: I haz the power, 2016-11-22 02:20:31]

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-11-22 04:30:55]

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

[commit-bot: I haz the power, 2016-11-22 04:30:55]

Dry run: This issue passed the CQ dry run.

[carlosk, 2016-11-22 18:34:26]

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

[commit-bot: I haz the power, 2016-11-22 18:34:42]

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

[carlosk, 2016-11-22 18:37:11]

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

[carlosk, 2016-11-22 18:37:12]

isherman@: Please review histograms.xml and UMA related changes.

lukasza@: Please review changes in MHTML generation and tests.

nasko@: Please review IPC changes.

[commit-bot: I haz the power, 2016-11-22 19:04:30]

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

[commit-bot: I haz the power, 2016-11-22 19:04:31]

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...)

[Łukasz Anforowicz, 2016-11-22 19:22:13]

LGTM with some nits.  Please loop me in when you gather the UMA results - I am
also curious about the error distribution :-)

https://codereview.chromium.org/2519273002/diff/20001/content/browser/downloa...
File content/browser/download/mhtml_generation_browsertest.cc (right):

https://codereview.chromium.org/2519273002/diff/20001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:116:
histo_tester_.reset(new base::HistogramTester());
nit: s/histo_tester_/histogram_tester_/ ?

From https://google.github.io/styleguide/cppguide.html#General_Naming_Rules:

"""Names should be descriptive; avoid abbreviation.

Do not worry about saving horizontal space as it is far more important to make
your code immediately understandable by a new reader. Do not use abbreviations
that are ambiguous or unfamiliar to readers outside your project, and do not
abbreviate by deleting letters within a word."""

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

https://codereview.chromium.org/2519273002/diff/20001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.cc:510: save_status =
MhtmlSaveStatus::FILE_CLOSING_ERROR;
Interesting, I didn't know that base::File::GetLength can return a negative
value to indicate a failure.

https://codereview.chromium.org/2519273002/diff/20001/content/common/download...
File content/common/download/mhtml_save_status.h (right):

https://codereview.chromium.org/2519273002/diff/20001/content/common/download...
content/common/download/mhtml_save_status.h:27: FRAME_NO_LONGER_EXIST,
nit: FRAME_NO_LONGER_EXISTS

https://codereview.chromium.org/2519273002/diff/20001/content/common/download...
content/common/download/mhtml_save_status.h:29: // A frame was not allowed to be
serialized by either policy or cache control.
nit: s/A frame/The main frame/ ?

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

https://codereview.chromium.org/2519273002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5458: : base::StringPrintf("failure (%d)",
static_cast<int>(save_status)));
I wonder if it would make sense to introduce a helper function in
mhtml_save_status.h (not sure if .cc is needed or not) for the MhtmlSaveStatus
-> std::string conversion (which seems to appear in quite a few places).

[Ilya Sherman, 2016-11-22 22:38:36]

metrics lgtm

[carlosk, 2016-11-22 23:25:25]

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

[commit-bot: I haz the power, 2016-11-22 23:26:08]

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

[carlosk, 2016-11-22 23:26:24]

Thanks. 

The Android build failures were not related to my patch. Lets see if they were
resolved in the meantime...

https://codereview.chromium.org/2519273002/diff/20001/content/browser/downloa...
File content/browser/download/mhtml_generation_browsertest.cc (right):

https://codereview.chromium.org/2519273002/diff/20001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:116:
histo_tester_.reset(new base::HistogramTester());
On 2016/11/22 19:22:13, Łukasz Anforowicz wrote:
> nit: s/histo_tester_/histogram_tester_/ ?
> 
> From https://google.github.io/styleguide/cppguide.html#General_Naming_Rules:
> 
> """Names should be descriptive; avoid abbreviation.
> 
> Do not worry about saving horizontal space as it is far more important to make
> your code immediately understandable by a new reader. Do not use abbreviations
> that are ambiguous or unfamiliar to readers outside your project, and do not
> abbreviate by deleting letters within a word."""

Done.

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

https://codereview.chromium.org/2519273002/diff/20001/content/browser/downloa...
content/browser/download/mhtml_generation_manager.cc:510: save_status =
MhtmlSaveStatus::FILE_CLOSING_ERROR;
On 2016/11/22 19:22:13, Łukasz Anforowicz wrote:
> Interesting, I didn't know that base::File::GetLength can return a negative
> value to indicate a failure.

It can't. But that's the "error code" response when CloseFile invokes this
method as a callback. See MHTMLGenerationManager::Job::CloseFile.

https://codereview.chromium.org/2519273002/diff/20001/content/common/download...
File content/common/download/mhtml_save_status.h (right):

https://codereview.chromium.org/2519273002/diff/20001/content/common/download...
content/common/download/mhtml_save_status.h:27: FRAME_NO_LONGER_EXIST,
On 2016/11/22 19:22:13, Łukasz Anforowicz wrote:
> nit: FRAME_NO_LONGER_EXISTS

Done.

https://codereview.chromium.org/2519273002/diff/20001/content/common/download...
content/common/download/mhtml_save_status.h:29: // A frame was not allowed to be
serialized by either policy or cache control.
On 2016/11/22 19:22:13, Łukasz Anforowicz wrote:
> nit: s/A frame/The main frame/ ?

Done.

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

https://codereview.chromium.org/2519273002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5458: : base::StringPrintf("failure (%d)",
static_cast<int>(save_status)));
On 2016/11/22 19:22:13, Łukasz Anforowicz wrote:
> I wonder if it would make sense to introduce a helper function in
> mhtml_save_status.h (not sure if .cc is needed or not) for the MhtmlSaveStatus
> -> std::string conversion (which seems to appear in quite a few places).

Done.

[commit-bot: I haz the power, 2016-11-23 01:40:02]

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

[commit-bot: I haz the power, 2016-11-23 01:40:03]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-11-23 18:07:35]

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
File content/common/content_param_traits_macros.h (right):

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
content/common/content_param_traits_macros.h:39:
static_cast<int>(content::MhtmlSaveStatus::STATUS_COUNT) - 1)
Why the need for static casting? Can't we just define the enum with LAST member
like the rest and follow the common pattern?

https://codereview.chromium.org/2519273002/diff/60001/content/common/download...
File content/common/download/mhtml_save_status.cc (right):

https://codereview.chromium.org/2519273002/diff/60001/content/common/download...
content/common/download/mhtml_save_status.cc:8: #include
"base/strings/stringprintf.h"
Why is this include needed?

https://codereview.chromium.org/2519273002/diff/60001/content/common/download...
content/common/download/mhtml_save_status.cc:32: return "INVALID STATUS";
Keep strings consistent - you don't use all caps anywhere else, including
failures, so this should match them.

[carlosk, 2016-11-23 20:08:40]

Thanks.

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
File content/common/content_param_traits_macros.h (right):

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
content/common/content_param_traits_macros.h:39:
static_cast<int>(content::MhtmlSaveStatus::STATUS_COUNT) - 1)
On 2016/11/23 18:07:34, nasko wrote:
> Why the need for static casting? Can't we just define the enum with LAST
member
> like the rest and follow the common pattern?

Static casting is needed for the subtraction operation. I considered this other
pattern (also used above for ResourceType) somewhat better as it makes adding
new values to the enum simpler. The *LAST pattern requires that new values added
as a new value and also to the *LAST line to set its value, whereas this one
requires only the former step. I don't have strong feelings either way so WDYT?

https://codereview.chromium.org/2519273002/diff/60001/content/common/download...
File content/common/download/mhtml_save_status.cc (right):

https://codereview.chromium.org/2519273002/diff/60001/content/common/download...
content/common/download/mhtml_save_status.cc:8: #include
"base/strings/stringprintf.h"
On 2016/11/23 18:07:34, nasko wrote:
> Why is this include needed?

My mistake. Removed.

https://codereview.chromium.org/2519273002/diff/60001/content/common/download...
content/common/download/mhtml_save_status.cc:32: return "INVALID STATUS";
On 2016/11/23 18:07:34, nasko wrote:
> Keep strings consistent - you don't use all caps anywhere else, including
> failures, so this should match them.

This seemed like a big enough error case to flag it differently. I reverted to
not use all caps but added extra symbols to make it clear this is not an
expected status.

[nasko, 2016-11-24 00:07:31]

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
File content/common/content_param_traits_macros.h (right):

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
content/common/content_param_traits_macros.h:39:
static_cast<int>(content::MhtmlSaveStatus::STATUS_COUNT) - 1)
On 2016/11/23 20:08:40, carlosk wrote:
> On 2016/11/23 18:07:34, nasko wrote:
> > Why the need for static casting? Can't we just define the enum with LAST
> member
> > like the rest and follow the common pattern?
> 
> Static casting is needed for the subtraction operation. I considered this
other
> pattern (also used above for ResourceType) somewhat better as it makes adding
> new values to the enum simpler. The *LAST pattern requires that new values
added
> as a new value and also to the *LAST line to set its value, whereas this one
> requires only the former step. I don't have strong feelings either way so
WDYT?

I prefer the compiler do the work for us, which means I'd rather not have static
casts. Devs should be capable enough to fix the LAST value when adding a new one
to the enum.

[carlosk, 2016-11-24 00:21:12]

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
File content/common/content_param_traits_macros.h (right):

https://codereview.chromium.org/2519273002/diff/60001/content/common/content_...
content/common/content_param_traits_macros.h:39:
static_cast<int>(content::MhtmlSaveStatus::STATUS_COUNT) - 1)
On 2016/11/24 00:07:31, nasko wrote:
> On 2016/11/23 20:08:40, carlosk wrote:
> > On 2016/11/23 18:07:34, nasko wrote:
> > > Why the need for static casting? Can't we just define the enum with LAST
> > member
> > > like the rest and follow the common pattern?
> > 
> > Static casting is needed for the subtraction operation. I considered this
> other
> > pattern (also used above for ResourceType) somewhat better as it makes
adding
> > new values to the enum simpler. The *LAST pattern requires that new values
> added
> > as a new value and also to the *LAST line to set its value, whereas this one
> > requires only the former step. I don't have strong feelings either way so
> WDYT?
> 
> I prefer the compiler do the work for us, which means I'd rather not have
static
> casts. Devs should be capable enough to fix the LAST value when adding a new
one
> to the enum.

Done.

[nasko, 2016-11-24 00:24:57]

LGTM with a nit.

https://codereview.chromium.org/2519273002/diff/100001/content/common/downloa...
File content/common/download/mhtml_save_status.h (right):

https://codereview.chromium.org/2519273002/diff/100001/content/common/downloa...
content/common/download/mhtml_save_status.h:37: // NOTE: always keep this entry
at the end and Add new status types only
nit: add

[carlosk, 2016-11-24 00:28:13]

Thanks.

https://codereview.chromium.org/2519273002/diff/100001/content/common/downloa...
File content/common/download/mhtml_save_status.h (right):

https://codereview.chromium.org/2519273002/diff/100001/content/common/downloa...
content/common/download/mhtml_save_status.h:37: // NOTE: always keep this entry
at the end and Add new status types only
On 2016/11/24 00:24:57, nasko wrote:
> nit: add

Done.

[carlosk, 2016-11-24 00:28:22]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-11-24 00:28:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, isherman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2519273002/#ps120001
(title: "Fixed typo.")

[commit-bot: I haz the power, 2016-11-24 00:29:31]

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-11-24 02:30:40]

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

[commit-bot: I haz the power, 2016-11-24 02:30:41]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[carlosk, 2016-11-24 03:03:51]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2016-11-24 03:04:41]

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-11-24 04:21:19]

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

[commit-bot: I haz the power, 2016-11-24 04:21:21]

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-11-28 21:17:23]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2016-11-28 21:18:37]

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-11-29 00:01:35]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480367843435060,
"parent_rev": "4b2bfcea62f26f9b866604e13f6b97ec852f8732", "commit_rev":
"38f7599b47efcbf503a340c0179071a6098014cd"}

[commit-bot: I haz the power, 2016-11-29 00:04:40]

Description was changed from

==========
Fail when saving page as MHTML provides information about the cause.

This change adds more detailed information to tracing and UMA histograms about
the reason why a MHTML save operation has failed. This will help us understand
why some specific pages consistently fail and better handle these situations.

It introduces a new enum, MhtmlSaveStatus, that lists all tracked causes. Part
of the reasons are tracked by the browser and the others by the render process
but the reporting is centralized in the former. A few tests from
MHTMLGenerationTest were also changed to verify the MHTML failures they test for
are correctly reported.

BUG=645686,655723,655708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fail when saving page as MHTML provides information about the cause.

This change adds more detailed information to tracing and UMA histograms about
the reason why a MHTML save operation has failed. This will help us understand
why some specific pages consistently fail and better handle these situations.

It introduces a new enum, MhtmlSaveStatus, that lists all tracked causes. Part
of the reasons are tracked by the browser and the others by the render process
but the reporting is centralized in the former. A few tests from
MHTMLGenerationTest were also changed to verify the MHTML failures they test for
are correctly reported.

BUG=645686,655723,655708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/29da4fe0a2b453e474e6cd530332a2ed02f70576
Cr-Commit-Position: refs/heads/master@{#434783}
==========

[commit-bot: I haz the power, 2016-11-29 00:04:41]

Patchset 7 (id:??) landed as
https://crrev.com/29da4fe0a2b453e474e6cd530332a2ed02f70576
Cr-Commit-Position: refs/heads/master@{#434783}