Remove popup overlay from MHTML when requested

Remove popup overlay from MHTML when requested

If the page is being loaded in background mode and saved to MHTML,
we should try to remove popup overlay that obstructs viewing the
normal content. This is controlled by MHTMLGenerationParams::
remove_popup_overlay.

BUG=684091
TEST=new tests added

Review-Url: https://codereview.chromium.org/2640923003
Cr-Commit-Position: refs/heads/master@{#445612}
Committed: https://chromium.googlesource.com/chromium/src/+/ec72fc4b04c800b1d54063d09b79350fdcd0cc15

[jianli, 2017-01-19 00:24:55]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 00:25:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2017-01-19 01:35:21]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, nasko@chromium.org, tkent@chromium.org

[jianli, 2017-01-19 01:35:21]

tkent for Blink
fgorski for offline_pages
nasko for content

[tkent, 2017-01-19 03:38:48]

third_party/WebKit lgtm

[commit-bot: I haz the power, 2017-01-19 04:02:23]

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

[commit-bot: I haz the power, 2017-01-19 04:02:24]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-01-19 17:59:08]

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
File components/offline_pages/core/client_policy_controller.h (right):

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
components/offline_pages/core/client_policy_controller.h:61: bool
ShouldRemovePopupOverlay(const std::string& name_space) const;
I've just discussed this with Dmitry. 2 comments:
1. It should only be affecting background work, not snapshotting the live page
(which should be reflected by name of the method and documentation)
2. You have more than just async loading to worry about:
* "download" can be handled in the background
* "async_loading"
* "ntp_suggestions"
(This second part applies to initializing policies in cc file)

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
File components/offline_pages/core/offline_page_archiver.h (right):

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
components/offline_pages/core/offline_page_archiver.h:62: struct
CreateArchiveParams {
you could add parameter to indicate whether the work happens in the background
or on live page addressing the earlier comment.

[Pete Williamson, 2017-01-19 18:08:27]

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

[Pete Williamson, 2017-01-19 18:08:28]

Drive by comment:

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:150: // The bounding box of
the element should contain center point of the
FYI - I have seen some popups in EM pages that do not cover the center of the
window. Would we be better without this check?

Example URLs with this behavior: 
http://www.ndtv.com/tv-schedule?pfrom=home-topnavigation2016
https://www.icicibank.com/

Although the dialog box does not cover the center, they grey out the rest of the
window, so if they do that with a transparent window, it might cover the center,
this might be a good test for your code.

[dcheng, 2017-01-19 18:24:54]

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

[dcheng, 2017-01-19 18:24:55]

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:147: if (!box ||
box->style()->display() == EDisplay::None)
Note that an element styled with display:none won't have a LayoutObject (and
thus no LayoutBox), so the second part of the || is unnecessary.

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:152: LocalDOMWindow* window
= element.document().domWindow();
Nit: DCHECK instead of guarding with if, since we shouldn't be serializing
detached frames.

[jianli, 2017-01-19 23:55:04]

Patchset #3 (id:40001) has been deleted

[jianli, 2017-01-19 23:55:14]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[jianli, 2017-01-19 23:55:22]

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
File components/offline_pages/core/offline_page_archiver.h (right):

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
components/offline_pages/core/offline_page_archiver.h:62: struct
CreateArchiveParams {
On 2017/01/19 17:59:08, fgorski wrote:
> you could add parameter to indicate whether the work happens in the background
> or on live page addressing the earlier comment.

I will add this parameter to SavePageParams. Here we want to explicitly say to
remove_popup_overlay.

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:147: if (!box ||
box->style()->display() == EDisplay::None)
On 2017/01/19 18:24:55, dcheng wrote:
> Note that an element styled with display:none won't have a LayoutObject (and
> thus no LayoutBox), so the second part of the || is unnecessary.

Done.

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:150: // The bounding box of
the element should contain center point of the
On 2017/01/19 18:08:28, Pete Williamson wrote:
> FYI - I have seen some popups in EM pages that do not cover the center of the
> window. Would we be better without this check?
> 
> Example URLs with this behavior: 
> http://www.ndtv.com/tv-schedule?pfrom=home-topnavigation2016
> https://www.icicibank.com/
> 
> Although the dialog box does not cover the center, they grey out the rest of
the
> window, so if they do that with a transparent window, it might cover the
center,
> this might be a good test for your code.

The 1st one is covered while the 2nd one is not handled. Will investigate and
enhance the popup removal in future patch. For now, we're covering most common
and safe scenario: popup appearing at the center

https://codereview.chromium.org/2640923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:152: LocalDOMWindow* window
= element.document().domWindow();
On 2017/01/19 18:24:55, dcheng wrote:
> Nit: DCHECK instead of guarding with if, since we shouldn't be serializing
> detached frames.

Done.

[jianli, 2017-01-19 23:56:10]

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
File components/offline_pages/core/client_policy_controller.h (right):

https://codereview.chromium.org/2640923003/diff/1/components/offline_pages/co...
components/offline_pages/core/client_policy_controller.h:61: bool
ShouldRemovePopupOverlay(const std::string& name_space) const;
On 2017/01/19 17:59:08, fgorski wrote:
> I've just discussed this with Dmitry. 2 comments:
> 1. It should only be affecting background work, not snapshotting the live page
> (which should be reflected by name of the method and documentation)
> 2. You have more than just async loading to worry about:
> * "download" can be handled in the background
> * "async_loading"
> * "ntp_suggestions"
> (This second part applies to initializing policies in cc file)

Changed to pass the background save info in SavePageParams.

[commit-bot: I haz the power, 2017-01-19 23:56:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 02:10:43]

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

[commit-bot: I haz the power, 2017-01-20 02:10:44]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-01-20 17:16:53]

lgtm for offline_pages code

https://codereview.chromium.org/2640923003/diff/60001/components/offline_page...
File components/offline_pages/core/offline_page_model.h (right):

https://codereview.chromium.org/2640923003/diff/60001/components/offline_page...
components/offline_pages/core/offline_page_model.h:74: // Whether the page is
being saved on the background.
nit: s/on/in/

[dcheng, 2017-01-20 18:33:10]

Sorry that lifetime of Blink objects is such a mess, and thanks for adding some
assertions for this. lgtm

[nasko, 2017-01-20 23:03:23]

content/ LGTM

[jianli, 2017-01-23 22:01:44]

Description was changed from

==========
Remove popup overlay from MHTML when requested

If the page is being loaded in background mode and saved to MHTML,
we should try to remove popup overlay that obstructs viewing the
normal content. This is controlled by MHTMLGenerationParams::
remove_popup_overlay.

BUG=669325
TEST=new tests added
==========

to

==========
Remove popup overlay from MHTML when requested

If the page is being loaded in background mode and saved to MHTML,
we should try to remove popup overlay that obstructs viewing the
normal content. This is controlled by MHTMLGenerationParams::
remove_popup_overlay.

BUG=684091
TEST=new tests added
==========

[jianli, 2017-01-23 22:01:53]

The CQ bit was checked by jianli@chromium.org

[jianli, 2017-01-23 22:01:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
nasko@chromium.org, dcheng@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2640923003/#ps80001
(title: "Fix comments")

[commit-bot: I haz the power, 2017-01-23 22:02:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:07:39]

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

[commit-bot: I haz the power, 2017-01-23 22:07:41]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jianli, 2017-01-23 22:25:32]

The CQ bit was checked by jianli@chromium.org

[jianli, 2017-01-23 22:25:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
nasko@chromium.org, dcheng@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2640923003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-23 22:26:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 02:36:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485210332949690,
"parent_rev": "ad092abe5691a30391dbc0fd846af75e9917d37b", "commit_rev":
"ec72fc4b04c800b1d54063d09b79350fdcd0cc15"}

[commit-bot: I haz the power, 2017-01-24 02:36:41]

Description was changed from

==========
Remove popup overlay from MHTML when requested

If the page is being loaded in background mode and saved to MHTML,
we should try to remove popup overlay that obstructs viewing the
normal content. This is controlled by MHTMLGenerationParams::
remove_popup_overlay.

BUG=684091
TEST=new tests added
==========

to

==========
Remove popup overlay from MHTML when requested

If the page is being loaded in background mode and saved to MHTML,
we should try to remove popup overlay that obstructs viewing the
normal content. This is controlled by MHTMLGenerationParams::
remove_popup_overlay.

BUG=684091
TEST=new tests added

Review-Url: https://codereview.chromium.org/2640923003
Cr-Commit-Position: refs/heads/master@{#445612}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec72fc4b04c800b1d54063d09b79...
==========

[commit-bot: I haz the power, 2017-01-24 02:36:42]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ec72fc4b04c800b1d54063d09b79...