Bug 132811

Summary: [EFL][WK2] Add ewk_view_fixed_layout_size_set|get()
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, gyuyoung.kim, hyuki.kim, lucas.de.marchi, ryuan.choi, seojuyung2, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch for landing none

Description Gyuyoung Kim 2014-05-11 22:40:50 PDT
Some mobile devices need to set fixed layout size by own decision. This patch supports to set|get fixed layout size by using WKPageSetFixedLayoutSize(), WKPageGetFixedLayoutSize().
Comment 1 Gyuyoung Kim 2014-05-11 23:17:26 PDT
Created attachment 231274 [details]
WIP
Comment 2 Gyuyoung Kim 2014-05-12 01:35:15 PDT
Created attachment 231277 [details]
Patch
Comment 3 Ryuan Choi 2014-05-12 19:27:09 PDT
Comment on attachment 231277 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:725
> +    EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl);

How about clearing width, height like ewk_view_contents_size_get ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
> +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);

How about Evas_Coord?

Below is example.
EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);
Comment 4 Gyuyoung Kim 2014-05-12 21:24:21 PDT
Comment on attachment 231277 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:725
>> +    EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl);
> 
> How about clearing width, height like ewk_view_contents_size_get ?

I don't know what is benefit when we clear width, height. Any benefit ?

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
>> +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);
> 
> How about Evas_Coord?
> 
> Below is example.
> EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);

As far as I know, Evas_Coord is integer type, isn't it ? In this API, I hope to use *unsigned* because this API needs to handle positive number.
Comment 5 Ryuan Choi 2014-05-12 23:29:52 PDT
(In reply to comment #4)
> (From update of attachment 231277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231277&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:725
> >> +    EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl);
> > 
> > How about clearing width, height like ewk_view_contents_size_get ?
> 
> I don't know what is benefit when we clear width, height. Any benefit ?
> 
Although I don't like it too, it's common in EFL world.

> >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
> >> +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);
> > 
> > How about Evas_Coord?
> > 
> > Below is example.
> > EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);
> 
> As far as I know, Evas_Coord is integer type, isn't it ? In this API, I hope to use *unsigned* because this API needs to handle positive number.

Similarly, EFL didn't use unsigned for the size values before.
Comment 6 Gyuyoung Kim 2014-05-12 23:36:44 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 231277 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=231277&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:725
> > >> +    EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl);
> > > 
> > > How about clearing width, height like ewk_view_contents_size_get ?
> > 
> > I don't know what is benefit when we clear width, height. Any benefit ?
> > 
> Although I don't like it too, it's common in EFL world.

Hmm, if there is no benefit or real yoda style, I would like to avoid to follow EFL style. When we review upcoming similar patch, I don't know how to guide this rule. I just can say "EFL is yoda style, so, you should follow it."


> > >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
> > >> +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);
> > > 
> > > How about Evas_Coord?
> > > 
> > > Below is example.
> > > EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);
> > 
> > As far as I know, Evas_Coord is integer type, isn't it ? In this API, I hope to use *unsigned* because this API needs to handle positive number.
> 
> Similarly, EFL didn't use unsigned for the size values before.

If so, I want to change it as well.
Comment 7 Ryuan Choi 2014-05-13 00:06:13 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 231277 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=231277&action=review
> > > 
> > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:725
> > > >> +    EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl);
> > > > 
> > > > How about clearing width, height like ewk_view_contents_size_get ?
> > > 
> > > I don't know what is benefit when we clear width, height. Any benefit ?
> > > 
> > Although I don't like it too, it's common in EFL world.
> 
> Hmm, if there is no benefit or real yoda style, I would like to avoid to follow EFL style. When we review upcoming similar patch, I don't know how to guide this rule. I just can say "EFL is yoda style, so, you should follow it."
> 
> 

+1
Comment 8 Gyuyoung Kim 2014-05-13 00:26:28 PDT
(In reply to comment #3)
> (From update of attachment 231277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231277&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:725
> > +    EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl);
> 
> How about clearing width, height like ewk_view_contents_size_get ?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
> > +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);
> 
> How about Evas_Coord?
> 
> Below is example.
> EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);

CC'ing juyoung. Juyoung, as mentioned in comment #3, has EFL coding rule or implicit coding style that ryuan pointed out ?
Comment 9 Chris Dumez 2014-05-13 08:50:30 PDT
Comment on attachment 231277 [details]
Patch

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

r=me % nits

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:903
> + * The webview size will be set with given size. The size value should be set to "positive number".

Nit: I don't think "The size value should be set to "positive number"" is very useful here.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:909
> +EAPI void ewk_view_layout_fixed_size_set(const Evas_Object *o, unsigned width, unsigned height);

nit: Evas_Coord

>>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
>>>>> +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);
>>>> 
>>>> How about Evas_Coord?
>>>> 
>>>> Below is example.
>>>> EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);
>>> 
>>> As far as I know, Evas_Coord is integer type, isn't it ? In this API, I hope to use *unsigned* because this API needs to handle positive number.
>> 
>> Similarly, EFL didn't use unsigned for the size values before.
> 
> If so, I want to change it as well.

nit: +1 for Evas_Coord

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1093
> +    unsigned width;

nit: please initialize to 0

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1094
> +    unsigned height;

ditto.
Comment 10 Gyuyoung Kim 2014-05-13 23:07:59 PDT
Created attachment 231436 [details]
Patch for landing
Comment 11 Gyuyoung Kim 2014-05-13 23:10:33 PDT
Comment on attachment 231277 [details]
Patch

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

>>>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:919
>>>>>> +EAPI void ewk_view_layout_fixed_size_get(const Evas_Object *o, unsigned *width, unsigned *height);
>>>>> 
>>>>> How about Evas_Coord?
>>>>> 
>>>>> Below is example.
>>>>> EAPI void evas_object_resize(Eo *obj, Evas_Coord w, Evas_Coord h);
>>>> 
>>>> As far as I know, Evas_Coord is integer type, isn't it ? In this API, I hope to use *unsigned* because this API needs to handle positive number.
>>> 
>>> Similarly, EFL didn't use unsigned for the size values before.
>> 
>> If so, I want to change it as well.
> 
> nit: +1 for Evas_Coord

I don't see we should use Evas_Coord for positive number though, I follow to use it according to you guys votes. :)
Comment 12 WebKit Commit Bot 2014-05-13 23:21:01 PDT
Comment on attachment 231436 [details]
Patch for landing

Clearing flags on attachment: 231436

Committed r168787: <http://trac.webkit.org/changeset/168787>
Comment 13 WebKit Commit Bot 2014-05-13 23:21:08 PDT
All reviewed patches have been landed.  Closing bug.