Bug 115028 – [EFL] Add command line option to MiniBrowser to set cookies policy

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Andrzej Badowski]

-p option to set cookies policy (0,1,2)

[Andrzej Badowski]

Created attachment 199210 [details]
proposed patch

[Gyuyoung Kim]

Comment on attachment 199210 [details]
proposed patch

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

> Tools/ChangeLog:7
> +

Missing description.

> Tools/MiniBrowser/efl/main.c:53
> +static ushort cookies_policy = EWK_COOKIE_ACCEPT_POLICY_NO_THIRD_PARTY;

Do you have any reason to set EWK_COOKIE_ACCEPT_POLICY_NO_THIRD_PARTY instead of EWK_COOKIE_ACCEPT_POLICY_ALWAYS ?  Isn't EWK_COOKIE_ACCEPT_POLICY_ALWAYS default value ?

> Tools/MiniBrowser/efl/main.c:1669
> +    cookies_policy = cookies_policy % 3;

I think we should remain that user can set the policy.

[Andrzej Badowski]

Comment on attachment 199210 [details]
proposed patch

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

>> Tools/ChangeLog:7
>> +
> 
> Missing description.

I will add it in next patch.

>> Tools/MiniBrowser/efl/main.c:53
>> +static ushort cookies_policy = EWK_COOKIE_ACCEPT_POLICY_NO_THIRD_PARTY;
> 
> Do you have any reason to set EWK_COOKIE_ACCEPT_POLICY_NO_THIRD_PARTY instead of EWK_COOKIE_ACCEPT_POLICY_ALWAYS ?  Isn't EWK_COOKIE_ACCEPT_POLICY_ALWAYS default value ?

Some comments to code say that EWK_COOKIE_ACCEPT_POLICY_NO_THIRD_PARTY is default value.

>> Tools/MiniBrowser/efl/main.c:1669
>> +    cookies_policy = cookies_policy % 3;
> 
> I think we should remain that user can set the policy.

There's an extra protection to set for the policy only an allowed value.

[Andrzej Badowski]

Created attachment 201578 [details]
proposed patch 2

[Grzegorz Czajkowski]

Comment on attachment 201578 [details]
proposed patch 2

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

The code looks fine.
Please don't forget to rebase the patch.

> Tools/ChangeLog:8
> +        add command line option to MiniBrowser EFL to set cookies policy:

Please start with capital letter.

> Tools/ChangeLog:11
> +        but more simple to the user of EFL MiniBrowser       

Please add dot.

[Andrzej Badowski]

(In reply to comment #5)
> (From update of attachment 201578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201578&action=review
> 
> The code looks fine.
> Please don't forget to rebase the patch.
> 
> > Tools/ChangeLog:8
> > +        add command line option to MiniBrowser EFL to set cookies policy:
> 
> Please start with capital letter.
> 
> > Tools/ChangeLog:11
> > +        but more simple to the user of EFL MiniBrowser       
> 
> Please add dot.

I've prepared appropriate amendments in ChangeLog (capital letter, dot). Patch will be soon.

[Andrzej Badowski]

Created attachment 202276 [details]
proposed-patch_3

[Grzegorz Czajkowski]

LGTM.

[Chris Dumez]

Comment on attachment 202276 [details]
proposed-patch_3

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

I think adding such command line parameter is a good idea but I don't like the approach.

> Tools/MiniBrowser/efl/main.c:161
> +            ('p', "policy-cookies", "cookies policy: 0 - always accept, 1 - never accept, 2 - don't accept third-party cookies", 2),

Using integers is really obscure IMHO. Why don't we use user-readable strings instead? It's not like performance really matters here. This is a user-facing setting and it should be user-readable.

[Gyuyoung Kim]

(In reply to comment #3)

> >> Tools/MiniBrowser/efl/main.c:1669
> >> +    cookies_policy = cookies_policy % 3;
> > 
> > I think we should remain that user can set the policy.
> 
> There's an extra protection to set for the policy only an allowed value.

I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #3)
> 
> > >> Tools/MiniBrowser/efl/main.c:1669
> > >> +    cookies_policy = cookies_policy % 3;
> > > 
> > > I think we should remain that user can set the policy.
> > 
> > There's an extra protection to set for the policy only an allowed value.
> 
> I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.

Or simply use a string as I propose.

[Gyuyoung Kim]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #3)
> > 
> > > >> Tools/MiniBrowser/efl/main.c:1669
> > > >> +    cookies_policy = cookies_policy % 3;
> > > > 
> > > > I think we should remain that user can set the policy.
> > > 
> > > There's an extra protection to set for the policy only an allowed value.
> > 
> > I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.
> 
> Or simply use a string as I propose.

Yes, good idea as well. ;)

[Andrzej Badowski]

(In reply to comment #9)
> (From update of attachment 202276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202276&action=review
> 
> I think adding such command line parameter is a good idea but I don't like the approach.
> 
> > Tools/MiniBrowser/efl/main.c:161
> > +            ('p', "policy-cookies", "cookies policy: 0 - always accept, 1 - never accept, 2 - don't accept third-party cookies", 2),
> 
> Using integers is really obscure IMHO. Why don't we use user-readable strings instead? It's not like performance really matters here. This is a user-facing setting and it should be user-readable.

(In reply to comment #10)
> (In reply to comment #3)
> 
> > >> Tools/MiniBrowser/efl/main.c:1669
> > >> +    cookies_policy = cookies_policy % 3;
> > > 
> > > I think we should remain that user can set the policy.
> > 
> > There's an extra protection to set for the policy only an allowed value.
> 
> I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #3)
> > 
> > > >> Tools/MiniBrowser/efl/main.c:1669
> > > >> +    cookies_policy = cookies_policy % 3;
> > > > 
> > > > I think we should remain that user can set the policy.
> > > 
> > > There's an extra protection to set for the policy only an allowed value.
> > 
> > I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.
> 
> Or simply use a string as I propose.

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #3)
> > > 
> > > > >> Tools/MiniBrowser/efl/main.c:1669
> > > > >> +    cookies_policy = cookies_policy % 3;
> > > > > 
> > > > > I think we should remain that user can set the policy.
> > > > 
> > > > There's an extra protection to set for the policy only an allowed value.
> > > 
> > > I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.
> > 
> > Or simply use a string as I propose.
> 
> Yes, good idea as well. ;)

There's good idea to use strings in command-line. At the beginning I thought it would be more complicated having a relatively long texts reflecting the sense of individual cases than put simply one char (0,1,2) , but now I think text options could be short (i.e. never, always, third-party) and will be certainly more intuitive then integers. User always can read detailed explanation by --help option. I will prepare new appropriate patch also taking into account information for user about wrong option.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #9)
> > (From update of attachment 202276 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202276&action=review
> > 
> > I think adding such command line parameter is a good idea but I don't like the approach.
> > 
> > > Tools/MiniBrowser/efl/main.c:161
> > > +            ('p', "policy-cookies", "cookies policy: 0 - always accept, 1 - never accept, 2 - don't accept third-party cookies", 2),
> > 
> > Using integers is really obscure IMHO. Why don't we use user-readable strings instead? It's not like performance really matters here. This is a user-facing setting and it should be user-readable.
> 
> (In reply to comment #10)
> > (In reply to comment #3)
> > 
> > > >> Tools/MiniBrowser/efl/main.c:1669
> > > >> +    cookies_policy = cookies_policy % 3;
> > > > 
> > > > I think we should remain that user can set the policy.
> > > 
> > > There's an extra protection to set for the policy only an allowed value.
> > 
> > I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.
> 
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #3)
> > > 
> > > > >> Tools/MiniBrowser/efl/main.c:1669
> > > > >> +    cookies_policy = cookies_policy % 3;
> > > > > 
> > > > > I think we should remain that user can set the policy.
> > > > 
> > > > There's an extra protection to set for the policy only an allowed value.
> > > 
> > > I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.
> > 
> > Or simply use a string as I propose.
> 
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (In reply to comment #3)
> > > > 
> > > > > >> Tools/MiniBrowser/efl/main.c:1669
> > > > > >> +    cookies_policy = cookies_policy % 3;
> > > > > > 
> > > > > > I think we should remain that user can set the policy.
> > > > > 
> > > > > There's an extra protection to set for the policy only an allowed value.
> > > > 
> > > > I'm still not sure if we need to support even when user uses a value out of the policy range. Is there any reason to support it ? IMHO, it is correct way to print a message - the input value is out of range.
> > > 
> > > Or simply use a string as I propose.
> > 
> > Yes, good idea as well. ;)
> 
> There's good idea to use strings in command-line. At the beginning I thought it would be more complicated having a relatively long texts reflecting the sense of individual cases than put simply one char (0,1,2) , but now I think text options could be short (i.e. never, always, third-party) and will be certainly more intuitive then integers. User always can read detailed explanation by --help option. I will prepare new appropriate patch also taking into account information for user about wrong option.

Great, thanks.

[Andrzej Badowski]

Created attachment 202519 [details]
proposed_patch_4

[WebKit Commit Bot]

Attachment 202519 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/efl/main.c']" exit_code: 1
Tools/MiniBrowser/efl/main.c:1596:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/MiniBrowser/efl/main.c:1598:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/MiniBrowser/efl/main.c:1600:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 202519 [details]
proposed_patch_4

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

Looks much better IMHO.

> Tools/MiniBrowser/efl/main.c:1596
> +    if (strcmp(input_string, "always") == 0) 

We avoid comparison to 0 in WebKit. How about if (!strcmp())?

> Tools/MiniBrowser/efl/main.c:1598
> +    if (strcmp(input_string, "never") == 0)

Ditto.

> Tools/MiniBrowser/efl/main.c:1600
> +    if (strcmp(input_string, "no-third-party") != 0)

if (strcmp())

[Andrzej Badowski]

Created attachment 202523 [details]
proposed_patch_5

[Chris Dumez]

Comment on attachment 202523 [details]
proposed_patch_5

r?

[Andrzej Badowski]

(In reply to comment #19)
> (From update of attachment 202523 [details])
> r?

I've set flag already.

[Chris Dumez]

Comment on attachment 202523 [details]
proposed_patch_5

Looks OK. r=me.

[WebKit Commit Bot]

Comment on attachment 202523 [details]
proposed_patch_5

Clearing flags on attachment: 202523

Committed r150511: <http://trac.webkit.org/changeset/150511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.