Save-Page-As-Complete-HTML: Even better handling of <object> elements.

Save-Page-As-Complete-HTML: Even better handling of <object> elements.

crrev.com/1416113012 improved handling of <object> elements by
save-page-as-complete-html, but always treated them as resources.
The current CL tries to report them as subframes if they contain
a html document.

This CL also adjusts WebFrame::fromFrameOwnerElement and
WebLocalFrameImpl::fromFrameOwnerElement so that they truly work with
instances of HTMLFrameOwnerElement (as the name of these methods
suggests), rather than only with instances of HTMLFrameElementBase (a
subclass of HTMLFrameOwnerElement that only covers <frame> and <iframe>
elements and not <object> element or other frame-owning elements).

FWIW, I tracked down the comment in Web(Local)FrameImpl.cpp that questions
why we only check for <iframe> and <frame> to the following commit:
abarth@webkit.org fd5b347 2012-09-28 18:31:12 :
    // FIXME: Why do we check specifically for <iframe> and <frame> here?
    // Why can't we get the WebFrameImpl from an <object> element,
    // for example.

BUG=553478
Committed: https://crrev.com/c5aebc965aa088c133f20b51f62759c723e5797b
Cr-Commit-Position: refs/heads/master@{#362165}

[Łukasz Anforowicz, 2015-11-23 23:12:30]

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

[Łukasz Anforowicz, 2015-11-23 23:12:30]

Nasko, can you please take a look?

[Łukasz Anforowicz, 2015-11-23 23:13:25]

Adding yosin@ and rdsmith@ to CC.

[nasko, 2015-11-23 23:38:19]

content/ LGTM with a nit.

https://codereview.chromium.org/1442463002/diff/20001/content/renderer/savabl...
File content/renderer/savable_resources.cc (right):

https://codereview.chromium.org/1442463002/diff/20001/content/renderer/savabl...
content/renderer/savable_resources.cc:50: GURL u =
current_doc.completeURL(value);
nit: I find single letter variables to be hard to follow. Why not "url" or
"doc_url"?

[Łukasz Anforowicz, 2015-11-24 00:02:06]

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

[Łukasz Anforowicz, 2015-11-24 00:02:06]

Randy, could you please take a look?

https://codereview.chromium.org/1442463002/diff/20001/content/renderer/savabl...
File content/renderer/savable_resources.cc (right):

https://codereview.chromium.org/1442463002/diff/20001/content/renderer/savabl...
content/renderer/savable_resources.cc:50: GURL u =
current_doc.completeURL(value);
On 2015/11/23 23:38:19, nasko wrote:
> nit: I find single letter variables to be hard to follow. Why not "url" or
> "doc_url"?

Done.  (using "element_url" as the variable name - it is element's url
"completed" into an absolute url on top of doc's base.  hmmm... I wonder why
blink used "complete" rather than "get-absolute" in the name of the method...)

[yosin_UTC9, 2015-11-24 01:32:41]

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

[yosin_UTC9, 2015-11-24 01:32:42]

non-owner lgtm

https://codereview.chromium.org/1442463002/diff/40001/content/renderer/savabl...
File content/renderer/savable_resources.cc (right):

https://codereview.chromium.org/1442463002/diff/40001/content/renderer/savabl...
content/renderer/savable_resources.cc:55: bool frameContainsHtmlDoc = false;
nit: Can we move out this if-statement, L55-L70 as static function taking
|web_frame|
to make |GetSavableResourceLinkForElement()| smaller and encapsulating logic of
|frameContainsHtmlDoc|?

https://codereview.chromium.org/1442463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1442463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1827: return 0;
I think it is OK to replace |0| to |nullptr| in this CL.

[Łukasz Anforowicz, 2015-11-24 17:57:11]

Thank you for reviewing yosin@.  Randy, can you please take a look?

https://codereview.chromium.org/1442463002/diff/40001/content/renderer/savabl...
File content/renderer/savable_resources.cc (right):

https://codereview.chromium.org/1442463002/diff/40001/content/renderer/savabl...
content/renderer/savable_resources.cc:55: bool frameContainsHtmlDoc = false;
On 2015/11/24 01:32:41, Yosi_UTC9 wrote:
> nit: Can we move out this if-statement, L55-L70 as static function taking
> |web_frame|
> to make |GetSavableResourceLinkForElement()| smaller and encapsulating logic
of
> |frameContainsHtmlDoc|?

Done.

For a moment I wondered if we should have
GetWebFrameIfElementContainsHtmlDocument(const WebElement&) rather than
DoesFrameContainHtmlDocument, but I think the latter is more straightforward and
easier to understand (even though having to pass both WebFrame& and WebElement&
feels a bit weird).

https://codereview.chromium.org/1442463002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1442463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1827: return 0;
On 2015/11/24 01:32:41, Yosi_UTC9 wrote:
> I think it is OK to replace |0| to |nullptr| in this CL.

Done.  (here and in a few surrounding / related functions for consistency)

[Randy Smith (Not in Mondays), 2015-11-28 00:07:18]

chrome/* LGTM.

[Łukasz Anforowicz, 2015-11-30 14:55:08]

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

[commit-bot: I haz the power, 2015-11-30 14:55:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442463002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442463002/80001

[commit-bot: I haz the power, 2015-11-30 16:11:23]

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

[commit-bot: I haz the power, 2015-11-30 16:11:25]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2015-11-30 17:01:42]

Daniel, can you rubberstamp this CL for third_party/WebKit?

[Łukasz Anforowicz, 2015-11-30 17:04:54]

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

[Łukasz Anforowicz, 2015-11-30 17:04:55]

Actually adding Daniel this time...

[dcheng, 2015-11-30 17:59:02]

lgtm

[Łukasz Anforowicz, 2015-11-30 18:11:14]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-11-30 18:11:15]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1442463002/#ps80001
(title: "Fixing a silly build issue... :-/")

[commit-bot: I haz the power, 2015-11-30 18:13:48]

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

[commit-bot: I haz the power, 2015-11-30 18:23:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-30 18:24:28]

Description was changed from

==========
Save-Page-As-Complete-HTML: Even better handling of <object> elements.

crrev.com/1416113012 improved handling of <object> elements by
save-page-as-complete-html, but always treated them as resources.
The current CL tries to report them as subframes if they contain
a html document.

This CL also adjusts WebFrame::fromFrameOwnerElement and
WebLocalFrameImpl::fromFrameOwnerElement so that they truly work with
instances of HTMLFrameOwnerElement (as the name of these methods
suggests), rather than only with instances of HTMLFrameElementBase (a
subclass of HTMLFrameOwnerElement that only covers <frame> and <iframe>
elements and not <object> element or other frame-owning elements).

FWIW, I tracked down the comment in Web(Local)FrameImpl.cpp that questions
why we only check for <iframe> and <frame> to the following commit:
abarth@webkit.org fd5b347 2012-09-28 18:31:12 :
    // FIXME: Why do we check specifically for <iframe> and <frame> here?
    // Why can't we get the WebFrameImpl from an <object> element,
    // for example.

BUG=553478
==========

to

==========
Save-Page-As-Complete-HTML: Even better handling of <object> elements.

crrev.com/1416113012 improved handling of <object> elements by
save-page-as-complete-html, but always treated them as resources.
The current CL tries to report them as subframes if they contain
a html document.

This CL also adjusts WebFrame::fromFrameOwnerElement and
WebLocalFrameImpl::fromFrameOwnerElement so that they truly work with
instances of HTMLFrameOwnerElement (as the name of these methods
suggests), rather than only with instances of HTMLFrameElementBase (a
subclass of HTMLFrameOwnerElement that only covers <frame> and <iframe>
elements and not <object> element or other frame-owning elements).

FWIW, I tracked down the comment in Web(Local)FrameImpl.cpp that questions
why we only check for <iframe> and <frame> to the following commit:
abarth@webkit.org fd5b347 2012-09-28 18:31:12 :
    // FIXME: Why do we check specifically for <iframe> and <frame> here?
    // Why can't we get the WebFrameImpl from an <object> element,
    // for example.

BUG=553478

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

[commit-bot: I haz the power, 2015-11-30 18:24:28]

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