Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

This CL moves the logic to retrieve the WebPluginContainer from static methods
in WebLocalFrameImpl & WebNode to member method on LocalFrame and Node.

It introduces a virtual method on PluginView to return the PluginContainer, that
is overridden in WebPluginContainerBase to remove the need for down casting.

Logically this CL should introduce no changes.

BUG=708879
Review-Url: https://codereview.chromium.org/2888203006
Cr-Commit-Position: refs/heads/master@{#474176}
Committed: https://chromium.googlesource.com/chromium/src/+/171284077b44d0102ffc9065331fb3b1efe3fb67

[slangley, 2017-05-19 02:03:29]

Description was changed from

==========
Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

git cl format


shuffle the logic around.


Seems to build at least.


git cl format


Fix logic to match previous.


Remove methods no longer required.


Seems working still.


First part done.

BUG=
==========

to

==========
Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

This CL moves the logic to retrieve the WebPluginContainer from static methods
in WebLocalFrameImpl & WebNode to member method on LocalFrame and Node.

It introduces a virtual method on PluginView to return the PluginContainer, that
is overridden in WebPluginContainerBase to remove the need for down casting.

Logically this CL should introduce no changes.

BUG=
==========

[slangley, 2017-05-19 02:09:52]

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

[slangley, 2017-05-19 02:17:28]

Description was changed from

==========
Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

This CL moves the logic to retrieve the WebPluginContainer from static methods
in WebLocalFrameImpl & WebNode to member method on LocalFrame and Node.

It introduces a virtual method on PluginView to return the PluginContainer, that
is overridden in WebPluginContainerBase to remove the need for down casting.

Logically this CL should introduce no changes.

BUG=
==========

to

==========
Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

This CL moves the logic to retrieve the WebPluginContainer from static methods
in WebLocalFrameImpl & WebNode to member method on LocalFrame and Node.

It introduces a virtual method on PluginView to return the PluginContainer, that
is overridden in WebPluginContainerBase to remove the need for down casting.

Logically this CL should introduce no changes.

BUG=708879
==========

[dcheng, 2017-05-19 09:07:02]

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:250: // plugin, then it runs
its WebPluginContainerBase. Otherwise, uses the
runs -> returns

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1202: if (frame) {
Do you know if this check is actually needed? I would be surprised to be calling
with a null |frame|.

(Actually maybe we can delete this? I don't see any references to this in
codesearch)

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp:161: if
(GetFrame()) {
This GetFrame() check shouldn't be required.

We probably need to clean up some of the ownership here: it's confusing because
this object is owned by a unique_ptr on WebLocalFrameImpl, and this object has a
weak member back to the WebLocalFrame. So despite the WeakMember<> here, they
have the same lifetime.

The only time this will return null is if we are calling public API methods on
|this| after the frame is already detached--but by definition, the embedder
already called Close(), so it would violate the API contract to do so.

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2586: Node* node) const {
From what I can tell, there are no callers that pass non-null node. Maybe we
should just remove that argument? Then we can simply this function a bit.
Currently, LocalFrame::GetWebPluginContainerBase has a pretty complex API
surface:
- If LocalFrame holds a PluginDocument, return the associated plugin base.
- Otherwise, if node is null, set the node argument to the current focused
element.
- If the node is associated with a plugin, returned the associated plugin base.
- Otherwise return null.

If we remove it, I think it will just be:
- If LocalFrame holds a PluginDocument, return the associated plugin base.
- Otherwise, return the focused element's associated plugin base, if any.

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2422: if (frame) {
Ditto: I think this is actually unused now

[slangley, 2017-05-21 23:53:33]

PTAL

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:250: // plugin, then it runs
its WebPluginContainerBase. Otherwise, uses the
On 2017/05/19 at 09:07:02, dcheng wrote:
> runs -> returns

Done

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1202: if (frame) {
On 2017/05/19 at 09:07:02, dcheng wrote:
> Do you know if this check is actually needed? I would be surprised to be
calling with a null |frame|.
> 
> (Actually maybe we can delete this? I don't see any references to this in
codesearch)

Deleted.

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp:161: if
(GetFrame()) {
On 2017/05/19 at 09:07:02, dcheng wrote:
> This GetFrame() check shouldn't be required.
> 
> We probably need to clean up some of the ownership here: it's confusing
because this object is owned by a unique_ptr on WebLocalFrameImpl, and this
object has a weak member back to the WebLocalFrame. So despite the WeakMember<>
here, they have the same lifetime.
> 
> The only time this will return null is if we are calling public API methods on
|this| after the frame is already detached--but by definition, the embedder
already called Close(), so it would violate the API contract to do so.

Removed the check - no other methods have it.

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2586: Node* node) const {
On 2017/05/19 at 09:07:02, dcheng wrote:
> From what I can tell, there are no callers that pass non-null node. Maybe we
should just remove that argument? Then we can simply this function a bit.
Currently, LocalFrame::GetWebPluginContainerBase has a pretty complex API
surface:
> - If LocalFrame holds a PluginDocument, return the associated plugin base.
> - Otherwise, if node is null, set the node argument to the current focused
element.
> - If the node is associated with a plugin, returned the associated plugin
base.
> - Otherwise return null.
> 
> If we remove it, I think it will just be:
> - If LocalFrame holds a PluginDocument, return the associated plugin base.
> - Otherwise, return the focused element's associated plugin base, if any.

There is one caller that passes a Node - "WebLocalFrameImpl::ExecuteCommand".
Based on the fact that GetFrame() is never null I removed this and just call
LocalFrame() directly.

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

https://codereview.chromium.org/2888203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2422: if (frame) {
On 2017/05/19 at 09:07:02, dcheng wrote:
> Ditto: I think this is actually unused now

Deleted.

[haraken, 2017-05-22 04:51:43]

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

[haraken, 2017-05-22 04:51:44]

LGTM

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2585: if (!isHTMLObjectElement(this)
&& !isHTMLEmbedElement(this)) {

Can we probably use isHTMLPluginElement()?

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:251: // currently focused
element (if any).

This looks like a weird semantics. Can we drop the part after "If the provided
node..." in a follow-up CL?

Alternately we can just make the caller site to query the focused element and
call Node::GetWebPluginContainerBase when LocalFrame::GetWebPluginContainerBase
returns nullptr. It looks weird to hide the logic in
LocalFrame::GetWebPluginContainerBase.

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/plugins/PluginView.h (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/plugins/PluginView.h:81: virtual
WebPluginContainerBase* GetWebPluginContainerBase() const {

Can we probably make this a pure virtual method?

[dcheng, 2017-05-22 09:48:29]

LGTM

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:251: // currently focused
element (if any).
On 2017/05/22 04:51:44, haraken wrote:
> 
> This looks like a weird semantics. Can we drop the part after "If the provided
> node..." in a follow-up CL?
> 
> Alternately we can just make the caller site to query the focused element and
> call Node::GetWebPluginContainerBase when
LocalFrame::GetWebPluginContainerBase
> returns nullptr. It looks weird to hide the logic in
> LocalFrame::GetWebPluginContainerBase.

I agree it's a bit confusing (which is why I was hoping we didn't have to plumb
a Node in at all). But I'm not sure if making current callsites explicitly grab
the focused node is better as it's called in a fair number of different places.

[haraken, 2017-05-22 13:32:56]

On 2017/05/22 09:48:29, dcheng wrote:
> LGTM
> 
>
https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/LocalFrame.h (right):
> 
>
https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/LocalFrame.h:251: // currently focused
> element (if any).
> On 2017/05/22 04:51:44, haraken wrote:
> > 
> > This looks like a weird semantics. Can we drop the part after "If the
provided
> > node..." in a follow-up CL?
> > 
> > Alternately we can just make the caller site to query the focused element
and
> > call Node::GetWebPluginContainerBase when
> LocalFrame::GetWebPluginContainerBase
> > returns nullptr. It looks weird to hide the logic in
> > LocalFrame::GetWebPluginContainerBase.
> 
> I agree it's a bit confusing (which is why I was hoping we didn't have to
plumb
> a Node in at all). But I'm not sure if making current callsites explicitly
grab
> the focused node is better as it's called in a fair number of different
places.

The weird semantics is already in the current
WebLocalFrameImpl::CurrentPluginContainer though. (So fixing the semantics would
be an orthogonal thing to this CL anyway.)

[slangley, 2017-05-22 23:53:25]

I've added TODO to followup fixing the strange semantics of how these methods
are working. I'd like to get this mechanical CL in and will send the followup CL
today.

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2585: if (!isHTMLObjectElement(this)
&& !isHTMLEmbedElement(this)) {
On 2017/05/22 at 04:51:44, haraken wrote:
> Can we probably use isHTMLPluginElement()?

If possible, i'd rather this change be purely mechanical. I can followup with
this change.

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:251: // currently focused
element (if any).
On 2017/05/22 at 04:51:44, haraken wrote:
> This looks like a weird semantics. Can we drop the part after "If the provided
node..." in a follow-up CL?
> 
> Alternately we can just make the caller site to query the focused element and
call Node::GetWebPluginContainerBase when LocalFrame::GetWebPluginContainerBase
returns nullptr. It looks weird to hide the logic in
LocalFrame::GetWebPluginContainerBase.

I added a TODO and will followup with a CL to address this.

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/plugins/PluginView.h (right):

https://codereview.chromium.org/2888203006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/plugins/PluginView.h:81: virtual
WebPluginContainerBase* GetWebPluginContainerBase() const {
On 2017/05/22 at 04:51:44, haraken wrote:
> Can we probably make this a pure virtual method?

This class has a mix of pure virtual and virtual - my preference is for this to
not be pure virtual and override only where appropriate.

[slangley, 2017-05-22 23:53:41]

The CQ bit was checked by slangley@chromium.org

[slangley, 2017-05-22 23:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2888203006/#ps80001
(title: "Address code review comments.")

[commit-bot: I haz the power, 2017-05-22 23:54:06]

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-05-23 01:15:42]

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

[commit-bot: I haz the power, 2017-05-23 01:15:43]

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

[slangley, 2017-05-24 03:14:39]

The CQ bit was checked by slangley@chromium.org

[slangley, 2017-05-24 03:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2888203006/#ps100001
(title: "Ensure we have a valid PluginView before using it.")

[commit-bot: I haz the power, 2017-05-24 03:15:51]

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-05-24 05:45:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495595679272400,
"parent_rev": "9dd6cc8ed582ed45422be5c4a0dad00464d2639a", "commit_rev":
"171284077b44d0102ffc9065331fb3b1efe3fb67"}

[commit-bot: I haz the power, 2017-05-24 05:45:33]

Description was changed from

==========
Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

This CL moves the logic to retrieve the WebPluginContainer from static methods
in WebLocalFrameImpl & WebNode to member method on LocalFrame and Node.

It introduces a virtual method on PluginView to return the PluginContainer, that
is overridden in WebPluginContainerBase to remove the need for down casting.

Logically this CL should introduce no changes.

BUG=708879
==========

to

==========
Move the logic to retrieve the WebPluginContainer to LocalFrame and Node.

This CL moves the logic to retrieve the WebPluginContainer from static methods
in WebLocalFrameImpl & WebNode to member method on LocalFrame and Node.

It introduces a virtual method on PluginView to return the PluginContainer, that
is overridden in WebPluginContainerBase to remove the need for down casting.

Logically this CL should introduce no changes.

BUG=708879

Review-Url: https://codereview.chromium.org/2888203006
Cr-Commit-Position: refs/heads/master@{#474176}
Committed:
https://chromium.googlesource.com/chromium/src/+/171284077b44d0102ffc9065331f...
==========

[commit-bot: I haz the power, 2017-05-24 05:45:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/171284077b44d0102ffc9065331f...

[bjohnson, 2017-08-01 05:37:53]

On 2017/05/24 05:45:34, commit-bot: I haz the power wrote:
> Committed patchset #6 (id:100001) as
>
https://chromium.googlesource.com/chromium/src/+/171284077b44d0102ffc9065331f...

This does change the logic. Before the patch the methods that called
`PluginContainerFromFrame ` would return 0 if `if (!frame->GetDocument() ||
!frame->GetDocument()->IsPluginDocument())`, but now they continue and return
`node->GetWebPluginContainerBase()` instead which breaks calls to
`SetPageZoomFactor` for instance.