Implement the multiple window creation for MiniBrowser and bind the 'F9' key for opening new window.
Created attachment 166195 [details] Patch
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.
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.
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.
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.
Created attachment 166274 [details] Patch
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.
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. :)
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.
Created attachment 166463 [details] Patch
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'.
Created attachment 166617 [details] Patch
(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!
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.
Created attachment 166659 [details] Patch
(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. :-)
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?
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 ?
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.
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().
Created attachment 167093 [details] Patch
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.
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.
Created attachment 167132 [details] Patch
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?
Created attachment 167311 [details] Patch
(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.
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 .
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.
Created attachment 167352 [details] Patch
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"
(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. :)
Created attachment 167510 [details] Patch
Comment on attachment 167510 [details] Patch Clearing flags on attachment: 167510 Committed r130620: <http://trac.webkit.org/changeset/130620>
All reviewed patches have been landed. Closing bug.