Would be nice to add --window-size command line option to EFL MiniBrowser (same Qt MiniBrowser has).
Created attachment 172075 [details] patch
Comment on attachment 172075 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=172075&action=review > Tools/MiniBrowser/efl/main.c:42 > +static char *window_size_string = NULL; Does this one really need to be global? Can it be defined in elm_main() instead? > Tools/MiniBrowser/efl/main.c:44 > +static int window_width = 800; If you do it this way, then the command line argument affects the size of ALL future windows, not just the main one. I don't think we want this behavior, do we? > Tools/MiniBrowser/efl/main.c:964 > +#define MAX_LENGTH 3 Why #define instead of a simple const int? > Tools/MiniBrowser/efl/main.c:966 > + Eina_Strbuf *buf; Those variables can be defined as they are needed, right? We don't really need to define all of them at the beginning of the function (unless in pedantic mode). > Tools/MiniBrowser/efl/main.c:979 > + for (c = input_string; c!= delimeter; ++c) eina_strbuf_append_n() ? > Tools/MiniBrowser/efl/main.c:993 > + for (c = delimeter + 1; c!= input_string + string_length; ++c) eina_strbuf_append_n() ?
Comment on attachment 172075 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=172075&action=review Nice > Tools/MiniBrowser/efl/main.c:964 > +#define MAX_LENGTH 3 why not just static unsigned ?
> If you do it this way, then the command line argument affects the size of ALL future windows, not just the main one. I don't think we want this behavior, do we? Isn't that OK? Think if I want to emulate the size of the iPhone, then I would like all new windows to start out using the same size
(In reply to comment #4) > > If you do it this way, then the command line argument affects the size of ALL future windows, not just the main one. I don't think we want this behavior, do we? > > Isn't that OK? Think if I want to emulate the size of the iPhone, then I would like all new windows to start out using the same size If that's the use case, then ok. I just wanted to highlight this and make sure it is the expected behavior.
Comment on attachment 172075 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=172075&action=review > Tools/MiniBrowser/efl/main.c:962 > +parse_window_size(const char *input_string, int *width, int *height) Actually, using eina_str_split_full() may help simplify this function: http://docs.enlightenment.org/auto/eina/group__Eina__String__Group.html#gac408efa406ea8e03d0f6b989ef579a29
(In reply to comment #6) > (From update of attachment 172075 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172075&action=review > > > Tools/MiniBrowser/efl/main.c:962 > > +parse_window_size(const char *input_string, int *width, int *height) > > Actually, using eina_str_split_full() may help simplify this function: > http://docs.enlightenment.org/auto/eina/group__Eina__String__Group.html#gac408efa406ea8e03d0f6b989ef579a29 thanks for the hint
Created attachment 172291 [details] patch v2 using eina_str_split_full
Comment on attachment 172291 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=172291&action=review > Tools/MiniBrowser/efl/main.c:966 > + char **arr; Pirate day? :) arrrrr > Tools/MiniBrowser/efl/main.c:973 > + if (elements == 2 && (strlen(arr[0]) <= max_length) && (strlen(arr[1]) <= max_length)) { > + > + parsed_width = atoi(arr[0]); why this newline? > Tools/MiniBrowser/efl/main.c:983 > + free(arr[0]); > + free(arr); What about arr[1]?
Comment on attachment 172291 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=172291&action=review > Tools/MiniBrowser/efl/main.c:963 > + static const unsigned max_length = 3; Maximum resolution of 999x999?
Comment on attachment 172291 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=172291&action=review >> Tools/MiniBrowser/efl/main.c:963 >> + static const unsigned max_length = 3; > > Maximum resolution of 999x999? Well yes, what's your suggestion? ;) >> Tools/MiniBrowser/efl/main.c:966 >> + char **arr; > > Pirate day? :) arrrrr took naming from http://docs.enlightenment.org/auto/eina/eina_str_01_8c-example.html#a1 >> Tools/MiniBrowser/efl/main.c:973 >> + parsed_width = atoi(arr[0]); > > why this newline? it's more readable imho, but I can remove it if you want >> Tools/MiniBrowser/efl/main.c:983 >> + free(arr); > > What about arr[1]? http://docs.enlightenment.org/auto/eina/group__Eina__String__Group.html#ga601dd2f2032b2a09184164db6f36cc87 To free it, free the first element of the array and the array itself
Comment on attachment 172291 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=172291&action=review > Tools/MiniBrowser/efl/main.c:969 > + arr = eina_str_split_full(input_string, "x", 0, &elements); eina_str_split_full(input_string, "x", 2, &elements); ? since we know we want 2 strings max. >> Tools/MiniBrowser/efl/main.c:983 >> + free(arr); > > What about arr[1]? It is probably safer to iterate over the array and free the elements until you find a NULL element.
Sizes bigger than 1024 are pretty common today :)
Created attachment 172293 [details] patch v3 max length=4.
Created attachment 172294 [details] patch v3 max length=4.
(In reply to comment #12) > (From update of attachment 172291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172291&action=review > > > Tools/MiniBrowser/efl/main.c:969 > > + arr = eina_str_split_full(input_string, "x", 0, &elements); > > eina_str_split_full(input_string, "x", 2, &elements); ? > > since we know we want 2 strings max. elements==2 is validation, so would keep '0' > > >> Tools/MiniBrowser/efl/main.c:983 > >> + free(arr); > > > > What about arr[1]? > > It is probably safer to iterate over the array and free the elements until you find a NULL element. As already replied in comment #11 To free it, free the first element of the array and the array itself. This is EFL.
Comment on attachment 172294 [details] patch v3 Clearing flags on attachment: 172294 Committed r133450: <http://trac.webkit.org/changeset/133450>
All reviewed patches have been landed. Closing bug.