Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

This patch preserves the existing behavior for other creators of MHTML, the extension API and the 'save as' dialog (which is behind a flag).

BUG=604413
Committed: https://crrev.com/3287e2051f82e1aa8c5daab8ed19e2a17a2b07e2
Cr-Commit-Position: refs/heads/master@{#388638}

[dewittj, 2016-04-18 16:54:42]

Description was changed from

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

BUG=
==========

to

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

BUG=604413
==========

[dewittj, 2016-04-18 16:54:42]

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

[dewittj, 2016-04-18 21:01:26]

dimich: Wondering if this seems like the right way to plumb this option through
to chrome/browser.  What do you think?

[dewittj, 2016-04-19 20:15:23]

ping

[Dmitry Titov, 2016-04-19 22:45:46]

lgtm with a nit:

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

https://codereview.chromium.org/1899803002/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:122: EXPECT_THAT(mhtml,
ContainsRegex("Content-Transfer-Encoding: base64"));
Is there a matcher that can just check if a string Contains(substring)?

[dewittj, 2016-04-19 22:46:41]

Patchset #1 (id:1) has been deleted

[dewittj, 2016-04-19 22:47:13]

Patchset #1 (id:20001) has been deleted

[dewittj, 2016-04-19 22:47:24]

Patchset #1 (id:40001) has been deleted

[dewittj, 2016-04-19 23:21:59]

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

[dewittj, 2016-04-19 23:24:40]

nasko: PTAL; you seem to have the broadest coverage here, ptal at the whole
patch if you don't mind.  If you prefer someone else review this, let me know.

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

https://codereview.chromium.org/1899803002/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:122: EXPECT_THAT(mhtml,
ContainsRegex("Content-Transfer-Encoding: base64"));
On 2016/04/19 22:45:46, Dmitry Titov wrote:
> Is there a matcher that can just check if a string Contains(substring)?

Done.

[dewittj, 2016-04-20 14:32:37]

Description was changed from

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

BUG=604413
==========

to

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

This patch preserves the existing behavior for other creators of MHTML, the
extension API and the 'save as' dialog (which is behind a flag).

BUG=604413
==========

[nasko, 2016-04-20 17:01:15]

Looks good overall, just some minor comments.

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

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:112:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateNonBinaryMHTMLWithImage) {
Please add a comment describing what this case is testing.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:122:
ASSERT_TRUE(base::ReadFileToString(path, &mhtml));
Can we also check file size? It is done in other test cases.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:128:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateBinaryMHTML) {
Add a comment here too.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:128:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateBinaryMHTML) {
Be consistent with naming. If the previous test case is named "WithImage", so
should this one as the difference is binary vs non-binary, not "with image" vs
"without image".

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:342: saved_main_file_path_, /*
use_binary_encoding = */ false,
nit: Comment for the name of parameter should go after the value.

https://codereview.chromium.org/1899803002/diff/80001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1899803002/diff/80001/content/public/browser/...
content/public/browser/web_contents.h:545: bool use_binary_encoding,
Please add a comment about what the new parameter means and that it is not
cross-browser interoperable. The content/public/ API is used by other browsers,
so we should treat comments here as public docs for other devs.

[dewittj, 2016-04-20 20:20:58]

dewittj@chromium.org changed reviewers:
+ michaelbai@chromium.org, rdevlin.cronin@chromium.org

[dewittj, 2016-04-20 20:20:59]

rdevlin.cronin: page_capture_api.cc
michaelbai: aw_contents.cc

PTAL, thanks.

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

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:112:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateNonBinaryMHTMLWithImage) {
On 2016/04/20 17:01:15, nasko (slow) wrote:
> Please add a comment describing what this case is testing.

Done.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:122:
ASSERT_TRUE(base::ReadFileToString(path, &mhtml));
On 2016/04/20 17:01:15, nasko (slow) wrote:
> Can we also check file size? It is done in other test cases.

Done.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:128:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateBinaryMHTML) {
On 2016/04/20 17:01:15, nasko (slow) wrote:
> Be consistent with naming. If the previous test case is named "WithImage", so
> should this one as the difference is binary vs non-binary, not "with image" vs
> "without image".

Done.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:342: saved_main_file_path_, /*
use_binary_encoding = */ false,
On 2016/04/20 17:01:15, nasko (slow) wrote:
> nit: Comment for the name of parameter should go after the value.

I will make the change, but there isn't anywhere in the style guide that
suggests this, in fact the only example has preceding (see Function Argument
Comments).

https://codereview.chromium.org/1899803002/diff/80001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1899803002/diff/80001/content/public/browser/...
content/public/browser/web_contents.h:545: bool use_binary_encoding,
On 2016/04/20 17:01:15, nasko (slow) wrote:
> Please add a comment about what the new parameter means and that it is not
> cross-browser interoperable. The content/public/ API is used by other
browsers,
> so we should treat comments here as public docs for other devs.

Done.

[nasko, 2016-04-20 22:22:38]

content/ LGTM with a couple of nits.

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1899803002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:342: saved_main_file_path_, /*
use_binary_encoding = */ false,
On 2016/04/20 20:20:59, dewittj wrote:
> On 2016/04/20 17:01:15, nasko (slow) wrote:
> > nit: Comment for the name of parameter should go after the value.
> 
> I will make the change, but there isn't anywhere in the style guide that
> suggests this, in fact the only example has preceding (see Function Argument
> Comments).

I'm going with consistency across code in content/, especially prevalent in IPC
message files. Example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

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

https://codereview.chromium.org/1899803002/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:63: int64_t
readFileSizeFromDisk(base::FilePath path) {
Method name should start with Capital letter.

https://codereview.chromium.org/1899803002/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1899803002/diff/100001/content/public/browser...
content/public/browser/web_contents.h:545: // 'base64'.  Binary encoding is
known to have interoperability issues and so
nit: s/so//

[Devlin, 2016-04-20 22:29:45]

extensions lgtm

[michaelbai, 2016-04-20 22:30:04]

lgtm

[dewittj, 2016-04-20 23:15:19]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-04-20 23:15:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
michaelbai@chromium.org, rdevlin.cronin@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1899803002/#ps120001
(title: "Final nits.")

[commit-bot: I haz the power, 2016-04-20 23:16:49]

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

[commit-bot: I haz the power, 2016-04-21 00:33:37]

Description was changed from

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

This patch preserves the existing behavior for other creators of MHTML, the
extension API and the 'save as' dialog (which is behind a flag).

BUG=604413
==========

to

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

This patch preserves the existing behavior for other creators of MHTML, the
extension API and the 'save as' dialog (which is behind a flag).

BUG=604413
==========

[commit-bot: I haz the power, 2016-04-21 00:33:38]

Committed patchset #4 (id:120001)

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

Description was changed from

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

This patch preserves the existing behavior for other creators of MHTML, the
extension API and the 'save as' dialog (which is behind a flag).

BUG=604413
==========

to

==========
Offline Pages: Use 'binary encoding' to create MHTML, instead of base64.

This results in a 33% space savings for images stored alongside MHTML.

This patch preserves the existing behavior for other creators of MHTML, the
extension API and the 'save as' dialog (which is behind a flag).

BUG=604413

Committed: https://crrev.com/3287e2051f82e1aa8c5daab8ed19e2a17a2b07e2
Cr-Commit-Position: refs/heads/master@{#388638}
==========

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

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