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
Created attachment 163594 [details] Patch
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.
(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.
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.
(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.
(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;
(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 ?
(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 ?
(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'.
Created attachment 163772 [details] Patch
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 ?
(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.
Comment on attachment 163772 [details] Patch If there is no comment anymore, I'd like to land this patch.
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
(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?
> > > 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.
Created attachment 163831 [details] Patch
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
Created attachment 163849 [details] Patch
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;
Created attachment 163852 [details] Patch
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.
Created attachment 163873 [details] Patch
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.
Created attachment 163988 [details] Patch
Comment on attachment 163988 [details] Patch Clearing flags on attachment: 163988 Committed r128535: <http://trac.webkit.org/changeset/128535>
All reviewed patches have been landed. Closing bug.