Summary: | Remove Chromium-specific code to call FrameLoaderClient::redirectDataToPlugin(nullptr) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | aestes, ap, bfulgham, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, japhet | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Daniel Bates
2017-02-16 00:23:45 PST
Created attachment 301723 [details]
Patch
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). 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. 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? (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. 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. 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*" 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. 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. 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. (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. Comment on attachment 301723 [details]
Patch
This change looks good. We can work with WinCairo to help address any (unlikely) fallout. r=me.
(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? Comment on attachment 301723 [details] Patch Clearing flags on attachment: 301723 Committed r212518: <http://trac.webkit.org/changeset/212518> All reviewed patches have been landed. Closing bug. |