Introduces a new MHTML generation parameter specifying different behvaior for cache-control headers.

Introduces a new MHTML generation parameter specifying different behvaior for cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/6dc5747a6c25cc8ea1d034503e9a33bcf5dd0009
Cr-Commit-Position: refs/heads/master@{#394013}

[dewittj, 2016-05-11 20:18:36]

Description was changed from

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
==========

to

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dewittj, 2016-05-11 22:32:12]

Patchset #5 (id:80001) has been deleted

[dewittj, 2016-05-12 00:43:21]

Patchset #1 (id:1) has been deleted

[dewittj, 2016-05-12 00:43:28]

Patchset #1 (id:20001) has been deleted

[dewittj, 2016-05-12 00:43:41]

Patchset #1 (id:40001) has been deleted

[dewittj, 2016-05-12 00:43:44]

Patchset #1 (id:60001) has been deleted

[dewittj, 2016-05-12 00:44:01]

Patchset #1 (id:90001) has been deleted

[dewittj, 2016-05-12 00:44:03]

Patchset #1 (id:110001) has been deleted

[dewittj, 2016-05-12 00:44:13]

Patchset #1 (id:130001) has been deleted

[dewittj, 2016-05-12 00:45:35]

Description was changed from

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dewittj, 2016-05-12 00:45:35]

dewittj@chromium.org changed reviewers:
+ dcheng@chromium.org, lukasza@chromium.org

[dewittj, 2016-05-12 00:47:42]

dewittj@chromium.org changed reviewers:
- dcheng@chromium.org

[dewittj, 2016-05-12 00:47:42]

dewittj@chromium.org changed reviewers:
- dcheng@chromium.org

[dewittj, 2016-05-12 00:47:43]

łukasza: Here is a first pass at making the cache control filter work on the
main frame only.  What do you think of the strategy/organization?  Most of the
work is done in WebFrameSerializer and the rest is plumbing.

[dewittj, 2016-05-12 00:47:43]

łukasza: Here is a first pass at making the cache control filter work on the
main frame only.  What do you think of the strategy/organization?  Most of the
work is done in WebFrameSerializer and the rest is plumbing.

[dewittj, 2016-05-12 15:53:04]

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

[dewittj, 2016-05-12 15:53:18]

+fgorski - would you mind reading this as a sanity check?

[dewittj, 2016-05-12 16:42:59]

Patchset #4 (id:210001) has been deleted

[dewittj, 2016-05-12 16:43:18]

Patchset #3 (id:190001) has been deleted

[dewittj, 2016-05-12 16:43:29]

Patchset #3 (id:230001) has been deleted

[dewittj, 2016-05-12 16:43:39]

Patchset #2 (id:170001) has been deleted

[Łukasz Anforowicz, 2016-05-12 18:56:49]

Overall shape / direction looks good.  I’ve left some comments, but nothing
major.  This CL will be a great first step that lays foundations for thinking
about handling no-store of subframes and subresources.

I agree with exposing no-store handling as an extra option for //content public
API;  I am not sure if we need to expose it at the current granularity, but I
think we can land the CL with the current state of MHTMLCacheControlPolicy enum
and we can always revisit if needed later, when adding handling of no-store for
subresources and subframes.

I think we need to hide a few things away from //content’s public API + tweak
some things to obey layering.  I am also not convinced if we need increased
granularity of MHTMLSerializationResult at this moment - will the higher layer
need to distinguish failure-to-write-to-file-or-disk-full VS
page-said-no-store-please?  In case of Offline Pages in both cases this would be
a silent failure from user perspective, right (i.e. no difference in UX for
these 2 failure modes)?

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

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:164: // First, generate
MHTML without specifying the new policy.
nit: // Generate MHTML without specifying FAIL_FOR_NO_STORE_MAIN_FRAME policy.

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:185: // First, generate
MHTML without specifying the new policy.
nit: // Generate MHTML specifying FAIL_FOR_NO_STORE_MAIN_FRAME policy.

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
content/browser/download/save_package.cc:29: #include
"content/browser/download/mhtml_generation_manager.h"
Is the new include needed?  I thought that save_package.cc is implemented on top
of the public WebContents API and doesn't directly talk to
MHTMLGenerationManager, but maybe I am misremembering.

BTW: Do we need to include content/public/common/mhtml_generation.h (for
MHTMLGenerationParams usage you've added below)?

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:7: #include <type_traits>
I don't think this is needed?

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:14: #include <type_traits>
I don't think this is needed?

https://codereview.chromium.org/1947263004/diff/250001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:24: #include
"content/browser/download/mhtml_generation_manager.h"
Did you want to include content/public/common/mhtml_generation.h instead?

BTW: I think that forward declaration will also work (and AFAIK is preferable by
Chromium style guide).

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:10: #include
"third_party/WebKit/public/web/WebFrameSerializer.h"
content/common/DEPS says: No inclusion of WebKit from the browser, other than
strictly enum/POD, header-only types, and some selected common code. 
WebFrameSerializer.h doesn't match this description.

I think either 1) the enums need to be moved to a separate header under
third_party/WebKit/public/web or 2) you need separate enums for content-layer (+
static_asserts somewhere that values are the same as in WebKit counterparts -
similar to third_party/WebKit/Source/web/AssertMatchingEnums.cpp?).

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:23: // |LAST| is used in
content/common/media/midi_messages.h with
Copy&paste issue - did you mean
content/public/common/common_param_traits_macros.h?

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:29: enum class MHTMLSerializationResult
{
I didn't see a place where this enum is used in //content's public API - I think
the highest this enum is used in the layering stack is
//content/browser/download/mhtml_generation_manager.h.  Did I miss anything? 
Can we hide this enum away from //content's public API?

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:33: FILE_WRITE_FAILED_IN_FOOTER,
Is there an existing code that needs to distinguish the 3 cases?  Or can this be
collapsed to a single FILE_WRITE_FAILED?  AFAICT we only really distinguish
between OK and not-OK (in MHTMLGenerationManager::OnSerializeAsMHTMLResponse at
https://codereview.chromium.org/1947263004/patch/250001/260005)

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:63:
blink::WebFrameSerializer::CacheControlPolicy toCacheControlPolicy(
Can this be hidden away from //content's public API surface?  Does the embedded
of //content ever want/can access Blink layer?

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

https://codereview.chromium.org/1947263004/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4910: bool success =
WebFrameSerializer::generateMHTMLHeader(
Hmmm... conveying MHTMLSerializationResult::NO_STORE_MAIN_FRAME by a bool result
from WebFrameSerializer::generateMHTMLHeader seems wrong (especially given that
a comment for WebFrameSerializer::generateMHTMLHeader doesn't talk about this
semantics at all).

We could change the return type of WebFrameSerializer::generateMHTMLHeader from
bool to an enum, but maybe handling of FAIL_FOR_NO_STORE_MAIN_FRAME can simply
happen here (i.e. if we expose a new bool
WebLocalFrame::cacheControlContainsNoCache() method).  WDYT?

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:149:
response.httpHeaderField(pragmaHeader)).containsNoStore;
Could this be written as:
return response.cacheControlContainsNoStore();
?

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameSerializer.h (left):

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:50: // Generates and returns
an MHTML header.
nit: This should describe the new return value / talk about returning the header
contents via the last parameter.

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:53: // FIXME: Add more
policies for subframes and subresources.
nit: I've been told by other reviewers to prefer TODO(alias) + to refer to a bug
:-)

[dewittj, 2016-05-12 22:58:02]

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

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:164: // First, generate
MHTML without specifying the new policy.
On 2016/05/12 18:56:48, Łukasz Anforowicz wrote:
> nit: // Generate MHTML without specifying FAIL_FOR_NO_STORE_MAIN_FRAME policy.

Done.

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:185: // First, generate
MHTML without specifying the new policy.
On 2016/05/12 18:56:48, Łukasz Anforowicz wrote:
> nit: // Generate MHTML specifying FAIL_FOR_NO_STORE_MAIN_FRAME policy.

Done.

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/downlo...
content/browser/download/save_package.cc:29: #include
"content/browser/download/mhtml_generation_manager.h"
On 2016/05/12 18:56:48, Łukasz Anforowicz wrote:
> Is the new include needed?  I thought that save_package.cc is implemented on
top
> of the public WebContents API and doesn't directly talk to
> MHTMLGenerationManager, but maybe I am misremembering.
> 
> BTW: Do we need to include content/public/common/mhtml_generation.h (for
> MHTMLGenerationParams usage you've added below)?
Done.

Yes, I went through quite a few revisions here, and should change this to
mhtml_generation.h

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:7: #include <type_traits>
On 2016/05/12 18:56:48, Łukasz Anforowicz wrote:
> I don't think this is needed?

Done.

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:14: #include <type_traits>
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> I don't think this is needed?

Done.

https://codereview.chromium.org/1947263004/diff/250001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:24: #include
"content/browser/download/mhtml_generation_manager.h"
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> Did you want to include content/public/common/mhtml_generation.h instead?
> 
> BTW: I think that forward declaration will also work (and AFAIK is preferable
by
> Chromium style guide).

Done.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:10: #include
"third_party/WebKit/public/web/WebFrameSerializer.h"
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> content/common/DEPS says: No inclusion of WebKit from the browser, other than
> strictly enum/POD, header-only types, and some selected common code. 
> WebFrameSerializer.h doesn't match this description.
> 
> I think either 1) the enums need to be moved to a separate header under
> third_party/WebKit/public/web or 2) you need separate enums for content-layer
(+
> static_asserts somewhere that values are the same as in WebKit counterparts -
> similar to third_party/WebKit/Source/web/AssertMatchingEnums.cpp?).

I actually did both.  WDYT about how this looks?

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:23: // |LAST| is used in
content/common/media/midi_messages.h with
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> Copy&paste issue - did you mean
> content/public/common/common_param_traits_macros.h?

Done.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:29: enum class MHTMLSerializationResult
{
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> I didn't see a place where this enum is used in //content's public API - I
think
> the highest this enum is used in the layering stack is
> //content/browser/download/mhtml_generation_manager.h.  Did I miss anything? 
> Can we hide this enum away from //content's public API?

I just got rid of the result since we are not using it yet; that will come in a
later CL if we end up needing for offline pages.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:33: FILE_WRITE_FAILED_IN_FOOTER,
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> Is there an existing code that needs to distinguish the 3 cases?  Or can this
be
> collapsed to a single FILE_WRITE_FAILED?  AFAICT we only really distinguish
> between OK and not-OK (in MHTMLGenerationManager::OnSerializeAsMHTMLResponse
at
> https://codereview.chromium.org/1947263004/patch/250001/260005)
obsolete.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:63:
blink::WebFrameSerializer::CacheControlPolicy toCacheControlPolicy(
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> Can this be hidden away from //content's public API surface?  Does the
embedded
> of //content ever want/can access Blink layer?

Done.

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

https://codereview.chromium.org/1947263004/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4910: bool success =
WebFrameSerializer::generateMHTMLHeader(
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> Hmmm... conveying MHTMLSerializationResult::NO_STORE_MAIN_FRAME by a bool
result
> from WebFrameSerializer::generateMHTMLHeader seems wrong (especially given
that
> a comment for WebFrameSerializer::generateMHTMLHeader doesn't talk about this
> semantics at all).
> 
> We could change the return type of WebFrameSerializer::generateMHTMLHeader
from
> bool to an enum, but maybe handling of FAIL_FOR_NO_STORE_MAIN_FRAME can simply
> happen here (i.e. if we expose a new bool
> WebLocalFrame::cacheControlContainsNoCache() method).  WDYT?

Since I don't expose more granular errors any more, it doesn't make as much
sense to have an enum for this particular thing.  I returned bool as
success/fail - the only thing that currently makes it fail today is this cache
control header.

I updated the comment; does this look better to you now?

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:149:
response.httpHeaderField(pragmaHeader)).containsNoStore;
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> Could this be written as:
> return response.cacheControlContainsNoStore();
> ?

No, that method is not const, since it caches the results of parsing the cache
control header.

I was unable to figure out a way to get a non-const ResourceResponse; if you
have ideas I'd be glad to hear!

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameSerializer.h (left):

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:50: // Generates and returns
an MHTML header.
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> nit: This should describe the new return value / talk about returning the
header
> contents via the last parameter.

Done.

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameSerializer.h:53: // FIXME: Add more
policies for subframes and subresources.
On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> nit: I've been told by other reviewers to prefer TODO(alias) + to refer to a
bug
> :-)

Done.

[Łukasz Anforowicz, 2016-05-12 23:56:27]

lgtm.  I hope that Blink owners can suggest how to improve or avoid header
parsing.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:10: #include
"third_party/WebKit/public/web/WebFrameSerializer.h"
On 2016/05/12 22:58:01, dewittj wrote:
> On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > content/common/DEPS says: No inclusion of WebKit from the browser, other
than
> > strictly enum/POD, header-only types, and some selected common code. 
> > WebFrameSerializer.h doesn't match this description.
> > 
> > I think either 1) the enums need to be moved to a separate header under
> > third_party/WebKit/public/web or 2) you need separate enums for
content-layer
> (+
> > static_asserts somewhere that values are the same as in WebKit counterparts
-
> > similar to third_party/WebKit/Source/web/AssertMatchingEnums.cpp?).
> 
> I actually did both.  WDYT about how this looks?

Looks good - thanks.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:29: enum class MHTMLSerializationResult
{
On 2016/05/12 22:58:01, dewittj wrote:
> On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > I didn't see a place where this enum is used in //content's public API - I
> think
> > the highest this enum is used in the layering stack is
> > //content/browser/download/mhtml_generation_manager.h.  Did I miss anything?

> > Can we hide this enum away from //content's public API?
> 
> I just got rid of the result since we are not using it yet; that will come in
a
> later CL if we end up needing for offline pages.

Acknowledged.

https://codereview.chromium.org/1947263004/diff/250001/content/public/common/...
content/public/common/mhtml_generation.h:33: FILE_WRITE_FAILED_IN_FOOTER,
On 2016/05/12 22:58:01, dewittj wrote:
> On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > Is there an existing code that needs to distinguish the 3 cases?  Or can
this
> be
> > collapsed to a single FILE_WRITE_FAILED?  AFAICT we only really distinguish
> > between OK and not-OK (in MHTMLGenerationManager::OnSerializeAsMHTMLResponse
> at
> > https://codereview.chromium.org/1947263004/patch/250001/260005)
> obsolete.

Acknowledged.

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

https://codereview.chromium.org/1947263004/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4910: bool success =
WebFrameSerializer::generateMHTMLHeader(
On 2016/05/12 22:58:01, dewittj wrote:
> On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > Hmmm... conveying MHTMLSerializationResult::NO_STORE_MAIN_FRAME by a bool
> result
> > from WebFrameSerializer::generateMHTMLHeader seems wrong (especially given
> that
> > a comment for WebFrameSerializer::generateMHTMLHeader doesn't talk about
this
> > semantics at all).
> > 
> > We could change the return type of WebFrameSerializer::generateMHTMLHeader
> from
> > bool to an enum, but maybe handling of FAIL_FOR_NO_STORE_MAIN_FRAME can
simply
> > happen here (i.e. if we expose a new bool
> > WebLocalFrame::cacheControlContainsNoCache() method).  WDYT?
> 
> Since I don't expose more granular errors any more, it doesn't make as much
> sense to have an enum for this particular thing.  I returned bool as
> success/fail - the only thing that currently makes it fail today is this cache
> control header.
> 
> I updated the comment; does this look better to you now?

Yes - looks good.  Thanks.

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:149:
response.httpHeaderField(pragmaHeader)).containsNoStore;
On 2016/05/12 22:58:01, dewittj wrote:
> On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > Could this be written as:
> > return response.cacheControlContainsNoStore();
> > ?
> 
> No, that method is not const, since it caches the results of parsing the cache
> control header.
> 
> I was unable to figure out a way to get a non-const ResourceResponse; if you
> have ideas I'd be glad to hear!

Oh, indeed - thanks for the explanation.  And I see that this constness problem
also happens in the Blink's core layer (i.e. blink::DocumentLoader::response()
returns a const-ref to ResourceResponse).

I hope that Blink owners can help here - it feels weird (wrong?) to me to repeat
the parsing call here, but maybe it is okay, given constness constraints?

[dewittj, 2016-05-13 15:55:31]

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

[dewittj, 2016-05-13 15:56:45]

dewittj@chromium.org changed reviewers:
+ michaelbai@chroumium.org
- sgurun@chromium.org

[dewittj, 2016-05-13 15:59:34]

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

[dewittj, 2016-05-13 16:00:12]

dewittj@chromium.org changed reviewers:
+ michaelbai@chromium.org
- fgorski@chromium.org, michaelbai@chroumium.org

[dewittj, 2016-05-13 16:02:20]

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

[dewittj, 2016-05-13 16:03:22]

dewittj@chromium.org changed reviewers:
- asanka@chromium.org

[dewittj, 2016-05-13 16:08:21]

Hi everyone, PTAL.

michaelbai:
  android_webview/native/aw_contents.cc
nick:
  content/browser/download/mhtml_generation_browsertest.cc
  content/browser/download/mhtml_generation_manager.cc
  content/browser/download/mhtml_generation_manager.h
  content/browser/download/save_package.cc
  content/browser/web_contents/web_contents_impl.cc
  content/browser/web_contents/web_contents_impl.h
  content/child/assert_matching_enums.cc
  content/public/browser/web_contents.h
  content/public/common/mhtml_generation.cc
  content/public/common/mhtml_generation.h
  content/renderer/render_frame_impl.cc
rdevlin.cronin:
  chrome/browser/extensions/api/page_capture/page_capture_api.cc
dcheng:
  content/common/frame_messages.h
  content/public/common/common_param_traits_macros.h
  third_party/WebKit/Source/web/WebFrameSerializer.cpp
  third_party/WebKit/public/web/WebFrameSerializer.h
  third_party/WebKit/public/web/WebFrameSerializerCacheControlPolicy.h

[Devlin, 2016-05-13 16:14:28]

extensions lgtm

https://codereview.chromium.org/1947263004/diff/270001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/270001/content/public/common/...
content/public/common/mhtml_generation.h:45: #endif  //
CONTENT_PUBLIC_COMMON_MHTML_GENERATION__
nit: this should be "#endif  // CONTENT_PUBLIC_COMMON_MHTML_GENERATION_H_"
(you're missing the ending H)

[dcheng, 2016-05-13 17:48:33]

dcheng@chromium.org changed reviewers:
+ hiroshige@chromium.org, toyoshim@chromium.org

[dcheng, 2016-05-13 17:48:34]

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:149:
response.httpHeaderField(pragmaHeader)).containsNoStore;
On 2016/05/12 at 23:56:27, Łukasz Anforowicz wrote:
> On 2016/05/12 22:58:01, dewittj wrote:
> > On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > > Could this be written as:
> > > return response.cacheControlContainsNoStore();
> > > ?
> > 
> > No, that method is not const, since it caches the results of parsing the
cache
> > control header.
> > 
> > I was unable to figure out a way to get a non-const ResourceResponse; if you
> > have ideas I'd be glad to hear!
> 
> Oh, indeed - thanks for the explanation.  And I see that this constness
problem also happens in the Blink's core layer (i.e.
blink::DocumentLoader::response() returns a const-ref to ResourceResponse).
> 
> I hope that Blink owners can help here - it feels weird (wrong?) to me to
repeat the parsing call here, but maybe it is okay, given constness constraints?

+toyoshim, +hiroshige: any ideas on how to improve this so we don't need to
manually reparse here?

[michaelbai, 2016-05-13 17:51:29]

android_webview LGTM

[hiroshige, 2016-05-13 18:01:52]

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

https://codereview.chromium.org/1947263004/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:149:
response.httpHeaderField(pragmaHeader)).containsNoStore;
On 2016/05/13 17:48:33, dcheng wrote:
> On 2016/05/12 at 23:56:27, Łukasz Anforowicz wrote:
> > On 2016/05/12 22:58:01, dewittj wrote:
> > > On 2016/05/12 18:56:49, Łukasz Anforowicz wrote:
> > > > Could this be written as:
> > > > return response.cacheControlContainsNoStore();
> > > > ?
> > > 
> > > No, that method is not const, since it caches the results of parsing the
> cache
> > > control header.
> > > 
> > > I was unable to figure out a way to get a non-const ResourceResponse; if
you
> > > have ideas I'd be glad to hear!
> > 
> > Oh, indeed - thanks for the explanation.  And I see that this constness
> problem also happens in the Blink's core layer (i.e.
> blink::DocumentLoader::response() returns a const-ref to ResourceResponse).
> > 
> > I hope that Blink owners can help here - it feels weird (wrong?) to me to
> repeat the parsing call here, but maybe it is okay, given constness
constraints?
> 
> +toyoshim, +hiroshige: any ideas on how to improve this so we don't need to
> manually reparse here?

How about making ResourceResponse::cacheControlContainsNoCache() and similar
methods to be const by making |ResourceResponse::m_cacheControlHeader| mutable,
and calling response.cacheControlContainsNoStore() here?

[dewittj, 2016-05-13 19:43:48]

Thanks for the advice!  I made the cache control accessors const over in
https://codereview.chromium.org/1980673002/

https://codereview.chromium.org/1947263004/diff/270001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/270001/content/public/common/...
content/public/common/mhtml_generation.h:45: #endif  //
CONTENT_PUBLIC_COMMON_MHTML_GENERATION__
On 2016/05/13 16:14:28, Devlin wrote:
> nit: this should be "#endif  // CONTENT_PUBLIC_COMMON_MHTML_GENERATION_H_"
> (you're missing the ending H)

Done.

[ncarter (slow), 2016-05-13 20:06:31]

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

https://codereview.chromium.org/1947263004/diff/290001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:92: // Whether to use
binary encoding while generating MHTML.
Update this comment.

https://codereview.chromium.org/1947263004/diff/290001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/290001/content/public/common/...
content/public/common/mhtml_generation.h:6: #define
CONTENT_PUBLIC_COMMON_MHTML_GENERATION_H_
Let's call this file mhtml_generation_params, since the most common naming
convention (by far) is to name files after the classes they contain. Make sure
you fix-up the #define guard here too.

[dewittj, 2016-05-13 20:33:13]

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

https://codereview.chromium.org/1947263004/diff/290001/content/browser/downlo...
content/browser/download/mhtml_generation_manager.cc:92: // Whether to use
binary encoding while generating MHTML.
On 2016/05/13 20:06:31, ncarter wrote:
> Update this comment.

Done.

https://codereview.chromium.org/1947263004/diff/290001/content/public/common/...
File content/public/common/mhtml_generation.h (right):

https://codereview.chromium.org/1947263004/diff/290001/content/public/common/...
content/public/common/mhtml_generation.h:6: #define
CONTENT_PUBLIC_COMMON_MHTML_GENERATION_H_
On 2016/05/13 20:06:31, ncarter wrote:
> Let's call this file mhtml_generation_params, since the most common naming
> convention (by far) is to name files after the classes they contain. Make sure
> you fix-up the #define guard here too.

Done.

Just discovered tools/git/move_source_file.py.  Genius!

[ncarter (slow), 2016-05-13 21:15:49]

lgtm

[dewittj, 2016-05-16 16:20:18]

dcheng: ping?

[dcheng, 2016-05-16 17:36:14]

It feels weird to have generateMHTMLHeader doing the policy check.

At the same time, I don't currently have a better suggestion. But maybe
something to think about as WebFrameSerializerCacheControlPolicy evolves.

RS LGTM since lukasza@ already looked over the Blink bits.

[dewittj, 2016-05-16 17:45:25]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-05-16 17:45:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelbai@chromium.org, rdevlin.cronin@chromium.org, lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/1947263004/#ps330001
(title: "Rebase onto origin/master.")

[commit-bot: I haz the power, 2016-05-16 17:46:04]

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

[commit-bot: I haz the power, 2016-05-16 23:46:20]

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

[commit-bot: I haz the power, 2016-05-16 23:46:22]

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

[dewittj, 2016-05-16 23:48:21]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2016-05-16 23:48:52]

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

[commit-bot: I haz the power, 2016-05-17 01:48:57]

Description was changed from

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-17 01:48:59]

Committed patchset #6 (id:330001)

[commit-bot: I haz the power, 2016-05-17 01:50:20]

Description was changed from

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduces a new MHTML generation parameter specifying different behvaior for
cache-control headers.

BUG=609566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/6dc5747a6c25cc8ea1d034503e9a33bcf5dd0009
Cr-Commit-Position: refs/heads/master@{#394013}
==========

[commit-bot: I haz the power, 2016-05-17 01:50:22]

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