Bug 124900 – Allow the QuickTime plug-in to be replaced by script in an isolated word

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Carlson]

Create a mechanism that allows script running in a isolated world to create element(s) in the <embed> or <object> shadow DOM to replace a plug-in, add a replacement for the QuickTime plug-in.

[Eric Carlson]

<rdar://problem/15349834>

[Eric Carlson]

Created attachment 217902 [details]
Patch for the bots to chew on

[Eric Carlson]

Created attachment 217903 [details]
Rebased patch.

[WebKit Commit Bot]

Attachment 217903 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/wincairo/TestExpectations', u'LayoutTests/plugins/quicktime-plugin-replacement-expected.txt', u'LayoutTests/plugins/quicktime-plugin-replacement.html', u'LayoutTests/plugins/resources/orange.mov', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/Modules/plugins/PluginReplacement.h', u'Source/WebCore/Modules/plugins/QuickTimePluginReplacement.cpp', u'Source/WebCore/Modules/plugins/QuickTimePluginReplacement.css', u'Source/WebCore/Modules/plugins/QuickTimePluginReplacement.h', u'Source/WebCore/Modules/plugins/QuickTimePluginReplacement.idl', u'Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h', u'Source/WebCore/bindings/js/JSDOMBinding.h', u'Source/WebCore/bindings/js/JSPluginElementFunctions.cpp', u'Source/WebCore/html/HTMLEmbedElement.cpp', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/html/HTMLObjectElement.cpp', u'Source/WebCore/html/HTMLPlugInElement.cpp', u'Source/WebCore/html/HTMLPlugInElement.h', u'Source/WebCore/html/HTMLPlugInImageElement.cpp', u'Source/WebCore/html/HTMLPlugInImageElement.h', u'Source/WebCore/html/HTMLVideoElement.h', u'Source/WebCore/platform/graphics/MediaPlayer.cpp', u'Source/WebCore/platform/graphics/MediaPlayer.h', u'Source/WebCore/platform/graphics/MediaPlayerPrivate.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h', u'Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h', u'Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl']" exit_code: 1
Source/WebCore/Modules/plugins/PluginReplacement.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 217903 [details]
Rebased patch.

Attachment 217903 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/36968040

[Eric Carlson]

Created attachment 217906 [details]
Updated patch

[EFL EWS Bot]

Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/36978032

[Build Bot]

Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/36218047

[EFL EWS Bot]

Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/36958047

[kov's GTK+ EWS bot]

Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/37028028

[Eric Carlson]

Created attachment 217913 [details]
Updated patch

[EFL EWS Bot]

Comment on attachment 217913 [details]
Updated patch

Attachment 217913 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/37028046

[Build Bot]

Comment on attachment 217913 [details]
Updated patch

Attachment 217913 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/37658001

[Eric Carlson]

Created attachment 217959 [details]
Another patch

Should fix the EFL bot.

[Philippe Normand]

To fix the GTK build I think Modules/plugins should be added in webcore_cppflags in WebCore/GNUmakefile.am

[kov's GTK+ EWS bot]

Comment on attachment 217959 [details]
Another patch

Attachment 217959 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/38408063

[Build Bot]

Comment on attachment 217959 [details]
Another patch

Attachment 217959 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/38528016

[Eric Carlson]

(In reply to comment #15)
> To fix the GTK build I think Modules/plugins should be added in webcore_cppflags in WebCore/GNUmakefile.am

But of course... Thanks.

[Eric Carlson]

Created attachment 217964 [details]
Another attempt

[Dean Jackson]

Comment on attachment 217964 [details]
Another attempt

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

> Source/WebCore/DerivedSources.make:876
> +	USER_AGENT_STYLE_SHEETS  := $(USER_AGENT_STYLE_SHEETS) $(WebCore)/Modules/plugins/QuickTimePluginReplacement.css

Nit: two spaces before :=

> Source/WebCore/DerivedSources.make:893
> +	USER_AGENT_SCRIPTS  := $(USER_AGENT_SCRIPTS) $(WebCore)/Modules/plugins/QuickTimePluginReplacement.js

ditto

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.cpp:136
> +bool QuickTimePluginReplacement::ensureReplacementScriptInjected()

Might be worth putting some
LOG(Plugins
in here to indicate what's going on. (and elsewhere)

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.cpp:173
> +    // Lookup the "createPluginReplacement" function

Nit: missing .

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.css:32
> +    z-index: 0;

Why this?

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:89
> +            if (this[property] != undefined)

Probably should use !== here. Well.... maybe.

> null == undefined
true
> null === undefined
false
> null != undefined
false
> null !== undefined
true

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:117
> +        var base = document.createElement("base");

Nit: You use single quotes everywhere else.

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:264
> +        this.video.currentTime = time / this.TimeScale

Nit: Missing ;

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:285
> +    timeScale: function()
> +    {
> +        return 30000;
> +    },

Is this always constant?

> Source/WebCore/html/HTMLPlugInElement.cpp:340
> +static Vector<ReplacementPlugin*>& registeredPluginReplacements()
> +{
> +    DEFINE_STATIC_LOCAL(Vector<ReplacementPlugin*>, registeredReplacements, ());
> +    static bool enginesQueried = false;
> +    
> +    if (enginesQueried)
> +        return registeredReplacements;
> +    enginesQueried = true;
> +
> +#if PLATFORM(MAC)
> +    QuickTimePluginReplacement::registerPluginReplacement(registerPluginReplacement);
> +#endif
> +    
> +    return registeredReplacements;
> +}
> +
> +#if PLATFORM(MAC)
> +static void registerPluginReplacement(const ReplacementPlugin& replacement)
> +{
> +    registeredPluginReplacements().append(new ReplacementPlugin(replacement));
> +}
> +#endif

This confused me for a while. I was trying to work out how registedReplacements gets populated. I wonder if it would be more obvious if this single line function was inline in registeredPluginReplacements(). That would also save having to define it before the function. Lastly, it also confused me that it has the same name as the non-static method.

> Source/WebCore/html/HTMLPlugInElement.h:63
> +        Playing,
> +        PreparingPluginReplacement,
> +        DisplayingPluginReplacement,

Hmmm, I vaguely remember some code that does if displayState < Playing (or one of those above), which I hope will still work with this new ordering.

[Eric Carlson]

Committed in r159827: https://trac.webkit.org/r159827

[Eric Carlson]

Comment on attachment 217964 [details]
Another attempt

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

Oops, I just found these comments in an open window...

>> Source/WebCore/DerivedSources.make:876
>> +	USER_AGENT_STYLE_SHEETS  := $(USER_AGENT_STYLE_SHEETS) $(WebCore)/Modules/plugins/QuickTimePluginReplacement.css
> 
> Nit: two spaces before :=

Fixed.

>> Source/WebCore/DerivedSources.make:893
>> +	USER_AGENT_SCRIPTS  := $(USER_AGENT_SCRIPTS) $(WebCore)/Modules/plugins/QuickTimePluginReplacement.js
> 
> ditto

ditto.

>> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.cpp:136
>> +bool QuickTimePluginReplacement::ensureReplacementScriptInjected()
> 
> Might be worth putting some
> LOG(Plugins
> in here to indicate what's going on. (and elsewhere)

Good idea, fixed.

>> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.cpp:173
>> +    // Lookup the "createPluginReplacement" function
> 
> Nit: missing .

Fixed.

>> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.css:32
>> +    z-index: 0;
> 
> Why this?

No good reason, removed.

>> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:117
>> +        var base = document.createElement("base");
> 
> Nit: You use single quotes everywhere else.

Fixed.

>> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:264
>> +        this.video.currentTime = time / this.TimeScale
> 
> Nit: Missing ;

Fixed.

>> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.js:285
>> +    },
> 
> Is this always constant?

MPEG-4 and QuickTime movie files have timescale in the file, but other media types do not. The old QuickTime API deals with that by having a importer for a media types define a hard code time scale for the that type, but AVFoundation just doesn't expose a time scale at all. 3000 works well in practice because common frame rates can be represented accurately, eg. 29.97 NTSC is 30000 / 1001. I added a comment to the code.

>> Source/WebCore/html/HTMLPlugInElement.cpp:340
>> +#endif
> 
> This confused me for a while. I was trying to work out how registedReplacements gets populated. I wonder if it would be more obvious if this single line function was inline in registeredPluginReplacements(). That would also save having to define it before the function. Lastly, it also confused me that it has the same name as the non-static method.

That won't work because registerPluginReplacement is passed to the module(s) that want to register new types. I changed the names to hopefully make this less confusing.

>> Source/WebCore/html/HTMLPlugInElement.h:63
>> +        DisplayingPluginReplacement,
> 
> Hmmm, I vaguely remember some code that does if displayState < Playing (or one of those above), which I hope will still work with this new ordering.

I *think* I handled all of those cases.

[Alexey Proskuryakov]

plugins/quicktime-plugin-replacement.html has been failing on Mavericks Release WK1 tester 100% of time.

Should this change be rolled out?

[Jer Noble]

(In reply to comment #23)
> plugins/quicktime-plugin-replacement.html has been failing on Mavericks Release WK1 tester 100% of time.
> 
> Should this change be rolled out?

The failure is just some extra logging from AVF.  So, no, this should not be rolled out.

[Alexey Proskuryakov]

Please fix.