Issue 4769 - Supporting adding whitelisted websites on ABP for Samsung Internet

Issue 4769 - Supporting adding whitelisted websites on ABP for Samsung Internet

[anton, 2017-03-06 09:36:33]

https://codereview.adblockplus.org/29376835/diff/29376836/res/layout/empty_vi...
File res/layout/empty_view.xml (right):

https://codereview.adblockplus.org/29376835/diff/29376836/res/layout/empty_vi...
res/layout/empty_view.xml:5: android:orientation="vertical"/>
is "orientation" property required for View?

https://codereview.adblockplus.org/29376835/diff/29376836/res/values-fr/strin...
File res/values-fr/strings.xml (right):

https://codereview.adblockplus.org/29376835/diff/29376836/res/values-fr/strin...
res/values-fr/strings.xml:40: <string
name="whitelist_website_invalid_url_error">URL invalide</string>
please check if it should be masculine or feminine (see russian file comment)

https://codereview.adblockplus.org/29376835/diff/29376836/res/values-ru/strin...
File res/values-ru/strings.xml (right):

https://codereview.adblockplus.org/29376835/diff/29376836/res/values-ru/strin...
res/values-ru/strings.xml:40: <string
name="whitelist_website_invalid_url_error">Неверная URL</string>
Feminine form is used instead of masculine. It sounds weird and in Wikipedia
(https://ru.wikipedia.org/wiki/URL) masculine form is used too.
Please double check it from the translators team.

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
File
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:41:

is empty line required?

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:45:
public WhitelistedWebsitesPreferenceCategory(Context context, AttributeSet
attrs)
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:75:
PreferenceManager.getDefaultSharedPreferences(this.getContext().getApplicationContext());
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:76:
final String key =
this.getContext().getString(R.string.key_whitelisted_websites);
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:78:
this.whitelistedWebsites.addAll(prefs.getStringSet(key,
Collections.<String>emptySet()));
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:88:
new WhitelistedWebsitePreference(this.getContext(), url, new
DialogInterface.OnClickListener()
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:99:
final UrlInputOpenerPreference urlPreference = new
UrlInputOpenerPreference(this.getContext());
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:104:
urlPreference.setOnUrlReadyListener(new
UrlInputOpenerPreference.OnUrlReadyListener()
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:119:
PreferenceManager.getDefaultSharedPreferences(this.getContext().getApplicationContext());
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:120:
final String key =
this.getContext().getString(R.string.key_whitelisted_websites);
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:130:
PreferenceManager.getDefaultSharedPreferences(this.getContext().getApplicationContext());
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:131:
final String key =
this.getContext().getString(R.string.key_whitelisted_websites);
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:145:
final String message =
context.getString(R.string.whitelist_remove_dialog_message, url);
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:157:
final View deleteButton =
view.findViewById(R.id.whitelisted_website_delete_button);
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:181:
builder.setPositiveButton(android.R.string.yes, new
DialogInterface.OnClickListener()
please make sure it suits into line length (see our coding style), afaik
limitation is 80

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:188:
WhitelistedWebsitePreference.this.onDeleteClickListener.onClick(dialog, which);
please make sure it suits into line length (see our coding style), afaik
limitation is 80

[Felix Dahlke, 2017-03-07 15:43:39]

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
File
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:45:
public WhitelistedWebsitesPreferenceCategory(Context context, AttributeSet
attrs)
On 2017/03/06 09:36:32, anton wrote:
> please make sure it suits into line length (see our coding style), afaik
> limitation is 80

For Java code it's 100, see:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

[anton, 2017-03-09 09:57:40]

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
File
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:45:
public WhitelistedWebsitesPreferenceCategory(Context context, AttributeSet
attrs)
On 2017/03/07 15:43:38, Felix Dahlke wrote:
> On 2017/03/06 09:36:32, anton wrote:
> > please make sure it suits into line length (see our coding style), afaik
> > limitation is 80
> 
> For Java code it's 100, see:
>
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

no problem

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:141:
WhitelistedWebsitePreference(Context context, String url,
DialogInterface.OnClickListener onDeleteClickListener)
exceeds 100 characters line limit (not too significant here though)

[diegocarloslima, 2017-03-09 20:15:04]

https://codereview.adblockplus.org/29376835/diff/29376836/res/layout/empty_vi...
File res/layout/empty_view.xml (right):

https://codereview.adblockplus.org/29376835/diff/29376836/res/layout/empty_vi...
res/layout/empty_view.xml:5: android:orientation="vertical"/>
On 2017/03/06 09:36:31, anton wrote:
> is "orientation" property required for View?

Nope, will fix that

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
File
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:41:

On 2017/03/06 09:36:32, anton wrote:
> is empty line required?

Not actually. Will fix that

[anton, 2017-03-10 06:58:12]

On 2017/03/09 20:15:04, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29376835/diff/29376836/res/layout/empty_vi...
> File res/layout/empty_view.xml (right):
> 
>
https://codereview.adblockplus.org/29376835/diff/29376836/res/layout/empty_vi...
> res/layout/empty_view.xml:5: android:orientation="vertical"/>
> On 2017/03/06 09:36:31, anton wrote:
> > is "orientation" property required for View?
> 
> Nope, will fix that
> 
>
https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
> File
>
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java
> (right):
> 
>
https://codereview.adblockplus.org/29376835/diff/29376836/src/org/adblockplus...
>
src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:41:
> 
> On 2017/03/06 09:36:32, anton wrote:
> > is empty line required?
> 
> Not actually. Will fix that

LGTM taking into account Russian string confirmed/fixed

[anton, 2017-03-10 06:58:21]

https://codereview.adblockplus.org/29376835/diff/29379624/res/values-ru/strin...
File res/values-ru/strings.xml (right):

https://codereview.adblockplus.org/29376835/diff/29379624/res/values-ru/strin...
res/values-ru/strings.xml:40: <string
name="whitelist_website_invalid_url_error">Неверная URL</string>
Emailed Tamara (CCed Diego) about it

[Felix Dahlke, 2017-03-16 17:20:14]

René - could you review this and move me to CC then? It's not that much, but not
knowing the code very well I need some time to get into it.

[René Jeschke, 2017-04-12 09:51:32]

LGTM