Bug 103679 – [EFL][WK2] MiniBrowser should have a legacy mode

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

[Yael]

It is very useful to start MiniBrowser in Legacy mode. e.g. to compare how a test looks in MiniBrowser to the pixel tests results.

[Yael]

Created attachment 176865 [details]
Patch

The new API name was suggested in https://bugs.webkit.org/show_bug.cgi?id=100674#c47

[Kenneth Rohde Christiansen]

Comment on attachment 176865 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:213
> +void EwkContext::setViewBehavior(Ewk_View_Behavior behavior)
> +{
> +    m_behavior = behavior;
> +}
> +
> +Ewk_View_Behavior EwkContext::viewBehavior() const
> +{
> +    return m_behavior;
> +}

Do we really want this to be public api? You could also just use a hidden environment variable.

Every thing we expose has to be supported

[Gyuyoung Kim]

Comment on attachment 176865 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:213
>> +}
> 
> Do we really want this to be public api? You could also just use a hidden environment variable.
> 
> Every thing we expose has to be supported

I agree with Kenneth. We should avoid to add unneeded APIs.

[Yael]

(In reply to comment #2)
> (From update of attachment 176865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176865&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:213
> > +void EwkContext::setViewBehavior(Ewk_View_Behavior behavior)
> > +{
> > +    m_behavior = behavior;
> > +}
> > +
> > +Ewk_View_Behavior EwkContext::viewBehavior() const
> > +{
> > +    return m_behavior;
> > +}
> 
> Do we really want this to be public api? You could also just use a hidden environment variable.
> 
> Every thing we expose has to be supported
From https://bugs.webkit.org/show_bug.cgi?id=100674#c47 I assumed that we do, but this can be done without a new API.

[Yael]

Created attachment 176962 [details]
Patch

Removed the new API.

[Kenneth Rohde Christiansen]

Comment on attachment 176962 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +
> +        Allow calling WKViewCreate without creating a context first. A default context
> +        will be used.
> +

You should explain what that actually means. How is that related to the legacy mode.

> Tools/MiniBrowser/efl/main.c:1063
> -    window->ewk_view = ewk_view_smart_add(evas, smart, ewk_context_default_get());
> +    window->ewk_view = legacy_behavior_enabled ? (Evas_Object*)WKViewCreate(evas, 0, 0) : ewk_view_smart_add(evas, smart, ewk_context_default_get());

Why not make this more clean

if (legacy_behavior_enabled) {
   // Use raw WK2 api to create a view using legacy mode.
   window->ewk_view = (Evas_Object*)WKViewCreate(evas, 0, 0);
} else
   window->ewk_view = ewk_view_smart_add(evas, smart, ewk_context_default_get());

[Yael]

(In reply to comment #6)
> (From update of attachment 176962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176962&action=review
> 
> > Source/WebKit2/ChangeLog:10
> > +
> > +        Allow calling WKViewCreate without creating a context first. A default context
> > +        will be used.
> > +
> 
> You should explain what that actually means. How is that related to the legacy mode.
> 
> > Tools/MiniBrowser/efl/main.c:1063
> > -    window->ewk_view = ewk_view_smart_add(evas, smart, ewk_context_default_get());
> > +    window->ewk_view = legacy_behavior_enabled ? (Evas_Object*)WKViewCreate(evas, 0, 0) : ewk_view_smart_add(evas, smart, ewk_context_default_get());
> 
> Why not make this more clean
> 
> if (legacy_behavior_enabled) {
>    // Use raw WK2 api to create a view using legacy mode.
>    window->ewk_view = (Evas_Object*)WKViewCreate(evas, 0, 0);
> } else
>    window->ewk_view = ewk_view_smart_add(evas, smart, ewk_context_default_get());
Because people usually ask me to condense lines as much as possible :)

[Kenneth Rohde Christiansen]

> > if (legacy_behavior_enabled) {
> >    // Use raw WK2 api to create a view using legacy mode.
> >    window->ewk_view = (Evas_Object*)WKViewCreate(evas, 0, 0);
> > } else
> >    window->ewk_view = ewk_view_smart_add(evas, smart, ewk_context_default_get());
> Because people usually ask me to condense lines as much as possible :)

We are doing something a bit out of the ordinary here, let's make that clear.

[Kenneth Rohde Christiansen]

Comment on attachment 176962 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:1063
>> +    window->ewk_view = legacy_behavior_enabled ? (Evas_Object*)WKViewCreate(evas, 0, 0) : ewk_view_smart_add(evas, smart, ewk_context_default_get());
> 
> Why not make this more clean
> 
> if (legacy_behavior_enabled) {
>    // Use raw WK2 api to create a view using legacy mode.
>    window->ewk_view = (Evas_Object*)WKViewCreate(evas, 0, 0);
> } else
>    window->ewk_view = ewk_view_smart_add(evas, smart, ewk_context_default_get());

As you don't add the smart now, why create it?

[Yael]

Created attachment 176971 [details]
Patch

Address Kenneth's comments

[WebKit Review Bot]

Comment on attachment 176971 [details]
Patch

Clearing flags on attachment: 176971

Committed r136320: <http://trac.webkit.org/changeset/136320>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.