Bug 97884 – [EFL][WK2] Support multiple window creation for MiniBrowser

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

[Jinwoo Song]

Implement the multiple window creation for MiniBrowser and bind the 'F9' key for opening new window.

[Jinwoo Song]

Created attachment 166195 [details]
Patch

[Byungwoo Lee]

Comment on attachment 166195 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:260
> +static int browserCreate(const char *url, const char *engine)

Why is the return type changed? 
There is no change about return value in the body of this function.

[Jinwoo Song]

Comment on attachment 166195 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:260
>> +static int browserCreate(const char *url, const char *engine)
> 
> Why is the return type changed? 
> There is no change about return value in the body of this function.

As the browserCreate does not need to return anymore, I'll change the type to void.
Thanks for pointing out the mistake.

[Byungwoo Lee]

Comment on attachment 166195 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:50
> MiniBrowser *browser;

The global 'browser' variable can be removed.

> Tools/MiniBrowser/efl/main.c:41
> +static char *engine = NULL;

I cannot found where this variable is used.

> Tools/MiniBrowser/efl/main.c:112
> +static void
> +browserDestroy(Ecore_Evas *ee)
> +{
> +    ecore_evas_free(ee);
> +    if (!eina_list_count(windows))
> +        ecore_main_loop_quit();
> +}

I think that this function can be removed.

It is better to move #110,111 line to the #120 in closeWindow().
There is no relation between ecore_evas_free and checking eina_list_count.

> Tools/MiniBrowser/efl/main.c:114
>  static void closeWindow(Ecore_Evas *ee)

I think that it is more clear to use MiniBrowser* for the parameter.

> Tools/MiniBrowser/efl/main.c:118
> +    MiniBrowser *app;
> +
> +    app = find_app_from_ee(ee);

MiniBrowser *app = find_app_from_ee(ee);

And I think null check will be needed.

> Tools/MiniBrowser/efl/main.c:135
> +    app = find_app_from_ee(ee);
> +    url_bar_width_set(app->url_bar, w);

Null check is needed before use

> Tools/MiniBrowser/efl/main.c:336
> -    Ecore_Event_Handler *handle = ecore_event_handler_add(ECORE_EVENT_SIGNAL_EXIT, main_signal_exit, 0);
> +    Ecore_Event_Handler *handle = ecore_event_handler_add(ECORE_EVENT_SIGNAL_EXIT, main_signal_exit, &windows);

windows is a global variable and the passed pointer is not used in the main_signal_exit.
Passing the pointer is not needed, or main_signal_exit should use the pointer.

[Jinwoo Song]

Comment on attachment 166195 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:41
>> +static char *engine = NULL;
> 
> I cannot found where this variable is used.

It stores the ecore-evas engine type from the command line argument.

>> Tools/MiniBrowser/efl/main.c:112
>> +}
> 
> I think that this function can be removed.
> 
> It is better to move #110,111 line to the #120 in closeWindow().
> There is no relation between ecore_evas_free and checking eina_list_count.

I agree and will remove this function and move the code as your suggestion.

>> Tools/MiniBrowser/efl/main.c:114
>>  static void closeWindow(Ecore_Evas *ee)
> 
> I think that it is more clear to use MiniBrowser* for the parameter.

This is a callback function of ecore_evas_callback_delete_request_set() with Ecore_Evas parameter.

>> Tools/MiniBrowser/efl/main.c:118
>> +    app = find_app_from_ee(ee);
> 
> MiniBrowser *app = find_app_from_ee(ee);
> 
> And I think null check will be needed.

okay, I'll do it.

>> Tools/MiniBrowser/efl/main.c:135
>> +    url_bar_width_set(app->url_bar, w);
> 
> Null check is needed before use

okay.

>>> Tools/MiniBrowser/efl/main.c:260
>>> +static int browserCreate(const char *url, const char *engine)
>> 
>> Why is the return type changed? 
>> There is no change about return value in the body of this function.
> 
> As the browserCreate does not need to return anymore, I'll change the type to void.
> Thanks for pointing out the mistake.

It seems to better to use Eina_Bool return type to check if the creation is succeeded or not.

>> Tools/MiniBrowser/efl/main.c:336
>> +    Ecore_Event_Handler *handle = ecore_event_handler_add(ECORE_EVENT_SIGNAL_EXIT, main_signal_exit, &windows);
> 
> windows is a global variable and the passed pointer is not used in the main_signal_exit.
> Passing the pointer is not needed, or main_signal_exit should use the pointer.

yes indeed.

[Jinwoo Song]

Created attachment 166274 [details]
Patch

[Byungwoo Lee]

Comment on attachment 166274 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:-48
> -MiniBrowser *browser;

This global variable still exists.

> Tools/MiniBrowser/efl/main.c:96
> +        closeWindow(app->ee);

closeWindow() is used here also.

At this context, find_app_from_ee() is not needed because app is already known.

How about separating closeWindow() and deleteRequestCallback()?

> Tools/MiniBrowser/efl/main.c:99
> +    if (!eina_list_count(windows))
> +        ecore_main_loop_quit();

if condition can be removed.
At this line, window will be always NULL.

> Tools/MiniBrowser/efl/main.c:111
> +    if (!eina_list_count(windows)) {

if(!windows) looks enough like #94

> Tools/MiniBrowser/efl/main.c:112
> +        free(app);

Is it mistake? I think this line should be out of the if() condition.

> Tools/MiniBrowser/efl/main.c:350
> +    eina_list_free(windows);

Is it needed?

If so, deleting 'app' also needed.

[Byungwoo Lee]

Comment on attachment 166274 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:-48
>> -MiniBrowser *browser;
> 
> This global variable still exists.

Oops. deleted. :)

[Jinwoo Song]

Comment on attachment 166274 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:96
>> +        closeWindow(app->ee);
> 
> closeWindow() is used here also.
> 
> At this context, find_app_from_ee() is not needed because app is already known.
> 
> How about separating closeWindow() and deleteRequestCallback()?

I understand your point and separated closeWindow() and deleteRequestCallback().

>> Tools/MiniBrowser/efl/main.c:99
>> +        ecore_main_loop_quit();
> 
> if condition can be removed.
> At this line, window will be always NULL.

you are right.

>> Tools/MiniBrowser/efl/main.c:111
>> +    if (!eina_list_count(windows)) {
> 
> if(!windows) looks enough like #94

done.

>> Tools/MiniBrowser/efl/main.c:112
>> +        free(app);
> 
> Is it mistake? I think this line should be out of the if() condition.

done.

>> Tools/MiniBrowser/efl/main.c:350
>> +    eina_list_free(windows);
> 
> Is it needed?
> 
> If so, deleting 'app' also needed.

It is an unneeded line so I removed it.

[Jinwoo Song]

Created attachment 166463 [details]
Patch

[Byungwoo Lee]

Comment on attachment 166463 [details]
Patch

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

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

How about adding NULL check for 'ee'?
if (!ee)
    return NULL;

> Tools/MiniBrowser/efl/main.c:90
> +{

Please add Null check before accessing 'app'

> Tools/MiniBrowser/efl/main.c:110
> +    MiniBrowser *app = browser_find(ee);
> +    if (!app)
> +        return;
> +    browser_close(app);

Can be shortened 'browser_close(browser_find(ee));' if browser_close has null check for 'app'.

[Jinwoo Song]

Created attachment 166617 [details]
Patch

[Jinwoo Song]

(In reply to comment #11)
> (From update of attachment 166463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166463&action=review
> 
> > Tools/MiniBrowser/efl/main.c:79
> > +
> 
> How about adding NULL check for 'ee'?
> if (!ee)
>     return NULL;
> 
> > Tools/MiniBrowser/efl/main.c:90
> > +{
> 
> Please add Null check before accessing 'app'
> 
> > Tools/MiniBrowser/efl/main.c:110
> > +    MiniBrowser *app = browser_find(ee);
> > +    if (!app)
> > +        return;
> > +    browser_close(app);
> 
> Can be shortened 'browser_close(browser_find(ee));' if browser_close has null check for 'app'.

All done, thanks for review!

[Byungwoo Lee]

Comment on attachment 166617 [details]
Patch

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

With some additional empty lines, it looks ok to me.

> Tools/MiniBrowser/efl/main.c:81
> +    if (!ee)
> +        return NULL;

How about adding empty line after this?

> Tools/MiniBrowser/efl/main.c:94
> +    if (!app)
> +        return;

ditto.

> Tools/MiniBrowser/efl/main.c:104
> +    while (windows)
> +        browser_close((MiniBrowser *)eina_list_data_get(windows));

ditto.

> Tools/MiniBrowser/efl/main.c:111
> +    browser_close(browser_find(ee));

ditto.

> Tools/MiniBrowser/efl/main.c:127
> +    app = browser_find(ee);
> +    if (!app)
> +        return;

ditto.

[Jinwoo Song]

Created attachment 166659 [details]
Patch

[Jinwoo Song]

(In reply to comment #14)
> (From update of attachment 166617 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166617&action=review
> 
> With some additional empty lines, it looks ok to me.
> 
> > Tools/MiniBrowser/efl/main.c:81
> > +    if (!ee)
> > +        return NULL;
> 
> How about adding empty line after this?
> 
> > Tools/MiniBrowser/efl/main.c:94
> > +    if (!app)
> > +        return;
> 
> ditto.
> 
> > Tools/MiniBrowser/efl/main.c:104
> > +    while (windows)
> > +        browser_close((MiniBrowser *)eina_list_data_get(windows));
> 
> ditto.
> 
> > Tools/MiniBrowser/efl/main.c:111
> > +    browser_close(browser_find(ee));
> 
> ditto.
> 
> > Tools/MiniBrowser/efl/main.c:127
> > +    app = browser_find(ee);
> > +    if (!app)
> > +        return;
> 
> ditto.

Done. :-)

[Ryuan Choi]

Comment on attachment 166659 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:106
> +    while (windows)
> +        browser_close((MiniBrowser *)eina_list_data_get(windows));

Why don't you use EINA_LIST_FREE?

[Laszlo Gombos]

Comment on attachment 166659 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:41
> +static char *engine = NULL;

I think we should use a more descriptive name, perhaps evas_engine.

> Tools/MiniBrowser/efl/main.c:170
> +        browser_create(DEFAULT_URL, engine);

Should this be window_create instead ?

[Jinwoo Song]

Comment on attachment 166659 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:41
>> +static char *engine = NULL;
> 
> I think we should use a more descriptive name, perhaps evas_engine.

Okay, I'll change to evas_engine.

>> Tools/MiniBrowser/efl/main.c:106
>> +        browser_close((MiniBrowser *)eina_list_data_get(windows));
> 
> Why don't you use EINA_LIST_FREE?

I can use EINA_LIST_FREE as following, but it seems current style looks more simple.
Do we have any more advantage to use EINA_LIST_FREE?

MiniBrowser *app;
EINA_LIST_FREE(windows, app)
{
    url_bar_del(app->url_bar);
    ecore_evas_free(app->ee);
    free(app);
}

>> Tools/MiniBrowser/efl/main.c:170
>> +        browser_create(DEFAULT_URL, engine);
> 
> Should this be window_create instead ?

window_create, window_close are more proper name. I'll modify them.

[Jinwoo Song]

Comment on attachment 166659 [details]
Patch

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

>>> Tools/MiniBrowser/efl/main.c:106
>>> +        browser_close((MiniBrowser *)eina_list_data_get(windows));
>> 
>> Why don't you use EINA_LIST_FREE?
> 
> I can use EINA_LIST_FREE as following, but it seems current style looks more simple.
> Do we have any more advantage to use EINA_LIST_FREE?
> 
> MiniBrowser *app;
> EINA_LIST_FREE(windows, app)
> {
>     url_bar_del(app->url_bar);
>     ecore_evas_free(app->ee);
>     free(app);
> }

As discussed in offline, I use EINA_LIST_FREE and take out eina_list_remove from browser_close().

[Jinwoo Song]

Created attachment 167093 [details]
Patch

[Ryuan Choi]

Comment on attachment 167093 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:92
> +static void window_close(MiniBrowser *app)

How about window_delete?
Functionality looks not close and you looks introduce window_create for creation.

Anyway, I think that Efl commonly use _add / _del.

> Tools/MiniBrowser/efl/main.c:131
> +    app = browser_find(ee);
> +    if (!app)
> +        return;

We'd better to move this before calling 126 line.

> Tools/MiniBrowser/efl/main.c:135
>      webview = evas_object_name_find(ecore_evas_get(ee), "browser");

After this patch, we don't need to find webview from ecore_evas, right?

> Tools/MiniBrowser/efl/main.c:341
> -        browser = browserCreate(url, engine);
> +        if (!window_create(url, evas_engine)) {

For me, returning browser (or window) and adding it to lists looks better.

At least, I think that window_create and window_close should consider windows lists in same view point.

[Jinwoo Song]

Comment on attachment 167093 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:92
>> +static void window_close(MiniBrowser *app)
> 
> How about window_delete?
> Functionality looks not close and you looks introduce window_create for creation.
> 
> Anyway, I think that Efl commonly use _add / _del.

window_delete may describe the things done in the function, but window_close looks good to me as well.

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

Fixed.

>> Tools/MiniBrowser/efl/main.c:135
>>      webview = evas_object_name_find(ecore_evas_get(ee), "browser");
> 
> After this patch, we don't need to find webview from ecore_evas, right?

yes, we can directly use the webview from app like app->browser. I'll fix it.

>> Tools/MiniBrowser/efl/main.c:341
>> +        if (!window_create(url, evas_engine)) {
> 
> For me, returning browser (or window) and adding it to lists looks better.
> 
> At least, I think that window_create and window_close should consider windows lists in same view point.

From the point of consistency, I agree with you.

[Jinwoo Song]

Created attachment 167132 [details]
Patch

[Ryuan Choi]

Comment on attachment 167132 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:341
> +    MiniBrowser *app;

Because this file has c extension, should we declare this in top of function for compatibility?

[Jinwoo Song]

Created attachment 167311 [details]
Patch

[Jinwoo Song]

(In reply to comment #25)
> (From update of attachment 167132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167132&action=review
> 
> > Tools/MiniBrowser/efl/main.c:341
> > +    MiniBrowser *app;
> 
> Because this file has c extension, should we declare this in top of function for compatibility?

Fixed.

[Kenneth Rohde Christiansen]

Comment on attachment 167132 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:59
> -            ('e', "engine", "ecore-evas engine to use."),
> +            ('e', "ecore-evas engine", "ecore-evas engine to use."),

huh? isn't this so you can write -e or --engine? I guess --ecore-evas engine won't work

> Tools/MiniBrowser/efl/main.c:73
> +static MiniBrowser *window_create(const char *url, const char *evas_engine);

evas_engine_name? it is a string

> Tools/MiniBrowser/efl/main.c:126
> +    app = browser_find(ee);

browser_find? we dont have multiple browsers, we have multiple browser windows. find_browser_window?

> Tools/MiniBrowser/efl/main.c:135
> +    evas_object_move(app->browser, 0, URL_BAR_HEIGHT);

browser is what here? the window?

> Tools/MiniBrowser/efl/main.c:350
> +        return quit(EINA_FALSE, "ERROR: could not create browser window\n");

missing .

[Jinwoo Song]

Comment on attachment 167132 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:59
>> +            ('e', "ecore-evas engine", "ecore-evas engine to use."),
> 
> huh? isn't this so you can write -e or --engine? I guess --ecore-evas engine won't work

That's a silly mistake. I'll fix it.

>> Tools/MiniBrowser/efl/main.c:73
>> +static MiniBrowser *window_create(const char *url, const char *evas_engine);
> 
> evas_engine_name? it is a string

okay, I'll change the variable name to 'evas_engine_name'.

>> Tools/MiniBrowser/efl/main.c:126
>> +    app = browser_find(ee);
> 
> browser_find? we dont have multiple browsers, we have multiple browser windows. find_browser_window?

I'll change to browser_window_find().

>> Tools/MiniBrowser/efl/main.c:135
>> +    evas_object_move(app->browser, 0, URL_BAR_HEIGHT);
> 
> browser is what here? the window?

Yes, app->browser seems to be improper name so I'll change to app->webview.

>> Tools/MiniBrowser/efl/main.c:350
>> +        return quit(EINA_FALSE, "ERROR: could not create browser window\n");
> 
> missing .

I'll fix it.

[Jinwoo Song]

Created attachment 167352 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 167352 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:48
>  typedef struct _MiniBrowser {
>      Ecore_Evas *ee;
>      Evas *evas;
> -    Evas_Object *browser;
> +    Evas_Object *webview;
>      Url_Bar *url_bar;
>  } MiniBrowser;

Maybe renaming MiniBrowser to BrowserWindow would make sense

> Tools/MiniBrowser/efl/main.c:86
> +    EINA_LIST_FOREACH(windows, l, data)
> +    {

should { be on the other line? That would be normal webkit style

> Tools/MiniBrowser/efl/main.c:99
> +static void window_close(MiniBrowser *app)
> +{
> +    url_bar_del(app->url_bar);
> +    ecore_evas_free(app->ee);
> +    free(app);
> +}

The name makes it seem like this method closes the window, but the implementation looks to be destructing the window instead. Why not call it window_free() ?

Also it is confusing that the argument is called "app" and not "window"

[Jinwoo Song]

(In reply to comment #31)
> (From update of attachment 167352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167352&action=review
> 
> > Tools/MiniBrowser/efl/main.c:48
> >  typedef struct _MiniBrowser {
> >      Ecore_Evas *ee;
> >      Evas *evas;
> > -    Evas_Object *browser;
> > +    Evas_Object *webview;
> >      Url_Bar *url_bar;
> >  } MiniBrowser;
> 
> Maybe renaming MiniBrowser to BrowserWindow would make sense
> 
Yes, BrowserWindow looks much better than MiniBrowser. I'll change it.

> > Tools/MiniBrowser/efl/main.c:86
> > +    EINA_LIST_FOREACH(windows, l, data)
> > +    {
> 
> should { be on the other line? That would be normal webkit style
> 
I'll fix it.

> > Tools/MiniBrowser/efl/main.c:99
> > +static void window_close(MiniBrowser *app)
> > +{
> > +    url_bar_del(app->url_bar);
> > +    ecore_evas_free(app->ee);
> > +    free(app);
> > +}
> 
> The name makes it seem like this method closes the window, but the implementation looks to be destructing the window instead. Why not call it window_free() ?
> 
> Also it is confusing that the argument is called "app" and not "window"

Okay, I'll follow your advise. 

It seems that this patch is doing refactoring the whole code, but I like to do this 
as I also didn't like some function/variable names. :)

[Jinwoo Song]

Created attachment 167510 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167510 [details]
Patch

Clearing flags on attachment: 167510

Committed r130620: <http://trac.webkit.org/changeset/130620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.