Bug 100942 – [EFL][WK2] Add --window-size command line option to EFL MiniBrowser

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

[Mikhail Pozdnyakov]

Would be nice to add --window-size command line option to EFL MiniBrowser (same Qt MiniBrowser has).

[Mikhail Pozdnyakov]

Created attachment 172075 [details]
patch

[Chris Dumez]

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() ?

[Kenneth Rohde Christiansen]

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 ?

[Kenneth Rohde Christiansen]

> 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

[Chris Dumez]

(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.

[Chris Dumez]

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

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

Created attachment 172291 [details]
patch v2

using eina_str_split_full

[Kenneth Rohde Christiansen]

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]?

[Chris Dumez]

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?

[Mikhail Pozdnyakov]

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

[Chris Dumez]

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.

[Kenneth Rohde Christiansen]

Sizes bigger than 1024 are pretty common today :)

[Mikhail Pozdnyakov]

Created attachment 172293 [details]
patch v3

max length=4.

[Mikhail Pozdnyakov]

Created attachment 172294 [details]
patch v3

max length=4.

[Mikhail Pozdnyakov]

(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.

[WebKit Review Bot]

Comment on attachment 172294 [details]
patch v3

Clearing flags on attachment: 172294

Committed r133450: <http://trac.webkit.org/changeset/133450>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.