Adds a feature to MHTML serialization that omits subframes and subresources marked no-store.

Adds a feature to MHTML serialization that omits subframes and subresources marked no-store.

Also integrates this new feature with the offline page archiver.  Note that because CSS is not considered a resource
when serializing, CSS will be covered separately.

BUG=609566
Committed: https://crrev.com/a8f8297de77fc3688e05b459f0cb6448209ad2ea
Cr-Commit-Position: refs/heads/master@{#395380}

[Dmitry Titov, 2016-05-17 22:47:10]

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

[Dmitry Titov, 2016-05-17 22:47:11]

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.h:90: virtual bool
shouldSkipURL(const KURL&)
ShouldSkipResourceWithURL is probably more clear.

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:177: bool checkNoStore =
cacheControlPolicy ==
WebFrameSerializerCacheControlPolicy::SkipAnyFrameOrResourceMarkedNoStore
nit: 'needToCheckNoStore'?

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameSerializer.h:70: BLINK_EXPORT static bool
frameShouldBeSerializedAsMHTML(
Add comment to the method.

[dewittj, 2016-05-17 23:15:09]

Description was changed from

==========
Check the cache-control policy on all subframes and subresources.

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

to

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.

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

[dewittj, 2016-05-17 23:43:26]

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

[dewittj, 2016-05-17 23:43:27]

dimich, łukasza: ptal

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.h:90: virtual bool
shouldSkipURL(const KURL&)
On 2016/05/17 22:47:10, Dmitry Titov wrote:
> ShouldSkipResourceWithURL is probably more clear.

Done.

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:177: bool checkNoStore =
cacheControlPolicy ==
WebFrameSerializerCacheControlPolicy::SkipAnyFrameOrResourceMarkedNoStore
On 2016/05/17 22:47:11, Dmitry Titov wrote:
> nit: 'needToCheckNoStore'?

Done.

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameSerializer.h:70: BLINK_EXPORT static bool
frameShouldBeSerializedAsMHTML(
On 2016/05/17 22:47:11, Dmitry Titov wrote:
> Add comment to the method.

Done.

[Dmitry Titov, 2016-05-18 00:18:47]

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

https://codereview.chromium.org/1977303003/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:246: 
it'd be nice to add a subresource test (css or image) but may be in a separate
CL if it is at all involved.

https://codereview.chromium.org/1977303003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:152: bool
cacheControlNoStoreHeaderPresent(WebLocalFrameImpl* webLocalFrameImpl)
It's probably better to just inline this function since it's only used once.

https://codereview.chromium.org/1977303003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:208:
DCHECK(frameShouldBeSerializedAsMHTML(webFrame, webDelegate));
These DCHECKS look complex and might not add any safety since it's mostly
(only?) tests that run debug build... I'd remove for clarity, but your call.

[dewittj, 2016-05-18 00:45:24]

Description was changed from

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.

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

to

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.  Note that
because CSS is not considered a resource
when serializing, CSS will be covered separately.

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

[Łukasz Anforowicz, 2016-05-18 17:27:25]

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

https://codereview.chromium.org/1977303003/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:202:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest,
Could you please also add a test that tries to open / navigate to the generated
mhtml file?  I am worried that your current solution results in dangling cid
URIs as described in https://crbug.com/570353.

I think it would be great if you could somehow reuse before-vs-after tests from
SavePageOriginalVsSavedComparisonTest test suite in
chrome/browser/download/save_page_browsertest.cc, but I understand that some
things won't work as-is:

1) TestOriginalVsSavedPage shuts down embedded_test_server before opening the
saved file.  This is done to make sure that the saved file is self-contained,
but in your scenario, you *do* want the no-store resources to be fetched again
from the network.

2) save_page_type parameter of TestOriginalVsSavedPage is not covering your
no-store flag.

I am not sure how to combine the various test flags nicely.  I think that maybe
introducing a test-only enum SaveTypeForTesting (MHTML, COMPLETE_HTML,
MHTML_WITH_NO_STORE) might work.  For the last enum value you wouldn't stop the
http test server + you could set MHTMLCacheControlPolicy as appropriate.

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

https://codereview.chromium.org/1977303003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4986: &delegate);
This skips generating the MHTML part for a subframe, but the parent frame will
continue emitting cid: URIs referring to the subframe (i.e. the subframe will
still be covered by the map in
MHTMLGenerationManager::Job::frame_tree_node_to_content_id_ and therefore will
be used by the parent frame when generating the parent frame's MHTML).

As I wrote in https://crbug.com/570353#c7, I hope that bottom-up generation in
MHTMLGenerationManager::Job can help here (right not MHTML is generated in the
order used by web_contents->ForEachFrame -
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:394: if
(m_delegate.shouldSkipResource(resource))
I wonder if we can skip processing a resource before a data copy takes place
(i.e. by tweaking addFontToResources so that it calls a [new] shouldAddResource
instead of shouldAddURL - this would return before copying data from
font->resourceBuffer into SharedBuffer).

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.h:96: virtual bool
shouldSkipResource(Resource*)
nit: I wonder we can avoid introducing a new method and instead change the
signature of the old method (so that it takes Resource* instead of KURL&).  Less
code = simpler and more readable code?

Hmmm... I guess what I am proposing requires some refactoring of
FrameSerializer.cpp to change FrameSerializer::shouldAddURL to something taking
a Resource* instead... if this is too big of a change for the current CL, then
maybe it can be done in a follow-up CL.

Also... I am not sure if what I am proposing would work for shouldAddURL(url)
call in FrameSerializer::serializeCSSStyleSheet (i.e. there is no Resource*
anywhere nearby in this case...).

WDYT?  Just an idea :-)

[dewittj, 2016-05-19 04:13:18]

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

https://codereview.chromium.org/1977303003/diff/60001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:246: 
On 2016/05/18 00:18:47, Dmitry Titov wrote:
> it'd be nice to add a subresource test (css or image) but may be in a separate
> CL if it is at all involved.

Done.

Note that CSS is not covered by this CL (I updated the description to match the
reality).

https://codereview.chromium.org/1977303003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:152: bool
cacheControlNoStoreHeaderPresent(WebLocalFrameImpl* webLocalFrameImpl)
On 2016/05/18 00:18:47, Dmitry Titov wrote:
> It's probably better to just inline this function since it's only used once.

To me it was confusing when written inline.  This makes it clear what the return
value depends on (no-store header present) and how to calculate that value.  Do
you mind if I leave this as-is?

https://codereview.chromium.org/1977303003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:208:
DCHECK(frameShouldBeSerializedAsMHTML(webFrame, webDelegate));
On 2016/05/18 00:18:47, Dmitry Titov wrote:
> These DCHECKS look complex and might not add any safety since it's mostly
> (only?) tests that run debug build... I'd remove for clarity, but your call.

Done.

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

https://codereview.chromium.org/1977303003/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:202:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest,
On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> Could you please also add a test that tries to open / navigate to the
generated
> mhtml file?
Done.

> I am worried that your current solution results in dangling cid
> URIs as described in https://crbug.com/570353.
It does indeed result in dangling CIDs.  I don't think this is a problem that
needs be fixed in this patch, since dangling CIDs are also present in other
MHTML generated by Chrome, and since this path for MHTML is only on Android and
is not being shared at present.  MHTML does not cause external protocol handler
requests for cid: on Android.

> 
> I think it would be great if you could somehow reuse before-vs-after tests
from
> SavePageOriginalVsSavedComparisonTest test suite in
> chrome/browser/download/save_page_browsertest.cc, but I understand that some
> things won't work as-is:
> 
> 1) TestOriginalVsSavedPage shuts down embedded_test_server before opening the
> saved file.  This is done to make sure that the saved file is self-contained,
> but in your scenario, you *do* want the no-store resources to be fetched again
> from the network.
In my particular scenario, I don't actually want the no-store resources to be
fetched at all, since I'm trying to create an archive independent of the
network.  It's not clear that fetching a no-store iframe anew if you are just
given this file is the right thing to do anyway. (cookie jar could be completely
different, for example.)

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

https://codereview.chromium.org/1977303003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4986: &delegate);
On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> This skips generating the MHTML part for a subframe, but the parent frame will
> continue emitting cid: URIs referring to the subframe (i.e. the subframe will
> still be covered by the map in
> MHTMLGenerationManager::Job::frame_tree_node_to_content_id_ and therefore will
> be used by the parent frame when generating the parent frame's MHTML).
> 
> As I wrote in https://crbug.com/570353#c7, I hope that bottom-up generation in
> MHTMLGenerationManager::Job can help here (right not MHTML is generated in the
> order used by web_contents->ForEachFrame -
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).

I'd prefer to leave this as-is and address generation order in a subsequent
patch.  Are you okay with that?

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:394: if
(m_delegate.shouldSkipResource(resource))
On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> I wonder if we can skip processing a resource before a data copy takes place
> (i.e. by tweaking addFontToResources so that it calls a [new]
shouldAddResource
> instead of shouldAddURL - this would return before copying data from
> font->resourceBuffer into SharedBuffer).

I am not sure how we could get the URL (or indeed the Resource) without fetching
a remote font.  Can I look into this in a future patch?

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.h:96: virtual bool
shouldSkipResource(Resource*)
On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> nit: I wonder we can avoid introducing a new method and instead change the
> signature of the old method (so that it takes Resource* instead of KURL&). 
Less
> code = simpler and more readable code?
> 
> Hmmm... I guess what I am proposing requires some refactoring of
> FrameSerializer.cpp to change FrameSerializer::shouldAddURL to something
taking
> a Resource* instead... if this is too big of a change for the current CL, then
> maybe it can be done in a follow-up CL.
> 
> Also... I am not sure if what I am proposing would work for shouldAddURL(url)
> call in FrameSerializer::serializeCSSStyleSheet (i.e. there is no Resource*
> anywhere nearby in this case...).
> 
> WDYT?  Just an idea :-)

Only other way that would work is if you had:
shouldSkipResourceWithURL(const KURL&, Resource*) where the Resource could be
nullptr.  It didn't seem very clean, especially since in the implementation
there was never a need for /both/ the URL and the Resource.

So I'd like to leave this as-is, perhaps better names are in order?  I'm open to
suggestions.

[dewittj, 2016-05-19 16:48:22]

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

[dewittj, 2016-05-19 16:48:39]

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

[dewittj, 2016-05-19 16:48:58]

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

[dewittj, 2016-05-19 16:50:21]

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

[dewittj, 2016-05-19 16:55:00]

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

[Łukasz Anforowicz, 2016-05-19 17:00:14]

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

https://codereview.chromium.org/1977303003/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:202:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest,
On 2016/05/19 04:13:17, dewittj wrote:
> On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > Could you please also add a test that tries to open / navigate to the
> generated
> > mhtml file?
> Done.
> 
> > I am worried that your current solution results in dangling cid
> > URIs as described in https://crbug.com/570353.
> It does indeed result in dangling CIDs.  I don't think this is a problem that
> needs be fixed in this patch, since dangling CIDs are also present in other
> MHTML generated by Chrome, and since this path for MHTML is only on Android
and
> is not being shared at present.  MHTML does not cause external protocol
handler
> requests for cid: on Android.
> 
Thanks for checking.  Since right now the no-store serialization flag is only
used by offline pages on Android, then I guess we can deal with dangling cid
URIs in a follow-up CL.

> > I think it would be great if you could somehow reuse before-vs-after tests
> from
> > SavePageOriginalVsSavedComparisonTest test suite in
> > chrome/browser/download/save_page_browsertest.cc, but I understand that some
> > things won't work as-is:
> > 
> > 1) TestOriginalVsSavedPage shuts down embedded_test_server before opening
the
> > saved file.  This is done to make sure that the saved file is
self-contained,
> > but in your scenario, you *do* want the no-store resources to be fetched
again
> > from the network.
> In my particular scenario, I don't actually want the no-store resources to be
> fetched at all, since I'm trying to create an archive independent of the
> network.  It's not clear that fetching a no-store iframe anew if you are just
> given this file is the right thing to do anyway. (cookie jar could be
completely
> different, for example.)
> 

Ok - thanks for explaining why we might still want to turn off the test server. 
Makes sense.

Thanks for adding the new tests.

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

https://codereview.chromium.org/1977303003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4986: &delegate);
On 2016/05/19 04:13:17, dewittj wrote:
> On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > This skips generating the MHTML part for a subframe, but the parent frame
will
> > continue emitting cid: URIs referring to the subframe (i.e. the subframe
will
> > still be covered by the map in
> > MHTMLGenerationManager::Job::frame_tree_node_to_content_id_ and therefore
will
> > be used by the parent frame when generating the parent frame's MHTML).
> > 
> > As I wrote in https://crbug.com/570353#c7, I hope that bottom-up generation
in
> > MHTMLGenerationManager::Job can help here (right not MHTML is generated in
the
> > order used by web_contents->ForEachFrame -
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).
> 
> I'd prefer to leave this as-is and address generation order in a subsequent
> patch.  Are you okay with that?

I am fine with that (given that MHTML saved by end-users should not be affected
by the no-store changes and you've checked that Android is gracefully handling
dangling CID URIs).

I assume that you are committing to working on this and landing a fix for the
next milestone (M-53)? :-)

BTW: I am not particularly attached to bottom-up generation option of fixing
this, but I do think that we need to do *something* about the CID behavior - it 
currently results in rather terrible experience for the desktop users who happen
to hit it.

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:394: if
(m_delegate.shouldSkipResource(resource))
On 2016/05/19 04:13:17, dewittj wrote:
> On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > I wonder if we can skip processing a resource before a data copy takes place
> > (i.e. by tweaking addFontToResources so that it calls a [new]
> shouldAddResource
> > instead of shouldAddURL - this would return before copying data from
> > font->resourceBuffer into SharedBuffer).
> 
> I am not sure how we could get the URL (or indeed the Resource) without
fetching
> a remote font.

You're right - fetching is unavoidable, but we can avoid serialization work
(which for images/fonts involves currently a memory allocation + copy, but in
theory could become more expensive in the future).

> Can I look into this in a future patch?

Hmmm... how about a relatively small change to:

1. Introduce a new method:
   bool FrameSerializer::shouldProcessResource(const Resource& resource)
   {
       return shouldAddURL(resource.url()) &&
              !m_delegate.shouldSkipResource(resource);
   }

2. Call this method instead of calling shouldAddURL inside:
   - FrameSerializer::addImageToResources
   - FrameSerializer::addFontToResources

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.h:96: virtual bool
shouldSkipResource(Resource*)
On 2016/05/19 04:13:17, dewittj wrote:
> On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > nit: I wonder we can avoid introducing a new method and instead change the
> > signature of the old method (so that it takes Resource* instead of KURL&). 
> Less
> > code = simpler and more readable code?
> > 
> > Hmmm... I guess what I am proposing requires some refactoring of
> > FrameSerializer.cpp to change FrameSerializer::shouldAddURL to something
> taking
> > a Resource* instead... if this is too big of a change for the current CL,
then
> > maybe it can be done in a follow-up CL.
> > 
> > Also... I am not sure if what I am proposing would work for
shouldAddURL(url)
> > call in FrameSerializer::serializeCSSStyleSheet (i.e. there is no Resource*
> > anywhere nearby in this case...).
> > 
> > WDYT?  Just an idea :-)
> 
> Only other way that would work is if you had:
> shouldSkipResourceWithURL(const KURL&, Resource*) where the Resource could be
> nullptr.  It didn't seem very clean, especially since in the implementation
> there was never a need for /both/ the URL and the Resource.
> 
> So I'd like to leave this as-is, perhaps better names are in order?  I'm open
to
> suggestions.

This is fine.  Thanks for looking into this.

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

https://codereview.chromium.org/1977303003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:37: int
global_request_id = 0;
Can this be made a static member of FindTrackingDelegate?

https://codereview.chromium.org/1977303003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:166:
&actual_number_of_frames));
This rather ugly code was written before web_contents()->GetAllFrames().size()
was available.  Can you use this (instead of ForEachFrame) and get rid of
increment method?

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

https://codereview.chromium.org/1977303003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4964: &delegate);
Do we still need this (this = |should_serialize_frame| variable + *public*
WebFrameSerializer::frameShouldBeSerializedAsMHTML method)?  It seems that you
are now handling no-store check within the serializer, in
WebFrameSerializer::generateMHTMLHeader and
WebFrameSerializer::generateMHTMLParts (which I think is great) so, checking
return value of generateMHTMLHeader and generateMHTMLParts should be sufficient?

https://codereview.chromium.org/1977303003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4972: if
(file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
Just double-checking: do we need to check here is generateMHTMLHeader returned
an empty, default WebData() - it seems to be returned this when frame should not
be serialized.

https://codereview.chromium.org/1977303003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: &delegate);
Same question as above - do we need to check if generateMHTMLParts returned an
empty WebData (and in this case we would need to set success=false)?

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:397: DLOG(ERROR) <<
"Adding " << url.getString() << " to resources!";
I assume this is temporary (i.e. for printf-style debugging) and can be removed
now?

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.h:90: virtual bool
shouldSkipResourceWithURL(const KURL&)
Nice name change.  Long time ago this was also used to check if a subframe
should be skipped, but right now (as the name change correctly highlights) this
is only used for subresources.

[dewittj, 2016-05-19 18:18:36]

Thanks, Łukasz.  PTAL.

Adding additional reviewers:
nick: content/
dcheng: webkit (or wait for łukasza to review then rs)

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

https://codereview.chromium.org/1977303003/diff/80001/content/browser/downloa...
content/browser/download/mhtml_generation_browsertest.cc:202:
IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest,
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> On 2016/05/19 04:13:17, dewittj wrote:
> > On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > > Could you please also add a test that tries to open / navigate to the
> > generated
> > > mhtml file?
> > Done.
> > 
> > > I am worried that your current solution results in dangling cid
> > > URIs as described in https://crbug.com/570353.
> > It does indeed result in dangling CIDs.  I don't think this is a problem
that
> > needs be fixed in this patch, since dangling CIDs are also present in other
> > MHTML generated by Chrome, and since this path for MHTML is only on Android
> and
> > is not being shared at present.  MHTML does not cause external protocol
> handler
> > requests for cid: on Android.
> > 
> Thanks for checking.  Since right now the no-store serialization flag is only
> used by offline pages on Android, then I guess we can deal with dangling cid
> URIs in a follow-up CL.
> 
> > > I think it would be great if you could somehow reuse before-vs-after tests
> > from
> > > SavePageOriginalVsSavedComparisonTest test suite in
> > > chrome/browser/download/save_page_browsertest.cc, but I understand that
some
> > > things won't work as-is:
> > > 
> > > 1) TestOriginalVsSavedPage shuts down embedded_test_server before opening
> the
> > > saved file.  This is done to make sure that the saved file is
> self-contained,
> > > but in your scenario, you *do* want the no-store resources to be fetched
> again
> > > from the network.
> > In my particular scenario, I don't actually want the no-store resources to
be
> > fetched at all, since I'm trying to create an archive independent of the
> > network.  It's not clear that fetching a no-store iframe anew if you are
just
> > given this file is the right thing to do anyway. (cookie jar could be
> completely
> > different, for example.)
> > 
> 
> Ok - thanks for explaining why we might still want to turn off the test
server. 
> Makes sense.
> 
> Thanks for adding the new tests.

Acknowledged.

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

https://codereview.chromium.org/1977303003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4986: &delegate);
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> On 2016/05/19 04:13:17, dewittj wrote:
> > On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > > This skips generating the MHTML part for a subframe, but the parent frame
> will
> > > continue emitting cid: URIs referring to the subframe (i.e. the subframe
> will
> > > still be covered by the map in
> > > MHTMLGenerationManager::Job::frame_tree_node_to_content_id_ and therefore
> will
> > > be used by the parent frame when generating the parent frame's MHTML).
> > > 
> > > As I wrote in https://crbug.com/570353#c7, I hope that bottom-up
generation
> in
> > > MHTMLGenerationManager::Job can help here (right not MHTML is generated in
> the
> > > order used by web_contents->ForEachFrame -
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).
> > 
> > I'd prefer to leave this as-is and address generation order in a subsequent
> > patch.  Are you okay with that?
> 
> I am fine with that (given that MHTML saved by end-users should not be
affected
> by the no-store changes and you've checked that Android is gracefully handling
> dangling CID URIs).
> 
> I assume that you are committing to working on this and landing a fix for the
> next milestone (M-53)? :-)
> 
> BTW: I am not particularly attached to bottom-up generation option of fixing
> this, but I do think that we need to do *something* about the CID behavior -
it 
> currently results in rather terrible experience for the desktop users who
happen
> to hit it.

I am going to work on this :)  Perhaps we can agree to work together!

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:394: if
(m_delegate.shouldSkipResource(resource))
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> On 2016/05/19 04:13:17, dewittj wrote:
> > On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > > I wonder if we can skip processing a resource before a data copy takes
place
> > > (i.e. by tweaking addFontToResources so that it calls a [new]
> > shouldAddResource
> > > instead of shouldAddURL - this would return before copying data from
> > > font->resourceBuffer into SharedBuffer).
> > 
> > I am not sure how we could get the URL (or indeed the Resource) without
> fetching
> > a remote font.
> 
> You're right - fetching is unavoidable, but we can avoid serialization work
> (which for images/fonts involves currently a memory allocation + copy, but in
> theory could become more expensive in the future).
> 
> > Can I look into this in a future patch?
> 
> Hmmm... how about a relatively small change to:
> 
> 1. Introduce a new method:
>    bool FrameSerializer::shouldProcessResource(const Resource& resource)
>    {
>        return shouldAddURL(resource.url()) &&
>               !m_delegate.shouldSkipResource(resource);
>    }
> 
> 2. Call this method instead of calling shouldAddURL inside:
>    - FrameSerializer::addImageToResources
>    - FrameSerializer::addFontToResources
>     
> 

Does this actually cause a copy?  It looks like it's a reference to an existing
SharedBuffer.

Also your solution doesn't simplify much since many calls to addImageToResources
specify a url from the src tag, which I am not sure is the same as
ImageResource.url().

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.h:96: virtual bool
shouldSkipResource(Resource*)
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> On 2016/05/19 04:13:17, dewittj wrote:
> > On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > > nit: I wonder we can avoid introducing a new method and instead change the
> > > signature of the old method (so that it takes Resource* instead of KURL&).

> > Less
> > > code = simpler and more readable code?
> > > 
> > > Hmmm... I guess what I am proposing requires some refactoring of
> > > FrameSerializer.cpp to change FrameSerializer::shouldAddURL to something
> > taking
> > > a Resource* instead... if this is too big of a change for the current CL,
> then
> > > maybe it can be done in a follow-up CL.
> > > 
> > > Also... I am not sure if what I am proposing would work for
> shouldAddURL(url)
> > > call in FrameSerializer::serializeCSSStyleSheet (i.e. there is no
Resource*
> > > anywhere nearby in this case...).
> > > 
> > > WDYT?  Just an idea :-)
> > 
> > Only other way that would work is if you had:
> > shouldSkipResourceWithURL(const KURL&, Resource*) where the Resource could
be
> > nullptr.  It didn't seem very clean, especially since in the implementation
> > there was never a need for /both/ the URL and the Resource.
> > 
> > So I'd like to leave this as-is, perhaps better names are in order?  I'm
open
> to
> > suggestions.
> 
> This is fine.  Thanks for looking into this.

Acknowledged.

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

https://codereview.chromium.org/1977303003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:37: int
global_request_id = 0;
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> Can this be made a static member of FindTrackingDelegate?

Done.

https://codereview.chromium.org/1977303003/diff/100001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:166:
&actual_number_of_frames));
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> This rather ugly code was written before web_contents()->GetAllFrames().size()
> was available.  Can you use this (instead of ForEachFrame) and get rid of
> increment method?

Done.

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

https://codereview.chromium.org/1977303003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4964: &delegate);
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> Do we still need this (this = |should_serialize_frame| variable + *public*
> WebFrameSerializer::frameShouldBeSerializedAsMHTML method)?  It seems that you
> are now handling no-store check within the serializer, in
> WebFrameSerializer::generateMHTMLHeader and
> WebFrameSerializer::generateMHTMLParts (which I think is great) so, checking
> return value of generateMHTMLHeader and generateMHTMLParts should be
sufficient?

Done.

https://codereview.chromium.org/1977303003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4972: if
(file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> Just double-checking: do we need to check here is generateMHTMLHeader returned
> an empty, default WebData() - it seems to be returned this when frame should
not
> be serialized.

Done.

https://codereview.chromium.org/1977303003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: &delegate);
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> Same question as above - do we need to check if generateMHTMLParts returned an
> empty WebData (and in this case we would need to set success=false)?

Done.

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:397: DLOG(ERROR) <<
"Adding " << url.getString() << " to resources!";
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> I assume this is temporary (i.e. for printf-style debugging) and can be
removed
> now?

Done.

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.h:90: virtual bool
shouldSkipResourceWithURL(const KURL&)
On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> Nice name change.  Long time ago this was also used to check if a subframe
> should be skipped, but right now (as the name change correctly highlights)
this
> is only used for subresources.

Acknowledged.

[dewittj, 2016-05-19 18:18:50]

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

[Łukasz Anforowicz, 2016-05-19 18:50:18]

Thanks - this is really close, but I think might need one more iteration (for
one more WebData emptiness check).

I've tried to review from Blink style perspective and nothing jumps out at me,
but probably Daniel would want to double-check if I haven't missed anything.

I've left a few nits about changing method parameter types to const-ref - I
think this might be worth doing, but I also see that the old methods take a
pointer to the frame (and don't remember why - maybe this is because I needed to
call non-const methods on the frame?  hopefully your situation is different and
const-ref would work for you).

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

https://codereview.chromium.org/1977303003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4986: &delegate);
On 2016/05/19 18:18:35, dewittj wrote:
> On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> > On 2016/05/19 04:13:17, dewittj wrote:
> > > On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > > > This skips generating the MHTML part for a subframe, but the parent
frame
> > will
> > > > continue emitting cid: URIs referring to the subframe (i.e. the subframe
> > will
> > > > still be covered by the map in
> > > > MHTMLGenerationManager::Job::frame_tree_node_to_content_id_ and
therefore
> > will
> > > > be used by the parent frame when generating the parent frame's MHTML).
> > > > 
> > > > As I wrote in https://crbug.com/570353#c7, I hope that bottom-up
> generation
> > in
> > > > MHTMLGenerationManager::Job can help here (right not MHTML is generated
in
> > the
> > > > order used by web_contents->ForEachFrame -
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).
> > > 
> > > I'd prefer to leave this as-is and address generation order in a
subsequent
> > > patch.  Are you okay with that?
> > 
> > I am fine with that (given that MHTML saved by end-users should not be
> affected
> > by the no-store changes and you've checked that Android is gracefully
handling
> > dangling CID URIs).
> > 
> > I assume that you are committing to working on this and landing a fix for
the
> > next milestone (M-53)? :-)
> > 
> > BTW: I am not particularly attached to bottom-up generation option of fixing
> > this, but I do think that we need to do *something* about the CID behavior -
> it 
> > currently results in rather terrible experience for the desktop users who
> happen
> > to hit it.
> 
> I am going to work on this :)  Perhaps we can agree to work together!

Sure - I'll be happy to help :-)

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/1977303003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:394: if
(m_delegate.shouldSkipResource(resource))
On 2016/05/19 18:18:35, dewittj wrote:
> On 2016/05/19 17:00:13, Łukasz Anforowicz wrote:
> > On 2016/05/19 04:13:17, dewittj wrote:
> > > On 2016/05/18 17:27:25, Łukasz Anforowicz wrote:
> > > > I wonder if we can skip processing a resource before a data copy takes
> place
> > > > (i.e. by tweaking addFontToResources so that it calls a [new]
> > > shouldAddResource
> > > > instead of shouldAddURL - this would return before copying data from
> > > > font->resourceBuffer into SharedBuffer).
> > > 
> > > I am not sure how we could get the URL (or indeed the Resource) without
> > fetching
> > > a remote font.
> > 
> > You're right - fetching is unavoidable, but we can avoid serialization work
> > (which for images/fonts involves currently a memory allocation + copy, but
in
> > theory could become more expensive in the future).
> > 
> > > Can I look into this in a future patch?
> > 
> > Hmmm... how about a relatively small change to:
> > 
> > 1. Introduce a new method:
> >    bool FrameSerializer::shouldProcessResource(const Resource& resource)
> >    {
> >        return shouldAddURL(resource.url()) &&
> >               !m_delegate.shouldSkipResource(resource);
> >    }
> > 
> > 2. Call this method instead of calling shouldAddURL inside:
> >    - FrameSerializer::addImageToResources
> >    - FrameSerializer::addFontToResources
> >     
> > 
> 
> Does this actually cause a copy?  It looks like it's a reference to an
existing
> SharedBuffer.

My bad - you're right, this is just a ref-count increase.
> 
> Also your solution doesn't simplify much since many calls to
addImageToResources
> specify a url from the src tag, which I am not sure is the same as
> ImageResource.url().

Ok - sounds like the current code is the correct one to use.  Thanks for looking
into this.

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

https://codereview.chromium.org/1977303003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: if
(file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
I think we also need the |data.size() == 0| check here?  (similarily to how
you've added it to the IsMainFrame case on line 4971).

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (left):

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:392: void
FrameSerializer::addToResources(Resource* resource, PassRefPtr<SharedBuffer>
data, const KURL& url)
nit: I think that callers always guarantee that |resource| is not null, so it
probably would be good to change this to const-ref.  This is old code, but this
change would make FrameSerializer::addToResources play nicely with the other
suggested const-ref change - in the new code / in
FrameSerializer::Delegate::shouldSkipResource).

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.h:96: virtual bool
shouldSkipResource(Resource*)
nit: Can this be ever null?  If not, then it probably makes sense to change this
to const-ref (especially given that the no-store checking accessors are now
"const" thanks to your other changes).

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

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:151: bool
cacheControlNoStoreHeaderPresent(WebLocalFrameImpl* webLocalFrameImpl)
nit: const-ref?

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:161: bool
frameShouldBeSerializedAsMHTML(WebLocalFrame* frame,
WebFrameSerializerCacheControlPolicy cacheControlPolicy)
nit: const-ref?

[dewittj, 2016-05-19 19:44:55]

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

https://codereview.chromium.org/1977303003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: if
(file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
On 2016/05/19 18:50:17, Łukasz Anforowicz wrote:
> I think we also need the |data.size() == 0| check here?  (similarily to how
> you've added it to the IsMainFrame case on line 4971).

No, we don't want to fail if any non-main-frame resource is skipped.  We just
want to silently skip it.

The implementation did write 0 bytes to the file, which is probably bad.  So I
updated it to include a size check.

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (left):

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:392: void
FrameSerializer::addToResources(Resource* resource, PassRefPtr<SharedBuffer>
data, const KURL& url)
On 2016/05/19 18:50:18, Łukasz Anforowicz wrote:
> nit: I think that callers always guarantee that |resource| is not null, so it
> probably would be good to change this to const-ref.  This is old code, but
this
> change would make FrameSerializer::addToResources play nicely with the other
> suggested const-ref change - in the new code / in
> FrameSerializer::Delegate::shouldSkipResource).

Done.

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.h:96: virtual bool
shouldSkipResource(Resource*)
On 2016/05/19 18:50:18, Łukasz Anforowicz wrote:
> nit: Can this be ever null?  If not, then it probably makes sense to change
this
> to const-ref (especially given that the no-store checking accessors are now
> "const" thanks to your other changes).

Done.

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

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:151: bool
cacheControlNoStoreHeaderPresent(WebLocalFrameImpl* webLocalFrameImpl)
On 2016/05/19 18:50:18, Łukasz Anforowicz wrote:
> nit: const-ref?

Done.

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:161: bool
frameShouldBeSerializedAsMHTML(WebLocalFrame* frame,
WebFrameSerializerCacheControlPolicy cacheControlPolicy)
On 2016/05/19 18:50:18, Łukasz Anforowicz wrote:
> nit: const-ref?

I don't think that's a great thing to do here, since toWebLocalFrameImpl takes a
pointer.  So I'd be dereferencing only to take the address later.

[Łukasz Anforowicz, 2016-05-19 19:51:25]

lgtm

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

https://codereview.chromium.org/1977303003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: if
(file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
On 2016/05/19 19:44:55, dewittj wrote:
> On 2016/05/19 18:50:17, Łukasz Anforowicz wrote:
> > I think we also need the |data.size() == 0| check here?  (similarily to how
> > you've added it to the IsMainFrame case on line 4971).
> 
> No, we don't want to fail if any non-main-frame resource is skipped.  We just
> want to silently skip it.

I see.  We might need to revisit if generating bottom-up and wanting to
distinguish between success/wrote-mhtml and success/skipped-frame.  Looks good
for now.

> 
> The implementation did write 0 bytes to the file, which is probably bad.  So I
> updated it to include a size check.

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

https://codereview.chromium.org/1977303003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:161: bool
frameShouldBeSerializedAsMHTML(WebLocalFrame* frame,
WebFrameSerializerCacheControlPolicy cacheControlPolicy)
On 2016/05/19 19:44:55, dewittj wrote:
> On 2016/05/19 18:50:18, Łukasz Anforowicz wrote:
> > nit: const-ref?
> 
> I don't think that's a great thing to do here, since toWebLocalFrameImpl takes
a
> pointer.  So I'd be dereferencing only to take the address later.

Blink's DEFINE_TYPE_CASTS macro makes sure that all toFoo methods have an
overload taking a reference as well (and I think this applies here as well).  At
the same time, having a pointer-vs-const-ref here doesn't seem very important.

[ncarter (slow), 2016-05-19 23:36:35]

content lgtm w/ some style nits

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

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:45: // Returns number
of result.
result -> results.

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:377:
std::vector<std::string> expectations{
std::vector<std::string> expectations = {

(per
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...)

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:399:
std::vector<std::string> expectations{
std::vector<std::string> expectations = {

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:402:
std::vector<std::string> forbidden{
 std::vector<std::string> forbidden = {

[dewittj, 2016-05-20 16:46:03]

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

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:45: // Returns number
of result.
On 2016/05/19 23:36:35, ncarter wrote:
> result -> results.

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:377:
std::vector<std::string> expectations{
On 2016/05/19 23:36:35, ncarter wrote:
> std::vector<std::string> expectations = {
> 
> (per
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...)

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:399:
std::vector<std::string> expectations{
On 2016/05/19 23:36:35, ncarter wrote:
> std::vector<std::string> expectations = {

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:402:
std::vector<std::string> forbidden{
On 2016/05/19 23:36:35, ncarter wrote:
>  std::vector<std::string> forbidden = {

Done.

[dewittj, 2016-05-20 18:38:34]

dcheng: ping

[dcheng, 2016-05-20 20:25:02]

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

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:39: // requested it.
I think this comment might be out of date?

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:84: }
Nit: newline before and // namespace after }

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:151:
NavigateToURL(shell(), GURL(net::FilePathToFileURL(params.file_path)));
This is already a GURL, no need to call GURL()'s conversion ctor.

https://codereview.chromium.org/1977303003/diff/140001/content/public/common/...
File content/public/common/mhtml_generation_params.h (right):

https://codereview.chromium.org/1977303003/diff/140001/content/public/common/...
content/public/common/mhtml_generation_params.h:22: LAST =
SKIP_ANY_FRAME_OR_RESOURCE_MARKED_NO_STORE
Seems like it'd be easier to just use the Web enum directly rather than keep
mirroring it. It gets rid of some static casts too.

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

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:715: bool useBinaryEncoding() override {
return params_.mhtml_binary_encoding; }
Is this just incidental cleanup to make the policy checks more consistent? Or
was it required for some other part of this patch?

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4971: if (data.size() == 0 ||
data.isEmpty()

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4971: if (data.size() == 0 ||
isEmpty()

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: if (data.size() > 0 &&
!data.isEmpty()

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: if (data.size() > 0 &&
!isEmpty()

[dewittj, 2016-05-20 21:22:22]

ncarter: FYI I changed content's DEPS to allow
WebFrameSerializerCacheControlPolicy, just an enum.

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

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:39: // requested it.
On 2016/05/20 20:25:02, dcheng wrote:
> I think this comment might be out of date?

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:84: }
On 2016/05/20 20:25:01, dcheng wrote:
> Nit: newline before and // namespace after }

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:151:
NavigateToURL(shell(), GURL(net::FilePathToFileURL(params.file_path)));
On 2016/05/20 20:25:02, dcheng wrote:
> This is already a GURL, no need to call GURL()'s conversion ctor.

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/public/common/...
File content/public/common/mhtml_generation_params.h (right):

https://codereview.chromium.org/1977303003/diff/140001/content/public/common/...
content/public/common/mhtml_generation_params.h:22: LAST =
SKIP_ANY_FRAME_OR_RESOURCE_MARKED_NO_STORE
On 2016/05/20 20:25:02, dcheng wrote:
> Seems like it'd be easier to just use the Web enum directly rather than keep
> mirroring it. It gets rid of some static casts too.

Done.

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

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:715: bool useBinaryEncoding() override {
return params_.mhtml_binary_encoding; }
On 2016/05/20 20:25:02, dcheng wrote:
> Is this just incidental cleanup to make the policy checks more consistent? Or
> was it required for some other part of this patch?

Incidental.  Do you prefer I revert it?  It cleaned up some of the API
signatures of WebFrameSerializer.

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4971: if (data.size() == 0 ||
On 2016/05/20 20:25:02, dcheng wrote:
> isEmpty()

Done.

https://codereview.chromium.org/1977303003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4986: if (data.size() > 0 &&
On 2016/05/20 20:25:02, dcheng wrote:
> !data.isEmpty()

Done.

[dcheng, 2016-05-20 21:28:00]

lgtm

[dewittj, 2016-05-23 16:14:49]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-05-23 16:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1977303003/#ps180001
(title: "Address dcheng's comments.")

[commit-bot: I haz the power, 2016-05-23 16:14:58]

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

[commit-bot: I haz the power, 2016-05-23 19:04:56]

Description was changed from

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.  Note that
because CSS is not considered a resource
when serializing, CSS will be covered separately.

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

to

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.  Note that
because CSS is not considered a resource
when serializing, CSS will be covered separately.

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

[commit-bot: I haz the power, 2016-05-23 19:04:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-23 19:06:25]

Description was changed from

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.  Note that
because CSS is not considered a resource
when serializing, CSS will be covered separately.

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

to

==========
Adds a feature to MHTML serialization that omits subframes and subresources
marked no-store.

Also integrates this new feature with the offline page archiver.  Note that
because CSS is not considered a resource
when serializing, CSS will be covered separately.

BUG=609566

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

[commit-bot: I haz the power, 2016-05-23 19:06:27]

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

[ncarter (slow), 2016-05-23 23:00:30]

https://codereview.chromium.org/1977303003/diff/180001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1977303003/diff/180001/content/browser/DEPS#n...
content/browser/DEPS:89:
"+third_party/WebKit/public/web/WebFrameSerializerCacheControlPolicy.h",
This is fine.