Bug 99114 – [EFL][WK2] Add Ewk_Window_Features API and related UI callbacks

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

[Jinwoo Song]

UI client callbacks related to window features such as toobars, menuBar, statusBar, resize should be added
so that it is possible to control the window features in the browser applications.

[Jinwoo Song]

Created attachment 172512 [details]
initial work.

This is a working draft, so please do not review this.

[Kangil Han]

Comment on attachment 172512 [details]
initial work.

You can set r- by yourself. :)

[Jinwoo Song]

Created attachment 173881 [details]
working patch.

[Jinwoo Song]

Created attachment 174124 [details]
patch

[Jinwoo Song]

Created attachment 174327 [details]
Patch

[Jinwoo Song]

Created attachment 174328 [details]
Patch

[Ryuan Choi]

Comment on attachment 174328 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:196
> +DECLARE_EWK_VIEW_CALLBACK(ScrollbarsVisible, "scorllbars,visible", bool);
> +DECLARE_EWK_VIEW_CALLBACK(StatusbarVisible, "statusbar,visible", bool);

Should we send this signal to applications?

And don't you need to remove CloseWindow signal?

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:41
> +        if (booleanValue)
> +            m_toolbarVisible = booleanValue->value();

How about adding simple inline method to reduce duplication.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:83
> +        WebCore::WindowFeatures* coreFeatures = new WebCore::WindowFeatures();

Why we should create WindowFeatures?

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:79
> +    bool toolbarVisible = viewImpl->windowFeatures()->toolbarVisible();
> +
> +    return toolbarVisible;

Just return looks enough

[Gyuyoung Kim]

Comment on attachment 174328 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:196
>> +DECLARE_EWK_VIEW_CALLBACK(StatusbarVisible, "statusbar,visible", bool);
> 
> Should we send this signal to applications?
> 
> And don't you need to remove CloseWindow signal?

Missing signal description in ewk_view.h

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:83
> +EAPI void ewk_window_features_int_property_get(const Ewk_Window_Features *window_features, int *x, int *y, int *w, int *h);

I think *_int_* is meaningless.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:74
> +    explicit EwkWindowFeatures(ImmutableDictionary* windowFeatures, EwkViewImpl* viewImpl);

You don't need to use *explicit* when there are two parameters.

[Chris Dumez]

Comment on attachment 174328 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:64
> +EAPI void ewk_window_features_bool_property_get(const Ewk_Window_Features *window_features, Eina_Bool *toolbar_visible, Eina_Bool *statusbar_visible, Eina_Bool *scrollbars_visible, Eina_Bool *menubar_visible, Eina_Bool *locationbar_visible, Eina_Bool *resizable, Eina_Bool *fullscreen);

Can't we have separate functions to get each property? All in one like this is just awful.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:83
>> +EAPI void ewk_window_features_int_property_get(const Ewk_Window_Features *window_features, int *x, int *y, int *w, int *h);
> 
> I think *_int_* is meaningless.

ewk_window_features_geometry_get() ?

[Chris Dumez]

Comment on attachment 174328 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:195
> +DECLARE_EWK_VIEW_CALLBACK(ScrollbarsVisible, "scorllbars,visible", bool);

typo in a signal..

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:841
> +        return static_cast<WKPageRef>(WKRetain(viewImpl->page()));

We used to return 0 in this case, why the change?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:107
> +    EwkWindowFeatures* windowFeatures() { return m_windowFeatures.get(); }

This returns NULL if the view was not created via window.create(), right?

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:38
> +    if (windowFeatures) {

You never seem to create EwkWindowFeatures with windowFeatures == NULL. Why is this needed?

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:86
> +    viewImpl->windowFeatures()->setToolbarVisible(toolbarVisible);

What if windowFeatures() returns NULL? It seems perfectly possible with the current code. For e.g. I have a feeling the main view has windowFeature set to NULL.

[Jinwoo Song]

Comment on attachment 174328 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:196
>>> +DECLARE_EWK_VIEW_CALLBACK(StatusbarVisible, "statusbar,visible", bool);
>> 
>> Should we send this signal to applications?
>> 
>> And don't you need to remove CloseWindow signal?
> 
> Missing signal description in ewk_view.h

Ryuan, as window features are sent to application by smart callback function now, those callbacks are duplicated. I'll remove them and upload a new patch.
I'll also remove 'CloseWindow' and 'CreateWindow' callbacks.
Gyuyoung, I'll do not add signals.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:41
>> +            m_toolbarVisible = booleanValue->value();
> 
> How about adding simple inline method to reduce duplication.

Good suggestion, I'll do it.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:83
>> +        WebCore::WindowFeatures* coreFeatures = new WebCore::WindowFeatures();
> 
> Why we should create WindowFeatures?

It's for initializing member variables when 'windowFeature' is NULL.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:83
>> +EAPI void ewk_window_features_int_property_get(const Ewk_Window_Features *window_features, int *x, int *y, int *w, int *h);
> 
> I think *_int_* is meaningless.

This API returns the *int* values such as window position and size and the other API returns the *bool* values.
That's the reason why *_int_* is in the API name.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:74
>> +    explicit EwkWindowFeatures(ImmutableDictionary* windowFeatures, EwkViewImpl* viewImpl);
> 
> You don't need to use *explicit* when there are two parameters.

You're right. I'll remove it.

>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:79
>> +    return toolbarVisible;
> 
> Just return looks enough

I'll fix it.

[Jinwoo Song]

Comment on attachment 174328 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:195
>> +DECLARE_EWK_VIEW_CALLBACK(ScrollbarsVisible, "scorllbars,visible", bool);
> 
> typo in a signal..

Ops, my mistake. As I commented, I'll remove the callbacks.

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:841
>> +        return static_cast<WKPageRef>(WKRetain(viewImpl->page()));
> 
> We used to return 0 in this case, why the change?

If we return 0, web page isn't loaded and nothing happens as you know. 
With this change, new page is loaded in the same window when the window_create is not implemented by app.
I think it's more kind policy to the webkit/efl clients. :)

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:107
>> +    EwkWindowFeatures* windowFeatures() { return m_windowFeatures.get(); }
> 
> This returns NULL if the view was not created via window.create(), right?

Yes, it is not initialized in EwkViewImpl(). I'll add the initialization code.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:38
>> +    if (windowFeatures) {
> 
> You never seem to create EwkWindowFeatures with windowFeatures == NULL. Why is this needed?

I'll add initialization code with NULL to EwkViewImpl() constructor.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:64
>> +EAPI void ewk_window_features_bool_property_get(const Ewk_Window_Features *window_features, Eina_Bool *toolbar_visible, Eina_Bool *statusbar_visible, Eina_Bool *scrollbars_visible, Eina_Bool *menubar_visible, Eina_Bool *locationbar_visible, Eina_Bool *resizable, Eina_Bool *fullscreen);
> 
> Can't we have separate functions to get each property? All in one like this is just awful.

I agree and will separate functions.

>>>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:83
>>>> +EAPI void ewk_window_features_int_property_get(const Ewk_Window_Features *window_features, int *x, int *y, int *w, int *h);
>>> 
>>> I think *_int_* is meaningless.
>> 
>> ewk_window_features_geometry_get() ?
> 
> This API returns the *int* values such as window position and size and the other API returns the *bool* values.
> That's the reason why *_int_* is in the API name.

Thanks for good naming, Chris.

>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:86
>> +    viewImpl->windowFeatures()->setToolbarVisible(toolbarVisible);
> 
> What if windowFeatures() returns NULL? It seems perfectly possible with the current code. For e.g. I have a feeling the main view has windowFeature set to NULL.

setToolbarsAreVisible is called in createWindow() of FrameLoader.cpp . So I think windowFeatures() does not return NULL in the current code.
But I'll change the patch to initialize m_windowFeatures and also add NULL check code.

[Jinwoo Song]

Created attachment 174404 [details]
Patch

[EFL EWS Bot]

Comment on attachment 174404 [details]
Patch

Attachment 174404 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14832809

[Chris Dumez]

Comment on attachment 174404 [details]
Patch

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

> Source/WebKit2/ChangeLog:29
> +        * UIProcess/API/efl/ewk_window_features.h: Added.

You apparently failed to add this new public header to EWebKit2.h

> Source/WebKit2/PlatformEfl.cmake:76
> +    UIProcess/API/efl/ewk_window_features.cpp

The corresponding public header should be installed (below in the same file)

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:842
> +        return static_cast<WKPageRef>(WKRetain(viewImpl->page()));

I don't think this is right. We should probably return 0 here.
If a web page tries to open a popup window, as a user I would prefer if the popup was not opened instead of having the popup content replace my current page. Wouldn't you?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:853
> +    newViewImpl->m_windowFeatures = ewkWindowFeatures;

The EwkWindowFeatures object will be created twice in this case. Once in this function, and a second time in the EwkViewImpl constructor (with empty features).
I think m_windowFeatures should be lazily created instead of initializing it in EwkViewImpl constructor to avoid this issue.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:35
> +bool getWindowFeatureValue(ImmutableDictionary* windowFeatures, const char* featureName)

This should be static and added to the class as private.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:42
> +    return 0;

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:45
> +double getWindowFeatureGeometry(ImmutableDictionary* windowFeatures, const char* featureName)

This should be static and added to the class as private.
It feels like getWindowFeatureValue() and getWindowFeatureGeometry() would be replaced by one templated function?

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:58
> +    if (windowFeatures) {

We should probably add an ASSERT(viewImpl); at the beginning of the constructor.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:72
> +        WebCore::WindowFeatures* coreFeatures = new WebCore::WindowFeatures();

This seems to be leaking, please use smart pointer to avoid this.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:89
> +Eina_Bool ewk_window_features_toolbar_visible_get(const Ewk_Window_Features *window_features)

star on wrong side

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:91
> +    EWK_OBJ_GET_IMPL_OR_RETURN(const EwkWindowFeatures, window_features, impl, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:96
> +Eina_Bool ewk_window_features_statusbar_visible_get(const Ewk_Window_Features *window_features)

star on wrong side

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:98
> +    EWK_OBJ_GET_IMPL_OR_RETURN(const EwkWindowFeatures, window_features, impl, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:103
> +Eina_Bool ewk_window_features_scrollbars_visible_get(const Ewk_Window_Features *window_features)

star on wrong side

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:105
> +    EWK_OBJ_GET_IMPL_OR_RETURN(const EwkWindowFeatures, window_features, impl, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:110
> +Eina_Bool ewk_window_features_menubar_visible_get(const Ewk_Window_Features *window_features)

star on wrong side

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:119
> +    EWK_OBJ_GET_IMPL_OR_RETURN(const EwkWindowFeatures, window_features, impl, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:124
> +Eina_Bool ewk_window_features_resizable_get(const Ewk_Window_Features *window_features)

star on wrong side

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:126
> +    EWK_OBJ_GET_IMPL_OR_RETURN(const EwkWindowFeatures, window_features, impl, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:131
> +Eina_Bool ewk_window_features_fullscreen_get(const Ewk_Window_Features *window_features)

star on wrong side

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:133
> +    EWK_OBJ_GET_IMPL_OR_RETURN(const EwkWindowFeatures, window_features, impl, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:52
> + * @return The property value which indicates if toolbar is visible

@c EINA_TRUE is the toolbar should be visible, @c EINA_FALSE otherwise.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:61
> + * @return The property value which indicates if statusbar is visible

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:70
> + * @return The property value which indicates if scrollbar are visible

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:79
> + * @return The property value which indicates if menubar is visible

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:88
> + * @return The property value which indicates if locationbar is visible

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:97
> + * @return The property value which indicates if the window is resizable

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:106
> + * @return The property value which indicates if the window is fullscreen

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:122
> +EAPI void ewk_window_features_geometry_get(const Ewk_Window_Features *window_features, int *x, int *y, int *w, int *h);

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:36
> +using namespace WebKit;

We avoid this in headers (even private ones).

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:37
> +using namespace WebCore;

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:61
> +    bool menubarVisible() const { return m_menubarVisible; }

menuBar? Feels like the camel case is wrong here.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:64
> +    bool locationbarVisible() const { return m_locationbarVisible; }

locationBar? Feels like the camel case is wrong here.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:70
> +    bool fullscreen() const { return m_fullscreen; }

fullScreen? Feels like the camel case is wrong here.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_window_features.cpp:37
> +static Evas_Object* createDefaultWindow(Ewk_View_Smart_Data *smartData, const Ewk_Window_Features *windowFeatures)

"stars on wrong side"

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_window_features.cpp:39
> +    // default vales of WebCore:WindowFeatures()

"values"

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_window_features.cpp:72
> +    const char* windowHTML = "<html><body onLoad=\"window.open('', '');\"></body></html>";

const char windowHTML[] may be better here (slight difference).

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_window_features.cpp:105
> +    const char* windowHTML = "<html><body onLoad=\"window.open('', '', 'left=100,top=150,width=400,height=400,location=no,menubar=no,status=yes,toolbar=no,scrollbars=yes,resizable=yes,fullscreen=no');\"></body></html>";

const char windowHTML[] may be better here.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:77
> +    return features ? features->toolbarVisible() : false;

Null check should probably be replaced by an assertion as it should never be NULL with your current implementation.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:83
> +    if (features)

Ditto.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:90
> +    return features ? features->menubarVisible() : false;

Ditto

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:96
> +    if (features)

Ditto.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:97
> +        features->setMenubarVisible(menubarVisible);

What's the point of updating features if the client (browser) has no way to know about it? (you removed the signals and windowFeatures is not exposed to the client).

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:103
> +    return features ? features->statusbarVisible() : false;

Ditto.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:109
> +    if (features)

Ditto.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:116
> +    return features ? features->resizable() : false;

Ditto.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:122
> +    if (features)

Ditto.

[Jinwoo Song]

Comment on attachment 174404 [details]
Patch

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

Fixed the issues commented by Chris.

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:842
>> +        return static_cast<WKPageRef>(WKRetain(viewImpl->page()));
> 
> I don't think this is right. We should probably return 0 here.
> If a web page tries to open a popup window, as a user I would prefer if the popup was not opened instead of having the popup content replace my current page. Wouldn't you?

From the users side, they may want to exact behavior. Okay, I'll return 0 here.

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:853
>> +    newViewImpl->m_windowFeatures = ewkWindowFeatures;
> 
> The EwkWindowFeatures object will be created twice in this case. Once in this function, and a second time in the EwkViewImpl constructor (with empty features).
> I think m_windowFeatures should be lazily created instead of initializing it in EwkViewImpl constructor to avoid this issue.

Okay, I'll apply your advice.

>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:97
>> +        features->setMenubarVisible(menubarVisible);
> 
> What's the point of updating features if the client (browser) has no way to know about it? (you removed the signals and windowFeatures is not exposed to the client).

Window.menubar.visible gets the property value from PageUIClientEfl::menuBarIsVisible(), so we update the value here.
If there is a use case the client application should know the value changes, I'll add a signal in another patch.

[Jinwoo Song]

Created attachment 174611 [details]
Patch

[Chris Dumez]

Comment on attachment 174611 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        so that it is possible to retreive the window object property values.

"retrieve"

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:107
> +    EwkWindowFeatures* windowFeatures() { return m_windowFeatures.get(); }

You missing lazy initialization here? It will return NULL for windows not created by JS.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:56
> +T1 EwkWindowFeatures::getWindowFeatureValue(ImmutableDictionary* windowFeatures, const char* featureName)

Seems to me featureName could be a String here since this is a C++ method

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:60
> +    if (featureValue)

nit: we usually do the opposite:
if (!featureValue)
  return false;

return featureValue->value();

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:85
> +    features->setToolbarVisible(toolbarVisible);

We should really notify the app in these setters, why did you remove the signals?
The app has currently no way of knowing that it changed and that it should update its UI.

[Jinwoo Song]

Comment on attachment 174611 [details]
Patch

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

>> Source/WebKit2/ChangeLog:13
>> +        so that it is possible to retreive the window object property values.
> 
> "retrieve"

fixed.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:851
> +    newViewImpl->m_windowFeatures = ewkWindowFeatures;

I tried to find a way to lazy initialization, but in the case the window is created by JS, m_windowFeatures should be initialized with the parameters.
So I left it here. Chris, what do you think about this?

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:107
>> +    EwkWindowFeatures* windowFeatures() { return m_windowFeatures.get(); }
> 
> You missing lazy initialization here? It will return NULL for windows not created by JS.

I added m_windowFeatures initialization code here for the cases when the window is not created by JS.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:56
>> +T1 EwkWindowFeatures::getWindowFeatureValue(ImmutableDictionary* windowFeatures, const char* featureName)
> 
> Seems to me featureName could be a String here since this is a C++ method

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:60
>> +    if (featureValue)
> 
> nit: we usually do the opposite:
> if (!featureValue)
>   return false;
> 
> return featureValue->value();

fixed.

>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:85
>> +    features->setToolbarVisible(toolbarVisible);
> 
> We should really notify the app in these setters, why did you remove the signals?
> The app has currently no way of knowing that it changed and that it should update its UI.

I added signal again.

[Jinwoo Song]

Created attachment 174663 [details]
Patch

[Chris Dumez]

Comment on attachment 174663 [details]
Patch

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

Ah, better :)

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:51
> +        WebCore::WindowFeatures* coreFeatures = new WebCore::WindowFeatures();

No raw pointers please.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:66
> +        delete coreFeatures;

Those 2 lines will be useless if you use a smart pointer (and you should)

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:72
> +T1 EwkWindowFeatures::getWindowFeatureValue(ImmutableDictionary* windowFeatures, const String featureName)

Missing reference: const String&

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:131
> +void ewk_window_features_geometry_get(const Ewk_Window_Features* window_features, int* x, int* y, int* width, int* height)

Should we use Evas_Coord instead of int since this is public EFL/C API? It seems EFL is using Evas_Coord:
http://docs.enlightenment.org/auto/evas/group__Evas__Object__Group__Basic.html#ga6f52fab1a96dc58c85ee481d84bac871

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:88
> +    viewImpl->smartCallback<ToolbarVisible>().call(&toolbarVisible);

The callbacks could be called by the EwkWindowFeatures setters. This would be less error prone (if someone calls a EwkWindowFeatures setters but forgets the emit callback on the view).

[Jinwoo Song]

Comment on attachment 174663 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:51
>> +        WebCore::WindowFeatures* coreFeatures = new WebCore::WindowFeatures();
> 
> No raw pointers please.

Opps, I missed it. I fixed to use smart pointer.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:66
>> +        delete coreFeatures;
> 
> Those 2 lines will be useless if you use a smart pointer (and you should)

Fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:72
>> +T1 EwkWindowFeatures::getWindowFeatureValue(ImmutableDictionary* windowFeatures, const String featureName)
> 
> Missing reference: const String&

Fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:131
>> +void ewk_window_features_geometry_get(const Ewk_Window_Features* window_features, int* x, int* y, int* width, int* height)
> 
> Should we use Evas_Coord instead of int since this is public EFL/C API? It seems EFL is using Evas_Coord:
> http://docs.enlightenment.org/auto/evas/group__Evas__Object__Group__Basic.html#ga6f52fab1a96dc58c85ee481d84bac871

Fixed.

>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:88
>> +    viewImpl->smartCallback<ToolbarVisible>().call(&toolbarVisible);
> 
> The callbacks could be called by the EwkWindowFeatures setters. This would be less error prone (if someone calls a EwkWindowFeatures setters but forgets the emit callback on the view).

Changed callbacks to be called in the setters.

[Jinwoo Song]

Created attachment 174880 [details]
Patch

[Chris Dumez]

Comment on attachment 174880 [details]
Patch

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

LGTM with minor comment:

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:83
> +    EwkViewImpl* viewImpl = toPageUIClientEfl(clientInfo)->m_viewImpl;

nit: Be consistent, sometimes you have 1 variable (features), sometimes you have 2 (viewImpl and features). IMO, you need only 1 (features) so let's get rid of the viewImpl temporary variable and make it consistent.

[Jinwoo Song]

Created attachment 174908 [details]
Patch

[Jinwoo Song]

(In reply to comment #24)
> (From update of attachment 174880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174880&action=review
> 
> LGTM with minor comment:
> 
> > Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:83
> > +    EwkViewImpl* viewImpl = toPageUIClientEfl(clientInfo)->m_viewImpl;
> 
> nit: Be consistent, sometimes you have 1 variable (features), sometimes you have 2 (viewImpl and features). IMO, you need only 1 (features) so let's get rid of the viewImpl temporary variable and make it consistent.

Fixed.

[Gyuyoung Kim]

Comment on attachment 174908 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:46
> +typedef struct Ewk_Object Ewk_Window_Features;

Ewk_Object -> EwkObject

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:53
> + * @return @c EINA_TRUE is the toolbar should be visible, @c EINA_FALSE otherwise.

Do not use . at the end of line.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:62
> + * @return @c EINA_TRUE is the statusbar should be visible, @c EINA_FALSE otherwise.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:71
> + * @return @c EINA_TRUE is the scrollbars should be visible, @c EINA_FALSE otherwise.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:80
> + * @return @c EINA_TRUE is the menubar should be visible, @c EINA_FALSE otherwise.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:89
> + * @return @c EINA_TRUE is the locationbar should be visible, @c EINA_FALSE otherwise.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.h:107
> + * @return @c EINA_TRUE is the window should be fullscreen, @c EINA_FALSE otherwise.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_window_features_private.h:25
> +

Unneeded line.

> Tools/MiniBrowser/efl/main.c:1054
> +

Looks unneeded line.

[Jinwoo Song]

Created attachment 174931 [details]
Patch

[Jinwoo Song]

Comment on attachment 174908 [details]
Patch

all fixed.

[Kenneth Rohde Christiansen]

Comment on attachment 174931 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:55
> +        m_geometry.setHeight(getWindowFeatureValue<double, WebDouble>(windowFeatures, ASCIILiteral("height")));
> +    } else {
> +        OwnPtr<WebCore::WindowFeatures> coreFeatures = adoptPtr(new WebCore::WindowFeatures());
> +
> +        m_toolbarVisible = coreFeatures->toolBarVisible;
> +        m_statusBarVisible = coreFeatures->statusBarVisible;

This is a bit ugly

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:128
> +void PageUIClientEfl::setIsResizable(WKPageRef, bool resizable, const void* clientInfo)
> +{
> +    EwkWindowFeatures* features = toPageUIClientEfl(clientInfo)->m_viewImpl->windowFeatures();
> +    ASSERT(features);
> +    features->setResizable(resizable);
> +}

why are these overwritable at all?

[EFL EWS Bot]

Comment on attachment 174931 [details]
Patch

Attachment 174931 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14908137

[Jinwoo Song]

Created attachment 174937 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 174937 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_window_features.cpp:32
> +#include <gtest/gtest.h>

Why do you include <gtest/gtest.h> again ? EWK2UnitTestBase.h already included it.

[Jinwoo Song]

Comment on attachment 174931 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:55
>> +        m_statusBarVisible = coreFeatures->statusBarVisible;
> 
> This is a bit ugly

I thought the EwkWindowFeatures should be initialized with the default values of WebCore::WindowFeatures when the webview is not created by JS.
In this case, as the windowFeatures parameter is not passed, we needs some default values to be returned when window.statusbar.visible is called.
Could you please let me know the better way or approach?

>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:128
>> +}
> 
> why are these overwritable at all?

Are you concerning about the 'resizable' variable or all of the variables?
EwkWindowFeatures store the parameters passed by window.open() and the values are returned when the property values of window object, such as window.statusbar.visible, are accessed.

[Jinwoo Song]

Comment on attachment 174937 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_window_features.cpp:32
>> +#include <gtest/gtest.h>
> 
> Why do you include <gtest/gtest.h> again ? EWK2UnitTestBase.h already included it.

I'll remove it.

[Kenneth Rohde Christiansen]

(In reply to comment #34)
> (From update of attachment 174931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174931&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:55
> >> +        m_statusBarVisible = coreFeatures->statusBarVisible;
> > 
> > This is a bit ugly
> 
> I thought the EwkWindowFeatures should be initialized with the default values of WebCore::WindowFeatures when the webview is not created by JS.
> In this case, as the windowFeatures parameter is not passed, we needs some default values to be returned when window.statusbar.visible is called.
> Could you please let me know the better way or approach?

Just initialize them in the initialization list with the same defaults. These default are not supposed to change anyway.

> 
> >> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:128
> >> +}
> > 
> > why are these overwritable at all?
> 
> Are you concerning about the 'resizable' variable or all of the variables?
> EwkWindowFeatures store the parameters passed by window.open() and the values are returned when the property values of window object, such as window.statusbar.visible, are accessed.

But those are all turned into a WebCore::WindowFeatures and not changed one-per-one.

[Jinwoo Song]

Comment on attachment 174931 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:55
>>>> +        m_statusBarVisible = coreFeatures->statusBarVisible;
>>> 
>>> This is a bit ugly
>> 
>> I thought the EwkWindowFeatures should be initialized with the default values of WebCore::WindowFeatures when the webview is not created by JS.
>> In this case, as the windowFeatures parameter is not passed, we needs some default values to be returned when window.statusbar.visible is called.
>> Could you please let me know the better way or approach?
> 
> Just initialize them in the initialization list with the same defaults. These default are not supposed to change anyway.

Yes, that would be more simpler and clearer. I'll initialize with the default values.

>>>> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:128
>>>> +}
>>> 
>>> why are these overwritable at all?
>> 
>> Are you concerning about the 'resizable' variable or all of the variables?
>> EwkWindowFeatures store the parameters passed by window.open() and the values are returned when the property values of window object, such as window.statusbar.visible, are accessed.
> 
> But those are all turned into a WebCore::WindowFeatures and not changed one-per-one.

These setter methods are called in createWindow() of FrameLoader.cpp with the parameter set by JavaScript as following,
page->chrome()->setStatusbarVisible(features.statusBarVisible);
Is there anything I misunderstood?

[Jinwoo Song]

Created attachment 174975 [details]
Patch

Applied

[Jinwoo Song]

(In reply to comment #38)
> Created an attachment (id=174975) [details]
> Patch
> 
> Applied
Applied comments by Kenneth and Gyuyoung.

[Kenneth Rohde Christiansen]

Comment on attachment 174975 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_window_features.cpp:37
> +    , m_geometry(0, 0, 0, 0)

Normally the minimum size is supposed to be 100x100. WebCore has code for ensuring that

[Jinwoo Song]

Created attachment 174996 [details]
Patch

Changed the default window size to 100x100.

[Gyuyoung Kim]

Comment on attachment 174996 [details]
Patch

Looks good to me. However, Kenneth might to want to take a final look.

[Jinwoo Song]

Kenneth, do you have any other comments?

[WebKit Review Bot]

Comment on attachment 174996 [details]
Patch

Clearing flags on attachment: 174996

Committed r135343: <http://trac.webkit.org/changeset/135343>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.