Issue 4813 - UTFDataFormatException while serializing filter

Issue 4813 - UTFDataFormatException while serializing filter

[anton, 2017-01-23 06:49:40]

https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:440: final
byte[] b = s.getBytes(StandardCharsets.UTF_8);
StandardCharsets is available starting Java 7. If it's not a problem and
somewhere in README it's noted that JDK 7 is required then we can use it. For
JDK 6 we can use `Charset.forName("UTF-8")` instead

[diegocarloslima, 2017-01-23 16:50:21]

On 2017/01/23 06:49:40, anton wrote:
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> (right):
> 
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:440:
final
> byte[] b = s.getBytes(StandardCharsets.UTF_8);
> StandardCharsets is available starting Java 7. If it's not a problem and
> somewhere in README it's noted that JDK 7 is required then we can use it. For
> JDK 6 we can use `Charset.forName("UTF-8")` instead

For this specific case, there's no big deal about making it Java 6 compatible.
But in my opinion, for projects that have a minSdkVersion >= 19, there's no
reason why not benefit from the features of Java 7. I have just started an
e-mail discussion about that

[anton, 2017-01-24 05:48:56]

On 2017/01/23 16:50:21, diegocarloslima wrote:
> On 2017/01/23 06:49:40, anton wrote:
> >
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> > File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> > src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:440:
> final
> > byte[] b = s.getBytes(StandardCharsets.UTF_8);
> > StandardCharsets is available starting Java 7. If it's not a problem and
> > somewhere in README it's noted that JDK 7 is required then we can use it.
For
> > JDK 6 we can use `Charset.forName("UTF-8")` instead
> 
> For this specific case, there's no big deal about making it Java 6 compatible.
> But in my opinion, for projects that have a minSdkVersion >= 19, there's no
> reason why not benefit from the features of Java 7. I have just started an
> e-mail discussion about that

I've answered in email but i can repeat that i don't mind using Java 7, but we
should note this requirement in READMe

[diegocarloslima, 2017-01-30 17:14:30]

https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:440: final
byte[] b = s.getBytes(StandardCharsets.UTF_8);
On 2017/01/23 06:49:40, anton wrote:
> StandardCharsets is available starting Java 7. If it's not a problem and
> somewhere in README it's noted that JDK 7 is required then we can use it. For
> JDK 6 we can use `Charset.forName("UTF-8")` instead

For instance, I'll use Charset.forName("UTF-8"). Then, I'll create a follow up
issue to add Java 7 features which I'll change this back to
StandardCharsets.UTF_8

[anton, 2017-02-15 11:01:54]

On 2017/01/30 17:14:30, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> (right):
> 
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:440:
final
> byte[] b = s.getBytes(StandardCharsets.UTF_8);
> On 2017/01/23 06:49:40, anton wrote:
> > StandardCharsets is available starting Java 7. If it's not a problem and
> > somewhere in README it's noted that JDK 7 is required then we can use it.
For
> > JDK 6 we can use `Charset.forName("UTF-8")` instead
> 
> For instance, I'll use Charset.forName("UTF-8"). Then, I'll create a follow up
> issue to add Java 7 features which I'll change this back to
> StandardCharsets.UTF_8

ok, any update on this?

[diegocarloslima, 2017-02-15 16:53:39]

On 2017/02/15 11:01:54, anton wrote:
> On 2017/01/30 17:14:30, diegocarloslima wrote:
> >
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> > File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29372653/diff/29372654/src/org/adblockplus...
> > src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:440:
> final
> > byte[] b = s.getBytes(StandardCharsets.UTF_8);
> > On 2017/01/23 06:49:40, anton wrote:
> > > StandardCharsets is available starting Java 7. If it's not a problem and
> > > somewhere in README it's noted that JDK 7 is required then we can use it.
> For
> > > JDK 6 we can use `Charset.forName("UTF-8")` instead
> > 
> > For instance, I'll use Charset.forName("UTF-8"). Then, I'll create a follow
up
> > issue to add Java 7 features which I'll change this back to
> > StandardCharsets.UTF_8
> 
> ok, any update on this?

yes, I have uploaded patch set 2 which removes Java 7 features for now

[anton, 2017-02-16 06:17:24]

https://codereview.adblockplus.org/29372653/diff/29373692/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29372653/diff/29373692/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:439: final
byte[] b = s.getBytes("UTF-8");
wouldn't it be better to have static final String CHARSET = "UTF-8" that is used
here and below (in serializeSubscription) ?

[diegocarloslima, 2017-02-22 22:43:53]

https://codereview.adblockplus.org/29372653/diff/29373692/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
(right):

https://codereview.adblockplus.org/29372653/diff/29373692/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:439: final
byte[] b = s.getBytes("UTF-8");
On 2017/02/16 06:17:24, anton wrote:
> wouldn't it be better to have static final String CHARSET = "UTF-8" that is
used
> here and below (in serializeSubscription) ?

Acknowledged.

[anton, 2017-03-13 08:04:07]

On 2017/02/22 22:43:53, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29372653/diff/29373692/src/org/adblockplus...
> File src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java
> (right):
> 
>
https://codereview.adblockplus.org/29372653/diff/29373692/src/org/adblockplus...
> src/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:439:
final
> byte[] b = s.getBytes("UTF-8");
> On 2017/02/16 06:17:24, anton wrote:
> > wouldn't it be better to have static final String CHARSET = "UTF-8" that is
> used
> > here and below (in serializeSubscription) ?
> 
> Acknowledged.

LGTM

[Felix Dahlke, 2017-03-13 09:21:56]

Didn't do a very thorough review, but LGTM.