[webkit-reviews] review granted: [Bug 123097] [GTK] MEDIA_CONTROLS_SCRIPT support : [Attachment 223000] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Xabier Rodríguez Calvar
<calvaris at igalia.com>'s request for review:
Bug 123097: [GTK] MEDIA_CONTROLS_SCRIPT support
https://bugs.webkit.org/show_bug.cgi?id=123097

Attachment 223000: Patch
https://bugs.webkit.org/attachment.cgi?id=223000&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223000&action=review


> Source/WebCore/CMakeLists.txt:3170
> +    add_custom_command(
> +	   OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScriptsData.cpp
${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScripts.h
> +	   MAIN_DEPENDENCY ${WEBCORE_DIR}/css/make-css-file-arrays.pl
> +	   DEPENDS ${WebCore_USER_AGENT_SCRIPTS}
${WEBCORE_DIR}/bindings/scripts/preprocessor.pm
> +	   COMMAND ${PERL_EXECUTABLE} -I${WEBCORE_DIR}/bindings/scripts
${WEBCORE_DIR}/css/make-css-file-arrays.pl --defines
"${FEATURE_DEFINES_WITH_SPACE_SEPARATOR}" --preprocessor
"${CODE_GENERATOR_PREPROCESSOR}"
${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScripts.h
${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScriptsData.cpp
${WebCore_USER_AGENT_SCRIPTS}
> +	   VERBATIM)

FYI: I'm working on a patch which would use JSMin rather than the clang/gcc
preprocessor for JavaScript files. See bug #127559.

> Source/WebCore/GNUmakefile.am:273
> +USER_AGENT_SCRIPT_FILES =
$(WebCore)/Modules/mediacontrols/mediaControlsApple.js
$(WebCore)/Modules/mediacontrols/mediaControlsGtk.js
> +DerivedSources/WebCore/UserAgentScriptsData.cpp:
DerivedSources/WebCore/UserAgentScripts.h
> +DerivedSources/WebCore/UserAgentScripts.h:
$(WebCore)/css/make-css-file-arrays.pl
$(WebCore)/bindings/scripts/preprocessor.pm $(USER_AGENT_SCRIPT_FILES)
WebKitFeatures.txt
> +	$(AM_V_at)$(PERL) -I$(WebCore)/bindings/scripts $< --defines
"$(feature_defines)" $@ DerivedSources/WebCore/UserAgentScriptsData.cpp
$(USER_AGENT_SCRIPT_FILES)

Ditto.

r=me, with a couple of nits:

> Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:27
> +    inheritFrom: function(parent) {
> +	   for (var property in parent) {
> +	       if (!this.hasOwnProperty(property))
> +		   this[property] = parent[property];
> +	   }
> +    },

Since this method is used in two subclasses of Controller, we should consider
moving this method into the base class. But that work can be done in another
bug.

> LayoutTests/media/click-volume-bar-not-pausing.html:54
> +		   setTimeout(function() {
> +		       // Forcing relayout calculations to say that you are
> +		       // triggering the volume slider to show up for
> +		       // controls that work that way.
> +		       document.body.offsetTop;
> +
> +		       volumeSliderCoordinates =
calculateElementCoordinates("volume-slider");
> +		       eventSender.mouseMoveTo(volumeSliderCoordinates[0],
volumeSliderCoordinates[1]);
> +
> +		       eventSender.mouseDown();
> +		       eventSender.mouseUp();
> +		   }, 130);

Why 130ms? Is it because of your fade animation?  An opacity animation should
affect the volume slider clickability.

> LayoutTests/media/media-volume-slider-rendered-normal.html:62
> +	       }, 130);

Ditto, w.r.t. 130ms.