Bug 96499 – [EFL] Remove some parameters in browserCreate()

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

[Seokju Kwon]

There are too many parameters in browserCreate(). Lots of it seems like settings.
And I have moved some parameters into settings.

Refer to https://bugs.webkit.org/show_bug.cgi?id=91718#c20

[Seokju Kwon]

Created attachment 163594 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163594 [details]
Patch

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

> Tools/EWebLauncher/main.c:160
> +static ELauncher_Settings *settings;

Should we make this as global variable ? It would be good to deal with a parameter personally.

[Seokju Kwon]

(In reply to comment #2)
> (From update of attachment 163594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163594&action=review
> 
> > Tools/EWebLauncher/main.c:160
> > +static ELauncher_Settings *settings;
> 
> Should we make this as global variable ? It would be good to deal with a parameter personally.

I thought that we can use it easily when creating new windows by event.

[Gyuyoung Kim]

Although EWebLauncher is simple browser, I still think we need to avoid to use global variable. When we need to add more features or functionality, improper global variable may make spaghetti code.

[Ryuan Choi]

(In reply to comment #4)
> Although EWebLauncher is simple browser, I still think we need to avoid to use global variable. When we need to add more features or functionality, improper global variable may make spaghetti code.

I agree that reducing global variables is better.

But in this case, I think that there is not simple way to keep and pass the settings for each view without keeping global setting information.

It's because we get the setting information via command line argument.
After getting settings in main, I think that we should have at least one copy (global variable) to provide them to each browserViews.

[Seokju Kwon]

(In reply to comment #5)
> (In reply to comment #4)
> > Although EWebLauncher is simple browser, I still think we need to avoid to use global variable. When we need to add more features or functionality, improper global variable may make spaghetti code.
> 
> I agree that reducing global variables is better.
> 
> But in this case, I think that there is not simple way to keep and pass the settings for each view without keeping global setting information.
> 
> It's because we get the setting information via command line argument.
> After getting settings in main, I think that we should have at least one copy (global variable) to provide them to each browserViews.

How about adding 'settings' to a struct of ELauncher?

+typedef struct _ELauncher_Settings {
+    const char *userAgent;
+    const char *engine;
+    const char *backingStore;
+    unsigned char isFlattening;
+    unsigned char isFullscreen;
+    const char *databasePath;
+} ELauncher_Settings;
+
 typedef struct _ELauncher {
     Ecore_Evas *ee;
     Evas *evas;
     Evas_Object *bg;
     Evas_Object *browser;
     Url_Bar *url_bar;
-    const char *theme;
-    const char *userAgent;
-    const char *backingStore;
-    unsigned char isFlattening;
+    ELauncher_Settings *settings;
 } ELauncher;

[Gyuyoung Kim]

(In reply to comment #6)
 
> How about adding 'settings' to a struct of ELauncher?
> 
> +typedef struct _ELauncher_Settings {
> +    const char *userAgent;
> +    const char *engine;
> +    const char *backingStore;
> +    unsigned char isFlattening;
> +    unsigned char isFullscreen;
> +    const char *databasePath;
> +} ELauncher_Settings;
> +
>  typedef struct _ELauncher {
>      Ecore_Evas *ee;
>      Evas *evas;
>      Evas_Object *bg;
>      Evas_Object *browser;
>      Url_Bar *url_bar;
> -    const char *theme;
> -    const char *userAgent;
> -    const char *backingStore;
> -    unsigned char isFlattening;
> +    ELauncher_Settings *settings;
>  } ELauncher;

Looks better to me. Ryuan, how do you think ?

[Ryuan Choi]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > How about adding 'settings' to a struct of ELauncher?
> > 
> > +typedef struct _ELauncher_Settings {
> > +    const char *userAgent;
> > +    const char *engine;
> > +    const char *backingStore;
> > +    unsigned char isFlattening;
> > +    unsigned char isFullscreen;
> > +    const char *databasePath;
> > +} ELauncher_Settings;
> > +
> >  typedef struct _ELauncher {
> >      Ecore_Evas *ee;
> >      Evas *evas;
> >      Evas_Object *bg;
> >      Evas_Object *browser;
> >      Url_Bar *url_bar;
> > -    const char *theme;
> > -    const char *userAgent;
> > -    const char *backingStore;
> > -    unsigned char isFlattening;
> > +    ELauncher_Settings *settings;
> >  } ELauncher;
> 
> Looks better to me. Ryuan, how do you think ?

Seokju, can I know how do you pass settings when F9 is pressed ?

[Seokju Kwon]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > 
> > > How about adding 'settings' to a struct of ELauncher?
> > > 
> > > +typedef struct _ELauncher_Settings {
> > > +    const char *userAgent;
> > > +    const char *engine;
> > > +    const char *backingStore;
> > > +    unsigned char isFlattening;
> > > +    unsigned char isFullscreen;
> > > +    const char *databasePath;
> > > +} ELauncher_Settings;
> > > +
> > >  typedef struct _ELauncher {
> > >      Ecore_Evas *ee;
> > >      Evas *evas;
> > >      Evas_Object *bg;
> > >      Evas_Object *browser;
> > >      Url_Bar *url_bar;
> > > -    const char *theme;
> > > -    const char *userAgent;
> > > -    const char *backingStore;
> > > -    unsigned char isFlattening;
> > > +    ELauncher_Settings *settings;
> > >  } ELauncher;
> > 
> > Looks better to me. Ryuan, how do you think ?
> 
> Seokju, can I know how do you pass settings when F9 is pressed ?

evas_object_event_callback_add(app->browser, EVAS_CALLBACK_KEY_DOWN, on_key_down, app)

on_key_down(void *data, Evas *e, Evas_Object *obj, void *event_info)
The first parameter in this definition will have the same value passed to evas_object_event_callback_add(), if ELauncher has 'settings'.

[Seokju Kwon]

Created attachment 163772 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163772 [details]
Patch

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

> Tools/EWebLauncher/main.c:835
> +        ECORE_GETOPT_VALUE_STR(settings->engine),

Why don't you move *settings->* to same place ?

[Seokju Kwon]

(In reply to comment #11)
> (From update of attachment 163772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163772&action=review
> 
> > Tools/EWebLauncher/main.c:835
> > +        ECORE_GETOPT_VALUE_STR(settings->engine),
> 
> Why don't you move *settings->* to same place ?

ecore_getopt_parse(&options, values, argc, argv);

We parse the values from command line argument.
It has an order. And I didn't move it for keeping the order.

[Gyuyoung Kim]

Comment on attachment 163772 [details]
Patch

If there is no comment anymore, I'd like to land this patch.

[Kenneth Rohde Christiansen]

Comment on attachment 163772 [details]
Patch

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

> Tools/EWebLauncher/main.c:143
> +typedef struct _ELauncher_Settings {

UserArguments? then create a parseApplicationArguments(argv, argc, &UserArguments) or similar?

> Tools/EWebLauncher/main.c:145
> +    const char *engine;

engine? Maybe that could use a bit more info

> Tools/EWebLauncher/main.c:146
> +    const char *backingStore;

backingStore

> Tools/EWebLauncher/main.c:147
> +    unsigned char isFlattening;

isFlattening? frameFlatteningEnabled ?

isFlattning just sounds wrong

[Seokju Kwon]

(In reply to comment #14)
> (From update of attachment 163772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163772&action=review
> 
> > Tools/EWebLauncher/main.c:143
> > +typedef struct _ELauncher_Settings {
> 
> UserArguments? then create a parseApplicationArguments(argv, argc, &UserArguments) or similar?
> 
> > Tools/EWebLauncher/main.c:145
> > +    const char *engine;
> 
> engine? Maybe that could use a bit more info
> 
> > Tools/EWebLauncher/main.c:146
> > +    const char *backingStore;
> 
> backingStore
> 
> > Tools/EWebLauncher/main.c:147
> > +    unsigned char isFlattening;
> 
> isFlattening? frameFlatteningEnabled ?
> 
> isFlattning just sounds wrong

I think that you have already reviewed these things in other patches.
Do you want to change these things?

[Kenneth Rohde Christiansen]

> > > Tools/EWebLauncher/main.c:143
> > > +typedef struct _ELauncher_Settings {
> > 
> > UserArguments? then create a parseApplicationArguments(argv, argc, &UserArguments) or similar?

This is the important part, the rest isnt that important.

[Seokju Kwon]

Created attachment 163831 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 163831 [details]
Patch

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

Why

> Tools/EWebLauncher/main.c:870
> +    ELauncher_Settings *settings;
> +    settings = (ELauncher_Settings *)malloc(sizeof(ELauncher_Settings));
> +    if (!settings)
> +        return quit(EINA_FALSE, "ERROR: could not create settings\n");
> +    memset(settings, 0, sizeof(ELauncher_Settings));

Why not just add it on the stack?

Also I prefer the name to show that it is actually parsed arguments (it uses unsigned char etc instead of Eina_Bool)

Can we call it UserArguments or similar?

> Tools/EWebLauncher/main.c:881
> +    settings->backingStore = (char *)backingStores[1];
> +
> +    themePath = findThemePath(settings->theme);

the parseApplicationArguments should be doing this as well. This function should just be applying them

[Seokju Kwon]

Created attachment 163849 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 163849 [details]
Patch

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

> Tools/EWebLauncher/main.c:860
> +    User_Arguments *userArgs;
> +    userArgs = (User_Arguments *)malloc(sizeof(User_Arguments));
> +    if (!userArgs)
> +        return quit(EINA_FALSE, "ERROR: could not create settings\n");
> +    memset(userArgs, 0, sizeof(User_Arguments));

I still dont get why you need to allocate them on the heap. You could just do static User_Arguments userArgs;

[Seokju Kwon]

Created attachment 163852 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 163852 [details]
Patch

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

> Tools/EWebLauncher/main.c:856
> +    static User_Arguments userArgs;

This variable does not need to be static.

[Seokju Kwon]

Created attachment 163873 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 163873 [details]
Patch

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

> Tools/EWebLauncher/main.c:857
> +    User_Arguments userArgs = {0};

Why not simply `User_Arguments userArgs;' and properly initialize everything later? This idiom might not be supported by all compilers.

[Seokju Kwon]

Created attachment 163988 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163988 [details]
Patch

Clearing flags on attachment: 163988

Committed r128535: <http://trac.webkit.org/changeset/128535>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.