Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
July 5, 2017, 9:05 a.m. by jens
Modified:
July 26, 2017, 8:30 a.m.
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
July 5, 2017, 9:13 a.m. (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: ...
July 6, 2017, 6:51 a.m. (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 ...
July 6, 2017, 8:49 a.m. (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 ...
July 6, 2017, 6:25 p.m. (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 ...
July 7, 2017, 1:03 p.m. (2017-07-07 13:03:19 UTC) #5
diegocarloslima
July 10, 2017, 3:44 p.m. (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

Powered by Google App Engine
This is Rietveld