Issue 4920541991403520: Create a minimal settings UI

Issue 4920541991403520: Create a minimal settings UI (Closed)

Created:
March 20, 2015, 10:42 p.m. by René Jeschke

Modified:
May 28, 2015, 7:30 p.m.

Reviewers:
Felix Dahlke

Visibility:
Public.

Patch Set 2 : Hopefully removed wrong files #

Patch Set 3 : Typos, extra-summary into strings.xml. #

Patch Set 4 : Added missing slashes #

Patch Set 5 : Cleanup of AddOnBridge #

Total comments: 66

Patch Set 6 : Addressed issues #

Patch Set 7 : Forgot commenting out #

Total comments: 1

Patch Set 8 : Some more renamings #

Patch Set 11 : Removed default case #

Created: March 22, 2015, 9:23 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+568 lines, -0 lines)			Patch
M	adblockplus/Api.jsm	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+34 lines, -0 lines	0 comments	Download
M	mobile/android/base/moz.build	View	1 2 3 4 5	1 chunk	+7 lines, -0 lines	0 comments	Download
M	mobile/android/base/resources/xml-v11/preferences.xml	View	1 2 3 4 5 6 7	1 chunk	+7 lines, -0 lines	0 comments	Download
A	mobile/android/base/resources/xml-v11/preferences_abb_abp.xml	View	1 2 3 4 5 6 7	1 chunk	+36 lines, -0 lines	0 comments	Download
M	mobile/android/base/resources/xml/preferences.xml	View	1 2 3 4 5 6 7	1 chunk	+11 lines, -0 lines	0 comments	Download
A	mobile/android/base/resources/xml/preferences_abb_abp.xml	View	1 2 3 4 5 6 7	1 chunk	+38 lines, -0 lines	0 comments	Download
A	mobile/android/base/resources/xml/preferences_abb_abp_acceptable_ads.xml	View	1 2 3 4 5 6 7	1 chunk	+28 lines, -0 lines	0 comments	Download
M	mobile/android/base/strings.xml.in	View	1 2 3 4 5 6 7	1 chunk	+25 lines, -0 lines	0 comments	Download
M	mobile/android/chrome/content/browser.js	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
A	mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java	View	1 2 3 4 5 6 7	1 chunk	+93 lines, -0 lines	0 comments	Download
A	mobile/android/thirdparty/org/adblockplus/browser/AdblockPlusApiCallback.java	View		1 chunk	+26 lines, -0 lines	0 comments	Download
A	mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java	View	1 2 3 4 5 6 7	1 chunk	+259 lines, -0 lines	0 comments	Download

Messages

Total messages: 14

Expand All Messages | Collapse All Messages

Felix Dahlke

Haven't reviewed the Java code yet, but here's a few comments about the rest. http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/adblockplus/Api.jsm ...

March 22, 2015, 1:52 p.m. (2015-03-22 13:52:34 UTC) #2

René Jeschke

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/adblockplus/Api.jsm File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/adblockplus/Api.jsm#newcode41 adblockplus/Api.jsm:41: let {FilterNotifier} = require("filterNotifier"); On 2015/03/22 13:52:34, Felix H. ...

March 22, 2015, 3:32 p.m. (2015-03-22 15:32:59 UTC) #3

Felix Dahlke

OK, reviewed everything now. http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/base/resources/xml-v11/preferences.xml File mobile/android/base/resources/xml-v11/preferences.xml (right): http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/base/resources/xml-v11/preferences.xml#newcode41 mobile/android/base/resources/xml-v11/preferences.xml:41: android:value="preferences_abp_adblockplus" /> On 2015/03/22 15:32:59, ...

March 22, 2015, 3:51 p.m. (2015-03-22 15:51:45 UTC) #4

René Jeschke

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java File mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java (right): http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java#newcode27 mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:27: public class AbpCheckBoxPreference extends CustomCheckBoxPreference implements On 2015/03/22 15:51:45, ...

March 22, 2015, 4:44 p.m. (2015-03-22 16:44:55 UTC) #5

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

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:27:
public class AbpCheckBoxPreference extends CustomCheckBoxPreference implements
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> What's that, 80 columns? :P We do have 100 for Java code.

Yes, and it would end on column 102 :p

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:56:
private void recheckValue()
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> Naming nit: Isn't this rather logically "refreshValue"?

Yup

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:62:
protected void onClick()
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> IIRC, setChecked is invoked whenever the value changes (thus including
> super.onClick) - so how about overwriting that instead?

Because I call 'setChecked()' to set the checkbox checked, and I want it to
change when the user 'clicks' it.

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

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:47:
HANDLER_THREAD = new HandlerThread("abp-addon-bridge");
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> "abp-bridge" would do. Technically, there's no difference between code
included
> as part of the browser and code from an extension.

Done.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:53: public
static boolean getBooleanFromJSObject(final NativeJSObject obj, final String
name,
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> Naming Nit: getBooleanFromJsObject, likewise get*FromJsObject below.

Done.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:110: public
static void queryBoolean(final AdblockPlusApiCallback callback, final String
action)
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> IMHO this is a bit confusing: queryBoolean sounds like it'll just return a
> boolean for some key, while in reality it's more lower level messaging
actually.
> 
> Since we don't need anything but setting/getting values yet, how about
> simplifying it a bit? We could have:
> 
> void queryBoolean(final AdblockPlusApiCallback callback, final String name)
> 
> (Or just "getBoolean" analogous to "setBoolean"?)
> 
> Which would then call the more lower level "createRequestData" prepending
"get"
> in this case.
> 
> Note that you've updated the actions to be in camel case in the mean time - so
> this won't work anymore. I was actually fine with the underscores after
reading
> the Java code here, makes sense to stick to typical Android naming. And if we
> name the checkbox "acceptable_ads_enabled" and the action
> "get_acceptable_ads_enabled", it'll be the same thing as
> "getAcceptableAdsEnabled", just underscores instead of camel case - which is
not
> a problem at all IMO.

It is called 'query' because it does not 'get' a boolean. It queries ABP for the
state of it, but does not return it. The result is supplied via the callback.

The 'action' name is legacy naming, can change that, though.

It also is not really low_level_messaging, as this method _must_ be asynchronous
anyway. I could have overengineered this thing by returning Futures or Promises,
but I wanted to keep it simple.

Also, 'not needed yet' is not a good reason for simplifying the API, I also had
the case in ABPdroid where I regretted  that I did not insist on keeping the
more complex CommandBridge :p.

Thing is: 'get' is wrong, as it does not return a value. Making it a getter is
also impossible, as this would mean blocking on the UI or gecko thread.

.... but, yes, the action thing is confusing, so I will change that.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:137: final
boolean checkInitState, final long creationTime)
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> I don't fully understand this one - 

We start an asynchronous chain of requests here:
The value and callback passed to this request are the values for the final
request (or an error).

The first request (when 'checkInitState' is set) will be a check for
'filtersLoaded'
The response of this request is then checked for success and filtersLoaded==true
If this is true, the real request will be issued
otherwise we retry for a while (loop to beginning) and eventually report a
timeout error.

The naming for 'checkInitState' is now wrong, as you wanted to have the action
called 'filtersLoaded' ... so I will change this, too.

Yes, I could extract the logic into a separate place, but this would not make a
lot of a difference, instead we would have a lot more code and duplication.

It is not really an overhead checking the ready state each time we do something,
so I would like to keep it here.

Also, it encapsulates all logic, and one can just send a 'ChainedRequest' and
everything _just works_.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:162: private
void callSuccessFunction(final NativeJSObject jsObject)
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> "invokeSuccessCallback"?

Ok

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:190: private
void maybeReTry()
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> How about "attemptRetry"?

Yup.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:195: }
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> Shouldn't we return here, or am I missing something?

Yeah, already changed.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:229:
GeckoAppShell.sendRequestToGecko(this.cloneForRequest());
On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> So.... Not entirely sure I understand what initCheck is for: But does this
mean
> we're checking the value twice? Surely worth a comment.

Yes, we are.

onResponse() happens on the main Gecko Thread, but we definitely have at least
three threads involved here:

the Android UI thread which started this request
the Gecko thread that handles it
the IO thread that fetches the filterSubscription
(or any other lazy-evaluation mechanism)

It is possible that we arrive here, after "initCommunication", but during
"filterLoading", which would mean that we're not yet ready. As loading continues
in the background, we need to re-check some time later.

That's what this class does, asynchronously delays until the extension is ready.

Felix Dahlke

Looks good now, except for the onClick stuff - seems to me like we can ...

March 22, 2015, 5:44 p.m. (2015-03-22 17:44:34 UTC) #6

Looks good now, except for the onClick stuff - seems to me like we can do better
with overwriting setChecked.

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

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:27:
public class AbpCheckBoxPreference extends CustomCheckBoxPreference implements
On 2015/03/22 16:44:55, René Jeschke wrote:
> On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> > What's that, 80 columns? :P We do have 100 for Java code.
> 
> Yes, and it would end on column 102 :p

Touché :P

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:62:
protected void onClick()
On 2015/03/22 16:44:55, René Jeschke wrote:
> On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> > IIRC, setChecked is invoked whenever the value changes (thus including
> > super.onClick) - so how about overwriting that instead?
> 
> Because I call 'setChecked()' to set the checkbox checked, and I want it to
> change when the user 'clicks' it.

Well, you could use super.setChecked. Potentially problematic here is that if
the checkbox is somehow checked without clicking it, we'll miss it.

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

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:110: public
static void queryBoolean(final AdblockPlusApiCallback callback, final String
action)
On 2015/03/22 16:44:55, René Jeschke wrote:
> On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> > IMHO this is a bit confusing: queryBoolean sounds like it'll just return a
> > boolean for some key, while in reality it's more lower level messaging
> actually.
> > 
> > Since we don't need anything but setting/getting values yet, how about
> > simplifying it a bit? We could have:
> > 
> > void queryBoolean(final AdblockPlusApiCallback callback, final String name)
> > 
> > (Or just "getBoolean" analogous to "setBoolean"?)
> > 
> > Which would then call the more lower level "createRequestData" prepending
> "get"
> > in this case.
> > 
> > Note that you've updated the actions to be in camel case in the mean time -
so
> > this won't work anymore. I was actually fine with the underscores after
> reading
> > the Java code here, makes sense to stick to typical Android naming. And if
we
> > name the checkbox "acceptable_ads_enabled" and the action
> > "get_acceptable_ads_enabled", it'll be the same thing as
> > "getAcceptableAdsEnabled", just underscores instead of camel case - which is
> not
> > a problem at all IMO.
> 
> It is called 'query' because it does not 'get' a boolean. It queries ABP for
the
> state of it, but does not return it. The result is supplied via the callback.
> 
> The 'action' name is legacy naming, can change that, though.
> 
> It also is not really low_level_messaging, as this method _must_ be
asynchronous
> anyway. I could have overengineered this thing by returning Futures or
Promises,
> but I wanted to keep it simple.
> 
> Also, 'not needed yet' is not a good reason for simplifying the API, I also
had
> the case in ABPdroid where I regretted  that I did not insist on keeping the
> more complex CommandBridge :p.
> 
> Thing is: 'get' is wrong, as it does not return a value. Making it a getter is
> also impossible, as this would mean blocking on the UI or gecko thread.
> 
> .... but, yes, the action thing is confusing, so I will change that.

OK, fair enough.

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:137: final
boolean checkInitState, final long creationTime)
On 2015/03/22 16:44:55, René Jeschke wrote:
> On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> > I don't fully understand this one - 
> 
> We start an asynchronous chain of requests here:
> The value and callback passed to this request are the values for the final
> request (or an error).
> 
> The first request (when 'checkInitState' is set) will be a check for
> 'filtersLoaded'
> The response of this request is then checked for success and
filtersLoaded==true
> If this is true, the real request will be issued
> otherwise we retry for a while (loop to beginning) and eventually report a
> timeout error.
> 
> The naming for 'checkInitState' is now wrong, as you wanted to have the action
> called 'filtersLoaded' ... so I will change this, too.
> 
> Yes, I could extract the logic into a separate place, but this would not make
a
> lot of a difference, instead we would have a lot more code and duplication.
> 
> It is not really an overhead checking the ready state each time we do
something,
> so I would like to keep it here.
> 
> Also, it encapsulates all logic, and one can just send a 'ChainedRequest' and
> everything _just works_.

Alright, get it now - so we're checking for filtersLoaded before doing anything
else. With that renamed callback, I get it :D

René Jeschke

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java File mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java (right): http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java#newcode62 mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:62: protected void onClick() On 2015/03/22 17:44:34, Felix H. Dahlke ...

March 22, 2015, 6:32 p.m. (2015-03-22 18:32:41 UTC) #7

Felix Dahlke

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java File mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java (right): http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java#newcode62 mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:62: protected void onClick() On 2015/03/22 18:32:41, René Jeschke wrote: ...

March 22, 2015, 8:10 p.m. (2015-03-22 20:10:24 UTC) #8

René Jeschke

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java File mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java (right): http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java#newcode62 mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:62: protected void onClick() On 2015/03/22 20:10:24, Felix H. Dahlke ...

March 22, 2015, 9:04 p.m. (2015-03-22 21:04:45 UTC) #9

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

http://codereview.adblockplus.org/4920541991403520/diff/5750085036015616/mobi...
mobile/android/thirdparty/org/adblockplus/browser/AbpCheckBoxPreference.java:62:
protected void onClick()
On 2015/03/22 20:10:24, Felix H. Dahlke wrote:
> On 2015/03/22 18:32:41, René Jeschke wrote:
> > On 2015/03/22 17:44:34, Felix H. Dahlke wrote:
> > > On 2015/03/22 16:44:55, René Jeschke wrote:
> > > > On 2015/03/22 15:51:45, Felix H. Dahlke wrote:
> > > > > IIRC, setChecked is invoked whenever the value changes (thus including
> > > > > super.onClick) - so how about overwriting that instead?
> > > > 
> > > > Because I call 'setChecked()' to set the checkbox checked, and I want it
> to
> > > > change when the user 'clicks' it.
> > > 
> > > Well, you could use super.setChecked. Potentially problematic here is that
> if
> > > the checkbox is somehow checked without clicking it, we'll miss it.
> > 
> > If we do put this logic into 'setChecked()' then we can never ever change
the
> > value, because the second this component gets initialized, it will read it's
> > default value and call 'setChecked()', which then makes the value persistent
> by
> > sending it to ABP.
> 
> You mean, CheckBoxPreference itself will call setChecked on itself initially?
> That's problematic of course, if that's the case we probably have to go for
> onClick.
> 
> > onAttachToActivity() fetches the current value from ABP and calls
> 'setChecked()'
> > to reflect this value onto our checkbox.
> 
> Can't we effectively call super.setChecked in onApiRequestSucceeded?
>  
> > From this moment on, we're displayed as a UI element ... and the only way
this
> > will get changed is by 'clicking'.
> 
> So far the theory :) Would arguably be more robust to support setChecked
> properly.

Ok, as discussed on IRC, we will leave this as is.

Felix Dahlke

http://codereview.adblockplus.org/4920541991403520/diff/5764640680181760/adblockplus/Api.jsm File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4920541991403520/diff/5764640680181760/adblockplus/Api.jsm#newcode56 adblockplus/Api.jsm:56: initCommunication : function() Nit: No space before :. Also ...

March 22, 2015, 9:05 p.m. (2015-03-22 21:05:09 UTC) #10

René Jeschke

March 22, 2015, 9:18 p.m. (2015-03-22 21:18:14 UTC) #11

René Jeschke

On 2015/03/22 21:18:14, René Jeschke wrote: > http://codereview.adblockplus.org/4920541991403520/diff/5764640680181760/adblockplus/Api.jsm > File adblockplus/Api.jsm (right): > > http://codereview.adblockplus.org/4920541991403520/diff/5764640680181760/adblockplus/Api.jsm#newcode56 ...

March 22, 2015, 9:23 p.m. (2015-03-22 21:23:56 UTC) #12

LGTM!

Expand All Messages | Collapse All Messages