Followup because we need to make these items `Themed*` and it's a pain.

via bug 1070087: (In reply to Anthony Lam (:antlam) from comment #30) > Comment on attachment 8499908 [details] > Private browsing, tapped button, smaller visual > > Can't find where I left this value but if it's not too much trouble could we > do #222222 for V1 visual feedback on-press? For focused (with non-touch > devices) we could use #363B40 for now (same as the background).

Good idea! I also found where I left that value, just a couple comments above in that same bug :P

Bug 1077195 will determine the final colors for private browsing.

I hate to complicate the generated code any more but oh well.

Comment on attachment 8502899 [details] [diff] [review] Change pressed/focused private browsing colors of new tablet menu items Review of attachment 8502899 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I'm just not convinced that STYLE_CONSTRUCTOR is necessary. ::: mobile/android/base/widget/ThemedView.java.frag @@ +30,5 @@ > private boolean mIsDark; > private boolean mAutoUpdateTheme = true; > > public Themed@VIEW_NAME_SUFFIX@(Context context, AttributeSet attrs) { > +#ifndef STYLE_CONSTRUCTOR I see no reason to make this constructor optional. Does it cause any build errors if you simply added it for all ThemedViews?

(In reply to Lucas Rocha (:lucasr) from comment #6) > I see no reason to make this constructor optional. Does it cause any build > errors if you simply added it for all ThemedViews? While I haven't tried it, LinearLayout's three param constructor requires API 11 [1] and one of the other Views (I don't remember off-hand) does not work either. I could simplify it by always having the three argument constructor, but calling `super(arg1, arg2)` on some and `super(arg1, arg2, arg3)` on others – I'll do that, unless you disagree. [1]: https://developer.android.com/reference/android/widget/LinearLayout.html#LinearLayout(android.content.Context, android.util.AttributeSet, int)

(In reply to Michael Comella (:mcomella) from comment #7) > (In reply to Lucas Rocha (:lucasr) from comment #6) > > I see no reason to make this constructor optional. Does it cause any build > > errors if you simply added it for all ThemedViews? > > While I haven't tried it, LinearLayout's three param constructor requires > API 11 [1] and one of the other Views (I don't remember off-hand) does not > work either. > > I could simplify it by always having the three argument constructor, but > calling `super(arg1, arg2)` on some and `super(arg1, arg2, arg3)` on others > – I'll do that, unless you disagree. Works for me.

Updated patch, as per comment 7.

Backed out in comment 11 for Fx-Team bustage (https://tbpl.mozilla.org/?tree=Fx-Team&rev=fa1f219e3eba). bug 1075531 was also backed out, but I don't think that caused the issue.

It seems the issue is that we weren't calling super(arg1, arg2) for the two argument constructors, which defines a default style for each class such as "com.android.internal.R.attr.imageButtonStyle".

Note that if we wanted to be extra safe, we could avoid making changes in unnecessary places (i.e. everywhere but ThemedImageButton).

Comment on attachment 8504135 [details] [diff] [review] Part 2: Remove style constructor from unnecessary generated views Review of attachment 8504135 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/widget/ThemedView.java.frag @@ +41,5 @@ > a.recycle(); > } > > +#ifdef STYLE_CONSTRUCTOR > + public Themed@VIEW_NAME_SUFFIX@(Context context, AttributeSet attrs, int defStyle) { The code duplication in these constructors is a bit unfortunate. No good way to avoid it? I don't feel so strongly about this because this is just a template anyway.

(In reply to Lucas Rocha (:lucasr) from comment #17) > The code duplication in these constructors is a bit unfortunate. No good way > to avoid it? One way is to look up the default styles by hand and include them in each template file. However, this duplicates the original source code and code face problems with new versions of Android. We could also remove the `final` modifiers on some of these variables and call out to a shared method, at the cost of that type safety. I'd be down to implement the second if you prefer.

(In reply to Michael Comella (:mcomella) from comment #18) > (In reply to Lucas Rocha (:lucasr) from comment #17) > > The code duplication in these constructors is a bit unfortunate. No good way > > to avoid it? > > One way is to look up the default styles by hand and include them in each > template file. However, this duplicates the original source code and code > face problems with new versions of Android. > > We could also remove the `final` modifiers on some of these variables and > call out to a shared method, at the cost of that type safety. > > I'd be down to implement the second if you prefer. Yeah, that sounds like a good compromise (removing the final modifiers) given that this is generated code that is unlikely to change often.

Called into initialize method, as recommended.

Comment on attachment 8502900 [details] Private browsing, tapped button Looks very subtle but I'm ok with this as a first iteration (kinda like how it goes with the whole dark theme)... We can refine as we go :) looks good!

Rebase.

Rebase.

https://hg.mozilla.org/integration/fx-team/rev/193d91361fc1 https://hg.mozilla.org/integration/fx-team/rev/ac4b384f5e1e

Ignore comment 24 - bzexport fail.

https://hg.mozilla.org/integration/fx-team/rev/43ae53dc6394 https://hg.mozilla.org/integration/fx-team/rev/73da2cbb36a5

https://hg.mozilla.org/mozilla-central/rev/43ae53dc6394 https://hg.mozilla.org/mozilla-central/rev/73da2cbb36a5