Bug 161424 – Break pluginReplacementEnabled into youTubeFlashPluginReplacementEnabled and quickTimePluginReplacementEnabled

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ricky Mondello]

Break pluginReplacementEnabled into youTubeFlashPluginReplacementEnabled and quickTimePluginReplacementEnabled

[Ricky Mondello]

Created attachment 287484 [details]
First attempt

[Tim Horton]

Comment on attachment 287484 [details]
First attempt

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

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.mm:118
> +bool QuickTimePluginReplacement::isEnabledForSettings(const Settings* settings)

Should this be "isEnabledBySettings"? Not sure.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:349
> +bool YouTubePluginReplacement::isEnabledForSettings(const WebCore::Settings *settings)

Star's on the wrong side.

> Source/WebCore/html/HTMLPlugInElement.cpp:378
>      if (m_pluginReplacement)
>          return true;

Not familiar with this code; is there any case where we'll take this path when we would have previously bailed because of the setting?

> Source/WebCore/testing/InternalSettings.idl:61
> +    [RaisesException] void setYouTubeFlashPluginReplacementEnabled(boolean enabled);

I hope the next patch adds a layout test that exercises this!

[Eric Carlson]

Comment on attachment 287484 [details]
First attempt

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

r=me as long as you get it to compile before submitting :-)

>> Source/WebCore/html/HTMLPlugInElement.cpp:378
>>          return true;
> 
> Not familiar with this code; is there any case where we'll take this path when we would have previously bailed because of the setting?

Maybe not a huge concern, but this change means that the setting can't turned off after a replacement has been created. Maybe something like this instead?

if (m_pluginReplacement)
    return m_pluginReplacement->isEnabledForSettings(document().settings())

[Ricky Mondello]

Created attachment 287523 [details]
Rename a member function, fix a *, hope it builds

It's not clear why my first attempt didn't build. Maybe due to generated code?

[Ricky Mondello]

Dean and I think that the failure is an EWS issue. Dean said he's seen this before when changing Internal settings. I'm going to put the second patch up for review.

[Dean Jackson]

(In reply to comment #5)
> Dean and I think that the failure is an EWS issue. Dean said he's seen this
> before when changing Internal settings. I'm going to put the second patch up
> for review.

Yeah. The build error is saying that JSInternalSettingsGenerated.cpp is still trying to compile something that is no longer in the IDL, so it looks like EWS isn't regenerating that file.

[Tim Horton]

(In reply to comment #6)
> (In reply to comment #5)
> > Dean and I think that the failure is an EWS issue. Dean said he's seen this
> > before when changing Internal settings. I'm going to put the second patch up
> > for review.
> 
> Yeah. The build error is saying that JSInternalSettingsGenerated.cpp is
> still trying to compile something that is no longer in the IDL, so it looks
> like EWS isn't regenerating that file.

This is a longstanding problem with removing things from that file.

[Ricky Mondello]

Created attachment 287557 [details]
Fix the contentSecurityPolicy tests

[WebKit Commit Bot]

Comment on attachment 287557 [details]
Fix the contentSecurityPolicy tests

Clearing flags on attachment: 287557

Committed r205271: <http://trac.webkit.org/changeset/205271>

[WebKit Commit Bot]

Comment on attachment 287523 [details]
Rename a member function, fix a *, hope it builds

Rejecting attachment 287523 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 287523, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
esting/InternalSettings.h
Hunk #1 FAILED at 91.
Hunk #2 FAILED at 157.
2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/testing/InternalSettings.h.rej
patching file Source/WebCore/testing/InternalSettings.idl
Hunk #1 FAILED at 57.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/testing/InternalSettings.idl.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dean Jackson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/1983339

[Anne van Kesteren]

As per comment 9 the code landed here. Not sure all of it did, but at this point that's probably best tracked from a new report.

[Radar WebKit Bug Importer]

<rdar://problem/117690334>