Bug 103342 – [EFL][WK2] Add ewk_settings APIs for text autosizing

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

[Jaehun Lim]

Add ewk_settings_text_autosizing_enabled_get / set() functions.

[Jaehun Lim]

Created attachment 176138 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 176138 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Text autosizing is disabled by default.

I think this API is not meaningless if text autosizing is disabled.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:354
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);

I think we don't need to keep this line when disabling TEXT_AUTOSIZING

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:368
> +#if ENABLE(TEXT_AUTOSIZING)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:370
> +#else

Missing UNUSED_PARAM

[Kenneth Rohde Christiansen]

Comment on attachment 176138 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:354
>> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
> 
> I think we don't need to keep this line when disabling TEXT_AUTOSIZING

The line is correct and people could give a null pointer.

I say leave it

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 176138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176138&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:354
> >> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
> > 
> > I think we don't need to keep this line when disabling TEXT_AUTOSIZING
> 
> The line is correct and people could give a null pointer.
> 
> I say leave it

Ok, I don't mind this. Jaehun, please fix I pointed out.

[Gyuyoung Kim]

(In reply to comment #2)
> (From update of attachment 176138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176138&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Text autosizing is disabled by default.
> 
> I think this API is not meaningless if text autosizing is disabled.

typo : not meaningless -> meaningless.

Kenneth, what do you think about this? Do you think this API can be landed without enabling TEXT_AUTOSIZING ?

[Gyuyoung Kim]

Comment on attachment 176138 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:354
>>>> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
>>> 
>>> I think we don't need to keep this line when disabling TEXT_AUTOSIZING
>> 
>> The line is correct and people could give a null pointer.
>> 
>> I say leave it
> 
> Ok, I don't mind this. Jaehun, please fix I pointed out.

I missed one thing. EFL port has used below line. To be compliant with existing implementation, I prefer to use below style. My first comment meant this. 

#if ENABLE(TEXT_AUTOSIZING)
   EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
   settings->preferences()->setTextAutosizingEnabled(enable);

   return true;
#else
   UNUSED_PARAM(enable);
   return false;
#endif
}

For example,

Eina_Bool ewk_settings_fullscreen_enabled_get(const Ewk_Settings* settings)
{
#if ENABLE(FULLSCREEN_API)
    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);
    return settings->preferences()->fullScreenEnabled();
#else
    return false;
#endif
}

[Jaehun Lim]

Created attachment 178160 [details]
Fixed patch

I fixed my patch according to Gyuyoung's comments.
Gyuyoung, Please review again.

[Jaehun Lim]

Created attachment 178162 [details]
Fixed patch

Fixed patch

[Jaehun Lim]

Created attachment 178167 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 178167 [details]
Patch

Please enable text autosizing in the build system soon

[WebKit Review Bot]

Comment on attachment 178167 [details]
Patch

Clearing flags on attachment: 178167

Committed r136941: <http://trac.webkit.org/changeset/136941>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jaehun Lim]

(In reply to comment #10)
> (From update of attachment 178167 [details])
> Please enable text autosizing in the build system soon

I'll make another bug to enable TEXT_AUTOSIZING soon.

But there is one problem how to define "HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP" in  http://trac.webkit.org/browser/trunk/Source/WebCore/page/Settings.cpp#L140.

I'll enable it after resolving https://bugs.webkit.org/show_bug.cgi?id=94371.