[webkit-reviews] review granted: [Bug 124900] Allow the QuickTime plug-in to be replaced by script in an isolated word : [Attachment 217964] Another attempt

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 27 13:58:51 PST 2013


Dean Jackson <dino at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 124900: Allow the QuickTime plug-in to be replaced by script in an isolated
word
https://bugs.webkit.org/show_bug.cgi?id=124900

Attachment 217964: Another attempt
https://bugs.webkit.org/attachment.cgi?id=217964&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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.


More information about the webkit-reviews mailing list