Issue 2491 - Fix design elements in onboarding slides

Issue 2491 - Fix design elements in onboarding slides

[René Jeschke, 2015-05-21 13:40:38]

Here's how it looks now (on a Samsung Galaxy S2):

https://www.dropbox.com/s/8y7560ywkug27pb/abb_nfrp_1.png?dl=0

https://www.dropbox.com/s/uk9qbpoe3lbdujx/abb_nfrp_2.png?dl=0

https://www.dropbox.com/s/pepc8vdsfrvn89j/abb_nfrp_3.png?dl=0

[René Jeschke, 2015-05-21 15:18:18]

Remarks about the solution I chose for embedding custom fonts:

The 'official' way is to put fonts into the 'assets' folder, as there's a static
method that allows creating Typefaces from assets. I decided to not go that way,
because of the amount of Fennec build files that I would have to modify for
this.

In the end I went with having the fonts in 'res/raw' and copy them around at
initialization. Works fine and is the most minimal invasive approach there is.

[Felix Dahlke, 2015-05-22 11:50:57]

It's kind of horrible, but it's really the nicest solution possible I suppose :)

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/base/resources/layout/abb_start_pane.xml (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/base/resources/layout/abb_start_pane.xml:26:
android:background="@drawable/abb_frp_button"
"frp" is not a thing in the browser, is it? Shouldn't it rather be
"abb_start_pane_button"? Or, since we don't prefix any of this so far, maybe
just "abb_button".

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/base/strings.xml.in (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/base/strings.xml.in:525: <string
name="abb_frp_step_2_text_where">Tap the Menu » Settings » Adblocking »
Acceptable Ads</string>
It's actually Menu->Settings->Adblocking->Adblocking->Acceptable Ads, isn't it?

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/StartPane.java (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/thirdparty/org/adblockplus/browser/StartPane.java:209: if (tag !=
null)
Shouldn't we also check that the tags starts with "ttf_" here? Tags can be used
for other things than fonts, right?

[René Jeschke, 2015-05-22 12:04:44]

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/base/resources/layout/abb_start_pane.xml (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/base/resources/layout/abb_start_pane.xml:26:
android:background="@drawable/abb_frp_button"
On 2015/05/22 11:50:57, Felix H. Dahlke wrote:
> "frp" is not a thing in the browser, is it? Shouldn't it rather be
> "abb_start_pane_button"? Or, since we don't prefix any of this so far, maybe
> just "abb_button".

You're right.

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/base/strings.xml.in (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/base/strings.xml.in:525: <string
name="abb_frp_step_2_text_where">Tap the Menu » Settings » Adblocking »
Acceptable Ads</string>
On 2015/05/22 11:50:57, Felix H. Dahlke wrote:
> It's actually Menu->Settings->Adblocking->Adblocking->Acceptable Ads, isn't
it?

Na, on my device it's Settings -> Adblocking -> Acceptable Ads :-)

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/StartPane.java (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/thirdparty/org/adblockplus/browser/StartPane.java:209: if (tag !=
null)
On 2015/05/22 11:50:57, Felix H. Dahlke wrote:
> Shouldn't we also check that the tags starts with "ttf_" here? Tags can be
used
> for other things than fonts, right?

We are matching non-null tags against the font hashmap, so every tag that is in
the font hashmap will result in a font, every other will be ignored.

I don't think we really need _two_ checks here, the hashmap should be enough.

[Felix Dahlke, 2015-05-22 12:09:57]

LGTM

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/base/strings.xml.in (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/base/strings.xml.in:525: <string
name="abb_frp_step_2_text_where">Tap the Menu » Settings » Adblocking »
Acceptable Ads</string>
On 2015/05/22 12:04:44, René Jeschke wrote:
> On 2015/05/22 11:50:57, Felix H. Dahlke wrote:
> > It's actually Menu->Settings->Adblocking->Adblocking->Acceptable Ads, isn't
> it?
> 
> Na, on my device it's Settings -> Adblocking -> Acceptable Ads :-)

OK good point, let's go with the less-awkward-looking option :)

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
File mobile/android/thirdparty/org/adblockplus/browser/StartPane.java (right):

http://codereview.adblockplus.org/6296160390086656/diff/5757334940811264/mobi...
mobile/android/thirdparty/org/adblockplus/browser/StartPane.java:209: if (tag !=
null)
On 2015/05/22 12:04:44, René Jeschke wrote:
> On 2015/05/22 11:50:57, Felix H. Dahlke wrote:
> > Shouldn't we also check that the tags starts with "ttf_" here? Tags can be
> used
> > for other things than fonts, right?
> 
> We are matching non-null tags against the font hashmap, so every tag that is
in
> the font hashmap will result in a font, every other will be ignored.
> 
> I don't think we really need _two_ checks here, the hashmap should be enough.

Yeah true.

[annlee, 2015-05-22 12:16:39]

LGTM :D :D