Bug 113284 – [EFL] Add method in ewk_settings for setting the CSS media type

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jose Lejin PJ]

API needed in ewk_settings for setting the CSS media type.

Refer http://www.w3schools.com/tags/att_style_media.asp
WebKit should know in what media type it is running(tv/screen/ handheld/projection/print etc.) to automatically behave.

WebCore has the method(refer Settings.h) to change the media type, which can be exposed in EFL WebKit API.

[Sunesh Chekkiyil]

Created attachment 195040 [details]
Test App attached.

[Jose Lejin PJ]

Created attachment 198505 [details]
Patch

[Jose Lejin PJ]

> 
> WebCore has the method(refer Settings.h) to change the media type, which can be exposed in EFL WebKit API.

WebCore::Settings::mediaTypeOverride() is used by testing code and we should not use this. We can store directly in ewk_settings.

Please refer comments in https://bugs.webkit.org/show_bug.cgi?id=113853

[Gyuyoung Kim]

Comment on attachment 198505 [details]
Patch

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

Please read WebKit EFL coding style : http://trac.webkit.org/wiki/EFLWebKitCodingStyle

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:429
> +    if (ewk_settings_css_media_type_get())

Why should we call this function twice ? Look at the implementation on mac port.

http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm#L1948

> Source/WebKit/efl/ewk/ewk_settings.cpp:393
> +void ewk_settings_css_media_type_set(const char *type)

Wrong * place.

> Source/WebKit/efl/ewk/ewk_settings.cpp:398
> +const char* ewk_settings_css_media_type_get(void)

Do not use *void* in implementation file. We use WebKit coding style except for public header.

> Source/WebKit/efl/ewk/ewk_settings.h:396
> + * @param type css media type to be set, must be write-able.

We have not used . at @param, @return.

> Source/WebKit/efl/ewk/ewk_settings.h:407
> + * @return css media type set by user.

ditto.

[Jose Lejin PJ]

(In reply to comment #4)
> (From update of attachment 198505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198505&action=review
> 
> Please read WebKit EFL coding style : http://trac.webkit.org/wiki/EFLWebKitCodingStyle
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:429
> > +    if (ewk_settings_css_media_type_get())
> 
> Why should we call this function twice ? Look at the implementation on mac port.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm#L1948
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:393
> > +void ewk_settings_css_media_type_set(const char *type)
> 
> Wrong * place.
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:398
> > +const char* ewk_settings_css_media_type_get(void)
> 
> Do not use *void* in implementation file. We use WebKit coding style except for public header.
> 
> > Source/WebKit/efl/ewk/ewk_settings.h:396
> > + * @param type css media type to be set, must be write-able.
> 
> We have not used . at @param, @return.
> 
> > Source/WebKit/efl/ewk/ewk_settings.h:407
> > + * @return css media type set by user.
> 
> ditto.

Thanks for review Gyuyoung Kim. I will provide updated patch.

[Jose Lejin PJ]

Created attachment 198676 [details]
Patch

Updated patch

[Gyuyoung Kim]

Comment on attachment 198676 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:3
> +        Add method in ewk_settings for setting the CSS media type

Please add [EFL] prefix.

> Source/WebKit/efl/ChangeLog:6
> +        Reviewed by Gyuyoung Kim.

I don't set r+ yet. Remove this.

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:431
> +        return overrideType;

return String::fromUTF8(overrideType); ?

[Jose Lejin PJ]

Created attachment 198677 [details]
Patch

Updated patch

[Gyuyoung Kim]

Comment on attachment 198677 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:431
> +        return String(overrideType);

I think it would be good if you use String::utf8() to treat invalid ASCII input string.

[Jose Lejin PJ]

Created attachment 198686 [details]
Patch

Updated patch

[Gyuyoung Kim]

Comment on attachment 198686 [details]
Patch

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

LGTM otherwise.

> Source/WebKit/efl/ewk/ewk_settings.h:407
> + * @return css media type set by user

I think we need to add this comment as well. "or @c NULL if none is set"

[Jose Lejin PJ]

Created attachment 198699 [details]
Patch

Patch for commit

[Jose Lejin PJ]

(In reply to comment #11)
> (From update of attachment 198686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198686&action=review
> 
> LGTM otherwise.
> 
> > Source/WebKit/efl/ewk/ewk_settings.h:407
> > + * @return css media type set by user
> 
> I think we need to add this comment as well. "or @c NULL if none is set"

Added this in updated patch.

[Gyuyoung Kim]

Ah, I missed to check if there is unit test. We have added unit test whenever adding a new public APIs. Please add API test.

[Jose Lejin PJ]

(In reply to comment #14)
> Ah, I missed to check if there is unit test. We have added unit test whenever adding a new public APIs. Please add API test.

Can you please tell on which file existing ewk_settings API tests are present. I will refer that and add tests for this new API.

[Jose Lejin PJ]

(In reply to comment #14)
> Ah, I missed to check if there is unit test. We have added unit test whenever adding a new public APIs. Please add API test.

After checking in IRC, looks like tests are added for WK2 only for ewk_settings APIs.

[Gyuyoung Kim]

(In reply to comment #16)
> (In reply to comment #14)
> > Ah, I missed to check if there is unit test. We have added unit test whenever adding a new public APIs. Please add API test.
> 
> After checking in IRC, looks like tests are added for WK2 only for ewk_settings APIs.

We are running WK1 unit tests on EFL buildbot as well.

test_ewk_contextmenu
test_ewk_frame
test_ewk_view

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release/builds/11767/steps/API%20tests/logs/stdio

As you know, there is no test_ewk_setting for WK1 yet. So, I mean you need to add test_ewk_setting which tests this new APIs.

[Chris Dumez]

Comment on attachment 198699 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:429
> +    const char* overrideType = ewk_settings_css_media_type_get();

or simply return String::fromUTF8(ewk_settings_css_media_type_get());

Passing 0 to String::fromUTF8() will return String().

> Source/WebKit/efl/ewk/ewk_settings.h:407
> + * @return css media type set by user or @c NULL if none is set

We usually document that the returned string is an eina stringshare. Please use other APIs as reference.

[Jose Lejin PJ]

(In reply to comment #18)
> (From update of attachment 198699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198699&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:429
> > +    const char* overrideType = ewk_settings_css_media_type_get();
> 
> or simply return String::fromUTF8(ewk_settings_css_media_type_get());
> 
> Passing 0 to String::fromUTF8() will return String().

Thanks for review. I will use this single return.
 
> > Source/WebKit/efl/ewk/ewk_settings.h:407
> > + * @return css media type set by user or @c NULL if none is set
> 
> We usually document that the returned string is an eina stringshare. Please use other APIs as reference.

Yes. I saw this now in other APIs. I update documentation.

[Jose Lejin PJ]

Created attachment 198833 [details]
Patch

Added unit tests

[Gyuyoung Kim]

Comment on attachment 198833 [details]
Patch

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

Thank you for your updated patch.

> Source/WebKit/ChangeLog:8
> +        * PlatformEfl.cmake:

It would be good if you mention test_ewk_setting is added.

[Chris Dumez]

Comment on attachment 198833 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_settings.h:394
> + * Setting the value to null will restore the default value.

null -> @c NULL

> Source/WebKit/efl/tests/test_ewk_setting.cpp:2
> + * Copyright (C) 2013 Samsung Electronics

Are you working for Samsung?

> Source/WebKit/efl/tests/test_ewk_setting.cpp:38
> +{

Please check the default value ("screen") of ewk_settings_css_media_type_get().

> Source/WebKit/efl/tests/test_ewk_setting.cpp:39
> +    ewk_settings_css_media_type_set("screen");

Since this is the default value, you should probably set something else than "screen" first, to make sure it works.

> Source/WebKit/efl/tests/test_ewk_setting.cpp:40
> +    ASSERT_EQ(strcmp(ewk_settings_css_media_type_get(), "screen"), 0);

We usually use ASSERT_STREQ() for string comparisons.

> Source/WebKit/efl/tests/test_ewk_setting.cpp:47
> +}

Please check resetting the default value by passing NULL.

[Jose Lejin PJ]

(In reply to comment #22)
> null -> @c NULL
Corrected.

> Are you working for Samsung?
I work for Cisco. Earlier was with Samsung :) I will update this.

> > Source/WebKit/efl/tests/test_ewk_setting.cpp:38
> > +{
> 
> Please check the default value ("screen") of ewk_settings_css_media_type_get().

Please see doc of ewk_settings_css_media_type_get(). (It will only return the value set through ewk_settings_css_media_type_set and not the one used internally. So if nothing is set, it will return null. I have updated documentation of ewk_settings_css_media_type_set now to avoid confusion.)

> > Source/WebKit/efl/tests/test_ewk_setting.cpp:39
> > +    ewk_settings_css_media_type_set("screen");
> 
> Since this is the default value, you should probably set something else than "screen" first, to make sure it works.
ok
> 
> > Source/WebKit/efl/tests/test_ewk_setting.cpp:40
> > +    ASSERT_EQ(strcmp(ewk_settings_css_media_type_get(), "screen"), 0);
> 
> We usually use ASSERT_STREQ() for string comparisons.
> 
> > Source/WebKit/efl/tests/test_ewk_setting.cpp:47
> > +}
> 
> Please check resetting the default value by passing NULL.
ok

[Chris Dumez]

(In reply to comment #23)
> (In reply to comment #22)
> > null -> @c NULL
> Corrected.
> 
> > Are you working for Samsung?
> I work for Cisco. Earlier was with Samsung :) I will update this.
> 
> > > Source/WebKit/efl/tests/test_ewk_setting.cpp:38
> > > +{
> > 
> > Please check the default value ("screen") of ewk_settings_css_media_type_get().
> 
> Please see doc of ewk_settings_css_media_type_get(). (It will only return the value set through ewk_settings_css_media_type_set and not the one used internally. So if nothing is set, it will return null. I have updated documentation of ewk_settings_css_media_type_set now to avoid confusion.)

Either way, you need to test the default value in the unit test, it being NULL or "screen".

[Jose Lejin PJ]

> Either way, you need to test the default value in the unit test, it being NULL or "screen".

I will add test by comparing to NULL.

Note : style checker not allowing to use NULL and alerting to use 0. So I am using 0 for NULL check.

[Jose Lejin PJ]

Created attachment 198840 [details]
Patch

Updated as per review

[Chris Dumez]

Comment on attachment 198840 [details]
Patch

LGTM. Thanks.

[Gyuyoung Kim]

Comment on attachment 198840 [details]
Patch

LGTM too.

[WebKit Commit Bot]

Comment on attachment 198840 [details]
Patch

Clearing flags on attachment: 198840

Committed r148740: <http://trac.webkit.org/changeset/148740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.