Bug 131257

Summary: [EFL] Turn on ENABLE_MEDIA_CONTROLS_SCRIPT
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, calvaris, cdumez, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, gyuyoung.kim, jer.noble, lucas.de.marchi, macpherson, menard, philipj, rakuco, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 131044    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2014-04-04 21:08:55 PDT
ENABLE_MEDIA_CONTROLS_SCRIPTS support javascript/css based media controls and MAC and GTK port use it.
Comment 1 Ryuan Choi 2014-04-04 21:19:06 PDT
Created attachment 228652 [details]
Patch
Comment 2 Ryuan Choi 2014-04-04 21:41:34 PDT
Created attachment 228654 [details]
Patch
Comment 3 Gyuyoung Kim 2014-04-05 02:24:25 PDT
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.
Comment 4 Ryuan Choi 2014-04-05 02:31:01 PDT
(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.
Comment 5 Gyuyoung Kim 2014-04-05 02:36:49 PDT
(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.
Comment 6 Xabier Rodríguez Calvar 2014-04-05 03:32:24 PDT
(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.
Comment 7 Xabier Rodríguez Calvar 2014-04-05 03:38:31 PDT
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.
Comment 8 Ryuan Choi 2014-04-07 04:19:13 PDT
Created attachment 228726 [details]
Patch
Comment 9 Ryuan Choi 2014-04-07 04:21:31 PDT
(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.
Comment 10 Xabier Rodríguez Calvar 2014-04-07 04:37:59 PDT
(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.
Comment 11 Gyuyoung Kim 2014-04-07 04:58:17 PDT
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.
Comment 12 WebKit Commit Bot 2014-04-07 05:34:53 PDT
Comment on attachment 228726 [details]
Patch

Clearing flags on attachment: 228726

Committed r166872: <http://trac.webkit.org/changeset/166872>
Comment 13 WebKit Commit Bot 2014-04-07 05:35:03 PDT
All reviewed patches have been landed.  Closing bug.