Bug 131257 – [EFL] Turn on ENABLE_MEDIA_CONTROLS_SCRIPT

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

ENABLE_MEDIA_CONTROLS_SCRIPTS support javascript/css based media controls and MAC and GTK port use it.

[Ryuan Choi]

Created attachment 228652 [details]
Patch

[Ryuan Choi]

Created attachment 228654 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 228654 [details]
Patch

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

Don't we need to rebaseline or skip on media control test cases ? Besides I wonder how to support media control UI(e.g. media control buttons). Can we share the common media control buttons with other ports if we enable the ENABLE_MEDIA_CONTROLS_SCRIPT ?

> Source/WebCore/ChangeLog:9
> +        Copied from mediaControlsApple.css until Efl port have specific desigin.

Typo : s/have/has/g, s/desigin/design/g

> Source/WebCore/ChangeLog:11
> +        Copied from mediaControlsApple.js until Efl port have specific desigin.

ditto.

[Ryuan Choi]

(In reply to comment #3)
> (From update of attachment 228654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228654&action=review
> 
> Don't we need to rebaseline or skip on media control test cases ? Besides I wonder how to support media control UI(e.g. media control buttons). Can we share the common media control buttons with other ports if we enable the ENABLE_MEDIA_CONTROLS_SCRIPT ?
>

Yes, we should rebaseline test cases.
Should I do it in this bug?
I considered that it looks big huge but I can do if you want.

MEDIA_CONTROLS_SCRIPT provides the media control which is implemented by javascript and css.

Now, I copied them from mac port so media control is same with mac port now.
GTK port has their own javascript/css for media control.

> > Source/WebCore/ChangeLog:9
> > +        Copied from mediaControlsApple.css until Efl port have specific desigin.
> 
> Typo : s/have/has/g, s/desigin/design/g
> 
> > Source/WebCore/ChangeLog:11
> > +        Copied from mediaControlsApple.js until Efl port have specific desigin.
> 
> ditto.

Thanks, I will fix it.

[Gyuyoung Kim]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 228654 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=228654&action=review
> > 
> > Don't we need to rebaseline or skip on media control test cases ? Besides I wonder how to support media control UI(e.g. media control buttons). Can we share the common media control buttons with other ports if we enable the ENABLE_MEDIA_CONTROLS_SCRIPT ?
> >
> 
> Yes, we should rebaseline test cases.
> Should I do it in this bug?
> I considered that it looks big huge but I can do if you want.

Yes, we also should handle the test case if it may influence on our test result.


> MEDIA_CONTROLS_SCRIPT provides the media control which is implemented by javascript and css.
> 
> Now, I copied them from mac port so media control is same with mac port now.
> GTK port has their own javascript/css for media control.

Ok, I see.

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> >Can we share the common media control buttons with other ports if we enable the ENABLE_MEDIA_CONTROLS_SCRIPT ?
> >
> 
> MEDIA_CONTROLS_SCRIPT provides the media control which is implemented by javascript and css.
> 
> Now, I copied them from mac port so media control is same with mac port now.
> GTK port has their own javascript/css for media control.

In GTK we decided to share the widgets and provide a subclass for the specific bits. This has worked perfectly so far and in the future, if you decide to separate them, it is easier than merging.

[Xabier Rodríguez Calvar]

Comment on attachment 228654 [details]
Patch

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

Complementing my previous comment.

>> Source/WebCore/ChangeLog:11
>> +        Copied from mediaControlsApple.js until Efl port have specific desigin.
> 
> ditto.

As I said, I would consider seriously to share the code with Apple, for that you can take GTK controls as example and define a skeleton for the subclass or even leaving it for later, because maybe your designers do not define any specific behavior.

> Source/WebCore/Modules/mediacontrols/mediaControlsEfl.js:1
> +/*

If you decide to go for sharing, you need to take GTK as skeleton of even remove this.

> Source/WebCore/PlatformEfl.cmake:215
> +set(WebCore_USER_AGENT_SCRIPTS
> +    ${WEBCORE_DIR}/Modules/mediacontrols/mediaControlsEfl.js
> +)

If you decide for sharing, you need to add Apple controls here too.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1042
> +    return ASCIILiteral(mediaControlsEflJavaScript);

If you decide for sharing, you need to touch this too.

[Ryuan Choi]

Created attachment 228726 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (In reply to comment #4)
> > >Can we share the common media control buttons with other ports if we enable the ENABLE_MEDIA_CONTROLS_SCRIPT ?
> > >
> > 
> > MEDIA_CONTROLS_SCRIPT provides the media control which is implemented by javascript and css.
> > 
> > Now, I copied them from mac port so media control is same with mac port now.
> > GTK port has their own javascript/css for media control.
> 
> In GTK we decided to share the widgets and provide a subclass for the specific bits. This has worked perfectly so far and in the future, if you decide to separate them, it is easier than merging.

Thanks for information.

I removed mediaControlsEfl.js/css.

When Efl port will extend the control, I will reference Gtk port.

[Xabier Rodríguez Calvar]

(In reply to comment #9)
> Thanks for information.

My pleasure.

> I removed mediaControlsEfl.js/css.
> 
> When Efl port will extend the control, I will reference Gtk port.

Yep, it looks better now.

[Gyuyoung Kim]

Comment on attachment 228726 [details]
Patch

LGTM. BTW, When will you extend media control for EFL port ? EFL port may need to have specific controls in future.

[WebKit Commit Bot]

Comment on attachment 228726 [details]
Patch

Clearing flags on attachment: 228726

Committed r166872: <http://trac.webkit.org/changeset/166872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.