New issue
Advanced search Search tips

Issue 672292 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Move MHTML footer and header generation to the browser process

Project Member Reported by carlosk@chromium.org, Dec 8 2016

Issue description

MHTML generation is a multi-process operation, shared between the browser process and all render processes holding frames of the page being saved. Currently the header and footer of the generated MHTML are written by the render processes, by the first frame to be serialized (always the main) and the last one, respectively.

Moving these operations to the browser has the main objective of allowing the saving of MHTML pages even if the last frame goes missing from the page (see issue 655708).

Another advantage is to lessen the work done by the render process, which is the more important one in terms of user perceived jank and input responsiveness.
 

Comment 1 by dim...@chromium.org, Dec 14 2016

Owner: carlosk@chromium.org
Status: Assigned (was: Available)

Comment 2 by chili@chromium.org, Jan 25 2017

Labels: Hotlist-Fixit
Cc: petewil@chromium.org
Status: Started (was: Assigned)
Fixit update: good one for fixit but I am currently working on it.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f

commit 7ae9f6fc18857892d7ffc3d5d3203720dd198c9f
Author: carlosk <carlosk@chromium.org>
Date: Fri Mar 10 00:59:00 2017

Move the writing of the MHTML footer to the browser process.

This change moves the MHTML generation step of writing the footer from
the renderer process to the browser process. Even though this spreads
MHTML code knowledge between Blink and content (a drawback) it does make
the generation process more robust and allows MHTML save operations to
become more robust to DOM changes.

The main issue we are facing with the Offline efforts is that frames can
be deleted after a MHTML save operation started and that will likely
cause it to fail completely. With this modification we can allow these
operations to optionally ignore missing frames.

I chose to still keep the footer code generation in MHTMLArchive but
make it explicit it is only used for testing purposes now. It seemed
more consistent and understandable for those tests to keep on generating
and reading fully valid MHTML files.

BUG= 672292 
TBR=jam@chromium.org

Review-Url: https://codereview.chromium.org/2731293004
Cr-Commit-Position: refs/heads/master@{#455935}

[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/chrome/browser/content_settings/content_settings_browsertest.cc
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/content/browser/download/mhtml_generation_manager.cc
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/content/browser/download/mhtml_generation_manager.h
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/content/common/frame_messages.h
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/third_party/WebKit/Source/platform/mhtml/MHTMLArchive.h
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/third_party/WebKit/Source/web/WebFrameSerializer.cpp
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/third_party/WebKit/Source/web/tests/MHTMLTest.cpp
[modify] https://crrev.com/7ae9f6fc18857892d7ffc3d5d3203720dd198c9f/third_party/WebKit/public/web/WebFrameSerializer.h

Blocking: -655708
Owner: dewittj@chromium.org
Taking over for fixit
Status: Fixed (was: Started)
Initial change at https://codereview.chromium.org/2842653002/ was rejected due to code copying and minimal benefits (other than parallelism with footer generation). This bug can be closed now.

Sign in to add a comment