Issue 2351 - Add a custom menu item for whitelisting the current site

Issue 2351 - Add a custom menu item for whitelisting the current site

[Felix Dahlke, 2015-04-25 21:49:43]

While this works, I'm not sure if it's the best approach yet - we have to add
code at various places in browser.js and BrowserApp.java.

What I could do is: Move pretty much all of the logic to BrowserApp.java and
communicate with ABP via AddonBridge.

What do you think, should I give that a go? The upside of how I've done it so
far is that it's fairly in line with how stuff like that generally works in
Firefox for Android.

[Felix Dahlke, 2015-04-27 09:49:39]

Investigated the alternative a bit, namely: Move all the logic to
BrowserApp.java and communicate with ABP via AddonBridge.

The upside of this approach is that it's actually less intrusive - no changes to
browser.js necessary, and the changes to BrowserApp.java are more in LOC, but a
bit less intrusive.

The downside however is, that this requires more effort, and feels like a hack.
BrowserApp.java (and indeed most of the Android code here) is more of a dumb
shell, the real logic is in JS.

Note that I've also found a bug: When switching tabs, the "Block Ads" menu item
isn't updated properly. Will look into that.

[Felix Dahlke, 2015-05-05 23:44:40]

OK, here's that minimally invasive approach we discussed: I'm not touching
browser.js anymore, all of the logic is in BrowserApp, communicating via
AddOnBridge.

I can't think of a less intrusive change to achieve this. I'm frankly not really
happy with the naming of some things, I'll sleep over it. But let's start with
the review nonetheless :)

[René Jeschke, 2015-05-06 10:56:32]

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/adbl...
File adblockplus/Api.jsm (right):

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/adbl...
adblockplus/Api.jsm:206: this.whitelistSite(data["url"], data["whitelisted"] ==
"true");
Wouldn't it be better to use a 'boolean' here (data["whitelisted"]), instead a
'string', as we are passing a JSON opject that understands booleans?

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right):

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:140: private
static void callFunction(final AdblockPlusApiCallback callback, final String
name,
I must admit, that I would prefer 
'final JSONObject parameters'
instead of
'final Map<String, String> parameters' 
here, because:
1. We only create _one_ object instead of two
2. With your solution we can not use numbers, booleans, arrays, dictionaries,
... which feels like a very limited approach for a generic function

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:209:
parameters.put("whitelisted", String.valueOf(whitelisted));
See comment in Api.jsm and above ... having a JSONObject would allow us to use a
boolean directly here, without stringly typing.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java
(right):

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:1:
package org.adblockplus.browser;
I added a copyright header to all new sources, should we make it mandatory?

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:11:
public class BrowserAppUtils
Just a nit and a matter of taste, I guess:

I tend to add a private, empty constructor to classes that are singletons by
design, to prevent any abuse or the like.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:20: final
String url = selectedTab.getURL();
Nit: A newline (before L20) would improve readability here, same for (before)
L41 and L68.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:27:
ThreadUtils.postToUiThread(new Runnable() {
We are still using Allman bracing style in code that is written by us :D, also
see below

[Felix Dahlke, 2015-05-06 16:42:49]

New patch set is up. Also changed a few things that were bother me, now I'm
happy :)

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/adbl...
File adblockplus/Api.jsm (right):

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/adbl...
adblockplus/Api.jsm:206: this.whitelistSite(data["url"], data["whitelisted"] ==
"true");
On 2015/05/06 10:56:33, René Jeschke wrote:
> Wouldn't it be better to use a 'boolean' here (data["whitelisted"]), instead a
> 'string', as we are passing a JSON opject that understands booleans?

Done.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right):

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:140: private
static void callFunction(final AdblockPlusApiCallback callback, final String
name,
On 2015/05/06 10:56:33, René Jeschke wrote:
> I must admit, that I would prefer 
> 'final JSONObject parameters'
> instead of
> 'final Map<String, String> parameters' 
> here, because:
> 1. We only create _one_ object instead of two
> 2. With your solution we can not use numbers, booleans, arrays, dictionaries,
> ... which feels like a very limited approach for a generic function

As discussed on IRC: A Map is not great, but we'd have to duplicate the
exception handling around JSONObject.put. We'll go with a Map<String, Object>
for now to avoid stringly typing :P

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java
(right):

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:1:
package org.adblockplus.browser;
On 2015/05/06 10:56:33, René Jeschke wrote:
> I added a copyright header to all new sources, should we make it mandatory?

Done.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:11:
public class BrowserAppUtils
On 2015/05/06 10:56:33, René Jeschke wrote:
> Just a nit and a matter of taste, I guess:
> 
> I tend to add a private, empty constructor to classes that are singletons by
> design, to prevent any abuse or the like.

Done.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:20: final
String url = selectedTab.getURL();
On 2015/05/06 10:56:33, René Jeschke wrote:
> Nit: A newline (before L20) would improve readability here, same for (before)
> L41 and L68.

Done.

http://codereview.adblockplus.org/5365916275572736/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/BrowserAppUtils.java:27:
ThreadUtils.postToUiThread(new Runnable() {
On 2015/05/06 10:56:33, René Jeschke wrote:
> We are still using Allman bracing style in code that is written by us :D, also
> see below

Done.

[René Jeschke, 2015-05-06 16:53:09]

LGTM