Summary: | [EFL] Add command line option to MiniBrowser to set cookies policy | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrzej Badowski <a.badowski> | ||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, g.czajkowski, gyuyoung.kim, lucas.de.marchi | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Andrzej Badowski
2013-04-23 03:13:46 PDT
Created attachment 199210 [details]
proposed patch
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. 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. Created attachment 201578 [details]
proposed patch 2
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. (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. Created attachment 202276 [details]
proposed-patch_3
LGTM. 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. (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 #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 #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. ;) (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. (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. Created attachment 202519 [details]
proposed_patch_4
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.
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()) Created attachment 202523 [details]
proposed_patch_5
Comment on attachment 202523 [details]
proposed_patch_5
r?
(In reply to comment #19) > (From update of attachment 202523 [details]) > r? I've set flag already. Comment on attachment 202523 [details]
proposed_patch_5
Looks OK. r=me.
Comment on attachment 202523 [details] proposed_patch_5 Clearing flags on attachment: 202523 Committed r150511: <http://trac.webkit.org/changeset/150511> All reviewed patches have been landed. Closing bug. |