Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1322)

Issue 29480569: Issue 5254 - Improve URL verification when whitelisting domains (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by jens
Modified:
1 year, 11 months ago
Reviewers:
anton, diegocarloslima
CC:
Felix Dahlke, René Jeschke
Visibility:
Public.

Description

Issue 5254 - Improve URL verification when whitelisting domains

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -3 lines) Patch
M adblockplussbrowser/build.gradle View 2 chunks +5 lines, -1 line 5 comments Download
A adblockplussbrowser/proguard-rules.pro View 1 chunk +2 lines, -0 lines 0 comments Download
M adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java View 2 chunks +2 lines, -2 lines 2 comments Download

Messages

Total messages: 6
jens
2 years ago (2017-07-05 09:13:30 UTC) #1
anton
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle#newcode1 adblockplussbrowser/build.gradle:1: this change (new line) is not required https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle#newcode42 adblockplussbrowser/build.gradle:42: ...
2 years ago (2017-07-06 06:51:11 UTC) #2
jens
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle#newcode42 adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { On ...
2 years ago (2017-07-06 08:49:06 UTC) #3
diegocarloslima
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle#newcode42 adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { On ...
2 years ago (2017-07-06 18:25:16 UTC) #4
anton
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser/build.gradle#newcode42 adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator', name: 'commons-validator', version: '1.6') { On ...
2 years ago (2017-07-07 13:03:19 UTC) #5
diegocarloslima
2 years ago (2017-07-10 15:44:52 UTC) #6
On 2017/07/07 13:03:19, anton wrote:
>
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser...
> File adblockplussbrowser/build.gradle (right):
> 
>
https://codereview.adblockplus.org/29480569/diff/29480570/adblockplussbrowser...
> adblockplussbrowser/build.gradle:42: compile (group: 'commons-validator',
name:
> 'commons-validator', version: '1.6') {
> On 2017/07/06 18:25:16, diegocarloslima wrote:
> > On 2017/07/06 08:49:06, jens wrote:
> > > On 2017/07/06 06:51:10, anton wrote:
> > > > should we really use the whole Apache commons module for just URL
> > validation?
> > > > In libadblockplus-android we originally copied Apache Strings right into
> app
> > > > source code and then reimplemented it with our own code.
> > > > 
> > > > Can't we really implement (or copy the code which is not desired but
> > possible)
> > > > this on our own?
> > > > I'd prefer to cover it by tests too.
> > > 
> > > I think it's okay to use commons validator, as the list of valid TLDs is
> > updated
> > > regularly. We just need to check if there is a new version available or
> > > alternatively use "version: '+'" to always use the lastest release. Even
if
> > the
> > > latter one is not ideal, it shouldn't be a problem if we cover the
validity
> > > check by tests. With proguard activated the APK is just slightly bigger
than
> > > before: 1.666.715 Byte vs. 1.709.685 Byte
> > > 
> > > Copying the code and keeping the TLDs up to date would be a bigger effort
> than
> > > to just use the lib. In view of the fact that the apk is only slightly
> bigger,
> > > this solution would be fine for me. But of course we can talk about this
> with
> > > the others.
> > > 
> > 
> > IHMO, for this project, we don't have a high size requirement as we do have
in
> > libadblockplus-android, so the size increase in this case sounds fine to me.
> But
> > also, a extremely high quality TLD validation is currently just a 'nice to
> have'
> > feature, since this is currently just used when the user whitelist a
website.
> So
> > I'm quite fine with either keeping the dependency or extracting the relevant
> > piece of code to our project.
> 
> okay, let's just revert adding new empty lines and then it LGTM

This also LGTM by reverting the unnecessary added line in build.gradle
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5