Bug 168417 – Remove Chromium-specific code to call FrameLoaderClient::redirectDataToPlugin(nullptr)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

In r125500 (bug #93283) we added code to PluginDocument::detachFromPluginElement() to call FrameLoaderClient::redirectDataToPlugin(nullptr). Calling redirectDataToPlugin() with nullptr was used by the Chromium port to signify that the plugin document was being destroyed so that they could tear down their plugin widget. And PluginDocument::detachFromPluginElement() is the only place that calls redirectDataToPlugin() passing nullptr. No other port made use of this machinery and the Chromium port has long since been removed from the Open Source WebKit Project. We should remove this code.

[Daniel Bates]

Created attachment 301723 [details]
Patch

[Daniel Bates]

Are the files Source/WebKit/mac/WebKit.order and Source/WebKit2/mac/WebKit2.order still relevant? Xcode still references these order files though they were last modified in r180570 (2 years ago).

[Daniel Bates]

<rdar://problem/30541748>

[Chris Dumez]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:1178
> +    if (pluginWidget.isPluginView())

Looks like a possible behavior change here? We used to reset m_manualLoader to nullptr when pluginWidget was null.

> Source/WebKit2/mac/WebKit2.order:-6993
> -__ZN6WebKit20WebFrameLoaderClient20redirectDataToPluginEPN7WebCore6WidgetE

No need to update the .order files. They are re-generated before releases.

[Daniel Bates]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:1178
>> +    if (pluginWidget.isPluginView())
> 
> Looks like a possible behavior change here? We used to reset m_manualLoader to nullptr when pluginWidget was null.

Notice that pluginWidget can never be null. That is what led me to change all Widget* to Widget&. I tried to convey this in my remark "No other port made use of this machinery" and the per-file remark for the file loader/EmptyClients.cpp in the WebCore ChangeLog entry. I can make these remarks more explicit or do you have suggestions on how I can better convey this?

[Daniel Bates]

(In reply to comment #4)
> > Source/WebKit2/mac/WebKit2.order:-6993
> > -__ZN6WebKit20WebFrameLoaderClient20redirectDataToPluginEPN7WebCore6WidgetE
> 
> No need to update the .order files. They are re-generated before releases.

Will undo the changes to the .order files before landing.

[Chris Dumez]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

> Source/WebCore/html/PluginDocument.cpp:-171
> -    frame()->loader().client().redirectDataToPlugin(nullptr);

What confuses is that it is called with null here.

[Brent Fulgham]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

This patch looks good. Are there existing plugin tests that confirm we do not break functionality?

> Source/WebCore/html/PluginDocument.cpp:130
>          if (Widget* widget = renderer->widget()) {

Could be "auto*"

[Brent Fulgham]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

>> Source/WebCore/html/PluginDocument.cpp:-171
>> -    frame()->loader().client().redirectDataToPlugin(nullptr);
> 
> What confuses is that it is called with null here.

I think this is pretty clearly explained in the ChangeLog. This code was added to support specific Chromium behavior, which no longer exists. We can get rid of this, which means we can now pass a reference everywhere.

[Chris Dumez]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

>>> Source/WebCore/html/PluginDocument.cpp:-171
>>> -    frame()->loader().client().redirectDataToPlugin(nullptr);
>> 
>> What confuses is that it is called with null here.
> 
> I think this is pretty clearly explained in the ChangeLog. This code was added to support specific Chromium behavior, which no longer exists. We can get rid of this, which means we can now pass a reference everywhere.

Yes, I read the Changelog. It still seemed to me that there may be a behavior change for the Window port as well as we were calling with null here and the windows implementation would reset m_manualLoader to null. After this change, redirectDataToPlugin() no longer resets m_manualLoader to null on Windows. This may be completely fine but I thought I would point out the possible behavior change.

[Brent Fulgham]

Comment on attachment 301723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301723&action=review

>>>> Source/WebCore/html/PluginDocument.cpp:-171
>>>> -    frame()->loader().client().redirectDataToPlugin(nullptr);
>>> 
>>> What confuses is that it is called with null here.
>> 
>> I think this is pretty clearly explained in the ChangeLog. This code was added to support specific Chromium behavior, which no longer exists. We can get rid of this, which means we can now pass a reference everywhere.
> 
> Yes, I read the Changelog. It still seemed to me that there may be a behavior change for the Window port as well as we were calling with null here and the windows implementation would reset m_manualLoader to null. After this change, redirectDataToPlugin() no longer resets m_manualLoader to null on Windows. This may be completely fine but I thought I would point out the possible behavior change.

Oh, I see!

We probably don't care, because the Windows port (at least here at Apple) does not permit the use of plugins.

This could be an issue for the WinCairo ports, though.

[Chris Dumez]

(In reply to comment #11)
> Comment on attachment 301723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301723&action=review
> 
> >>>> Source/WebCore/html/PluginDocument.cpp:-171
> >>>> -    frame()->loader().client().redirectDataToPlugin(nullptr);
> >>> 
> >>> What confuses is that it is called with null here.
> >> 
> >> I think this is pretty clearly explained in the ChangeLog. This code was added to support specific Chromium behavior, which no longer exists. We can get rid of this, which means we can now pass a reference everywhere.
> > 
> > Yes, I read the Changelog. It still seemed to me that there may be a behavior change for the Window port as well as we were calling with null here and the windows implementation would reset m_manualLoader to null. After this change, redirectDataToPlugin() no longer resets m_manualLoader to null on Windows. This may be completely fine but I thought I would point out the possible behavior change.
> 
> Oh, I see!
> 
> We probably don't care, because the Windows port (at least here at Apple)
> does not permit the use of plugins.
> 
> This could be an issue for the WinCairo ports, though.

Good, this was my only potential worry. The patch looks good otherwise.
I'll let Brent r+ then.

[Brent Fulgham]

Comment on attachment 301723 [details]
Patch

This change looks good. We can work with WinCairo to help address any (unlikely) fallout. r=me.

[Daniel Bates]

(In reply to comment #10)
> Comment on attachment 301723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301723&action=review
> 
> >>> Source/WebCore/html/PluginDocument.cpp:-171
> >>> -    frame()->loader().client().redirectDataToPlugin(nullptr);
> >> 
> >> What confuses is that it is called with null here.
> > 
> > I think this is pretty clearly explained in the ChangeLog. This code was added to support specific Chromium behavior, which no longer exists. We can get rid of this, which means we can now pass a reference everywhere.
> 
> Yes, I read the Changelog. It still seemed to me that there may be a
> behavior change for the Window port as well as we were calling with null
> here and the windows implementation would reset m_manualLoader to null.
> After this change, redirectDataToPlugin() no longer resets m_manualLoader to
> null on Windows.

Notice that <https://trac.webkit.org/changeset/125500> (bug #93283) added this branch to the Windows code such that we reset m_manualLoader to null. I take it you are saying that the Windows port may now depend on this functionality to behave correctly?

[WebKit Commit Bot]

Comment on attachment 301723 [details]
Patch

Clearing flags on attachment: 301723

Committed r212518: <http://trac.webkit.org/changeset/212518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.