Bug 124900 - Allow the QuickTime plug-in to be replaced by script in an isolated word
Summary: Allow the QuickTime plug-in to be replaced by script in an isolated word
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-26 13:49 PST by Eric Carlson
Modified: 2013-12-06 09:47 PST (History)
17 users (show)

See Also:


Attachments
Patch for the bots to chew on (122.84 KB, patch)
2013-11-26 14:41 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Rebased patch. (125.01 KB, patch)
2013-11-26 14:46 PST, Eric Carlson
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (125.59 KB, patch)
2013-11-26 14:59 PST, Eric Carlson
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (127.21 KB, patch)
2013-11-26 17:29 PST, Eric Carlson
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Another patch (127.04 KB, patch)
2013-11-27 11:16 PST, Eric Carlson
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Another attempt (135.11 KB, patch)
2013-11-27 12:39 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-11-26 13:49:43 PST
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.
Comment 1 Eric Carlson 2013-11-26 13:50:43 PST
<rdar://problem/15349834>
Comment 2 Eric Carlson 2013-11-26 14:41:06 PST
Created attachment 217902 [details]
Patch for the bots to chew on
Comment 3 Eric Carlson 2013-11-26 14:46:11 PST
Created attachment 217903 [details]
Rebased patch.
Comment 4 WebKit Commit Bot 2013-11-26 14:47:33 PST
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.
Comment 5 EFL EWS Bot 2013-11-26 14:54:03 PST
Comment on attachment 217903 [details]
Rebased patch.

Attachment 217903 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/36968040
Comment 6 Eric Carlson 2013-11-26 14:59:54 PST
Created attachment 217906 [details]
Updated patch
Comment 7 EFL EWS Bot 2013-11-26 15:40:41 PST
Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/36978032
Comment 8 Build Bot 2013-11-26 15:49:23 PST
Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/36218047
Comment 9 EFL EWS Bot 2013-11-26 16:04:57 PST
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
Comment 10 kov's GTK+ EWS bot 2013-11-26 16:13:32 PST
Comment on attachment 217906 [details]
Updated patch

Attachment 217906 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/37028028
Comment 11 Eric Carlson 2013-11-26 17:29:29 PST
Created attachment 217913 [details]
Updated patch
Comment 12 EFL EWS Bot 2013-11-26 18:12:59 PST
Comment on attachment 217913 [details]
Updated patch

Attachment 217913 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/37028046
Comment 13 Build Bot 2013-11-26 18:17:48 PST
Comment on attachment 217913 [details]
Updated patch

Attachment 217913 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/37658001
Comment 14 Eric Carlson 2013-11-27 11:16:42 PST
Created attachment 217959 [details]
Another patch

Should fix the EFL bot.
Comment 15 Philippe Normand 2013-11-27 11:42:38 PST
To fix the GTK build I think Modules/plugins should be added in webcore_cppflags in WebCore/GNUmakefile.am
Comment 16 kov's GTK+ EWS bot 2013-11-27 11:55:13 PST
Comment on attachment 217959 [details]
Another patch

Attachment 217959 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/38408063
Comment 17 Build Bot 2013-11-27 12:07:05 PST
Comment on attachment 217959 [details]
Another patch

Attachment 217959 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/38528016
Comment 18 Eric Carlson 2013-11-27 12:33:29 PST
(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.
Comment 19 Eric Carlson 2013-11-27 12:39:14 PST
Created attachment 217964 [details]
Another attempt
Comment 20 Dean Jackson 2013-11-27 13:57:18 PST
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.
Comment 21 Eric Carlson 2013-11-27 17:03:51 PST
Committed in r159827: https://trac.webkit.org/r159827
Comment 22 Eric Carlson 2013-12-03 12:10:12 PST
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.
Comment 23 Alexey Proskuryakov 2013-12-06 09:12:15 PST
plugins/quicktime-plugin-replacement.html has been failing on Mavericks Release WK1 tester 100% of time.

Should this change be rolled out?
Comment 24 Jer Noble 2013-12-06 09:36:18 PST
(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.
Comment 25 Alexey Proskuryakov 2013-12-06 09:47:15 PST
Please fix.