Adding a command-line flag to set MHTML generation options.

Adding a command-line flag to set MHTML generation options.

It is a content-level flag (since MHTML generation is a content feature) and initial
set of options is to follow standard cache-control: no-save option to avoid
saving MHTML files with potentially private/transient information.

BUG=614505
Committed: https://crrev.com/0710ba43dca3d29b5658e8aa221ac7adc6e3cfb5
Cr-Commit-Position: refs/heads/master@{#395820}

[Dmitry Titov, 2016-05-24 22:52:18]

dimich@chromium.org changed reviewers:
+ dewittj@chromium.org, dimich@chromium.org

[dewittj, 2016-05-24 23:03:21]

lgtm

https://codereview.chromium.org/2013633003/diff/1/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2013633003/diff/1/content/browser/download/sa...
content/browser/download/save_package.cc:347: std::string
MHTMLGeneratorOptionFlag =
Another option would be to override the default in MHTMLGenerationManager, which
would then apply to extension API as well...

https://codereview.chromium.org/2013633003/diff/1/content/browser/download/sa...
content/browser/download/save_package.cc:351: if (MHTMLGeneratorOptionFlag ==
"skip_nostore_main") {
hm, wish this could be a constant that's shared in the about_flags.cc as well.

[Dmitry Titov, 2016-05-24 23:29:06]

On 2016/05/24 23:03:21, dewittj wrote:
> lgtm
> 
>
https://codereview.chromium.org/2013633003/diff/1/content/browser/download/sa...
> File content/browser/download/save_package.cc (right):
> 
>
https://codereview.chromium.org/2013633003/diff/1/content/browser/download/sa...
> content/browser/download/save_package.cc:347: std::string
> MHTMLGeneratorOptionFlag =
> Another option would be to override the default in MHTMLGenerationManager,
which
> would then apply to extension API as well...

Done, by moving the code to constructor of MHTMLGenerationParams so it now
affects the default value of that struct.

> 
>
https://codereview.chromium.org/2013633003/diff/1/content/browser/download/sa...
> content/browser/download/save_package.cc:351: if (MHTMLGeneratorOptionFlag ==
> "skip_nostore_main") {
> hm, wish this could be a constant that's shared in the about_flags.cc as well.

Done.

[Dmitry Titov, 2016-05-24 23:29:57]

dimich@chromium.org changed reviewers:
+ creis@chromium.org

[Dmitry Titov, 2016-05-24 23:29:57]

+ creis@ for OWNERS look in content/

[Charlie Reis, 2016-05-25 00:01:33]

content/ LGTM with nits, though there's a failing about:flags check in the try
jobs.

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
File content/public/common/mhtml_generation_params.cc (right):

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
content/public/common/mhtml_generation_params.cc:15: 
nit: No blank line needed here.

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
content/public/common/mhtml_generation_params.cc:17: std::string
MHTMLGeneratorOptionFlag =
nit: This should start lowercase.

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
content/public/common/mhtml_generation_params.cc:30: 
nit: No blank line needed here.

[Dmitry Titov, 2016-05-25 00:27:14]

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
File content/public/common/mhtml_generation_params.cc (right):

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
content/public/common/mhtml_generation_params.cc:15: 
On 2016/05/25 00:01:33, Charlie Reis wrote:
> nit: No blank line needed here.

Done.

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
content/public/common/mhtml_generation_params.cc:17: std::string
MHTMLGeneratorOptionFlag =
On 2016/05/25 00:01:33, Charlie Reis wrote:
> nit: This should start lowercase.

Done.

https://codereview.chromium.org/2013633003/diff/40001/content/public/common/m...
content/public/common/mhtml_generation_params.cc:30: 
On 2016/05/25 00:01:33, Charlie Reis wrote:
> nit: No blank line needed here.

Done.

[Dmitry Titov, 2016-05-25 00:28:06]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-05-25 00:28:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dewittj@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2013633003/#ps60001
(title: "feedback + fixed test")

[commit-bot: I haz the power, 2016-05-25 00:29:03]

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

[commit-bot: I haz the power, 2016-05-25 00:40:21]

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

[commit-bot: I haz the power, 2016-05-25 00:40:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dmitry Titov, 2016-05-25 00:40:39]

The CQ bit was unchecked by dimich@chromium.org

[Dmitry Titov, 2016-05-25 00:42:14]

+ asvitkine@chromium.org for histograms.xml

[Dmitry Titov, 2016-05-25 00:44:04]

dimich@chromium.org changed reviewers:
+ isherman@chromium.org

[Dmitry Titov, 2016-05-25 00:44:05]

+ isherman@ for histograms.xml (asvitkine seems ot be OOO)

[Ilya Sherman, 2016-05-25 01:36:16]

histograms.xml lgtm

[Dmitry Titov, 2016-05-25 07:13:50]

The CQ bit was checked by dimich@chromium.org

[commit-bot: I haz the power, 2016-05-25 07:14:08]

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

[commit-bot: I haz the power, 2016-05-25 07:18:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-25 07:19:49]

Description was changed from

==========
Adding a command-line flag to set MHTML generation options.

It is a content-level flag (since MHTML generation is a content feature) and
initial
set of options is to follow standard cache-control: no-save option to avoid
saving MHTML files with potentially private/transient information.

BUG=614505
==========

to

==========
Adding a command-line flag to set MHTML generation options.

It is a content-level flag (since MHTML generation is a content feature) and
initial
set of options is to follow standard cache-control: no-save option to avoid
saving MHTML files with potentially private/transient information.

BUG=614505

Committed: https://crrev.com/0710ba43dca3d29b5658e8aa221ac7adc6e3cfb5
Cr-Commit-Position: refs/heads/master@{#395820}
==========

[commit-bot: I haz the power, 2016-05-25 07:19:51]

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