Issue 2606 - Firefox logo shown in background on empty Adblock Browser New Tab and Search pages

Issue 2606 - Firefox logo shown in background on empty Adblock Browser New Tab and Search pages

[anton, 2016-09-30 07:19:16]

On 2016/05/18 09:11:42, diegocarloslima wrote:

LGTM

[Felix Dahlke, 2016-09-30 07:28:55]

I wonder if we shouldn't just replace icon_home_empty_firefox.png with ours,
that would be a less intrusive change, and more robust towards future changes by
Mozilla (e.g. if they decide to use the icon elsewhere). On the other hand, it's
a bit ugly to have it named Firefox with an ABB logo inside.

No particularly strong opinion, we can also do it this way if you prefer that.
But in that case I think we should point to the relevant issue above all code
changes we're making, we typically do that to make it easier to see our changes.

[diegocarloslima, 2016-10-25 16:38:21]

On 2016/09/30 07:28:55, Felix Dahlke wrote:
> I wonder if we shouldn't just replace icon_home_empty_firefox.png with ours,
> that would be a less intrusive change, and more robust towards future changes
by
> Mozilla (e.g. if they decide to use the icon elsewhere). On the other hand,
it's
> a bit ugly to have it named Firefox with an ABB logo inside.
> 
> No particularly strong opinion, we can also do it this way if you prefer that.
> But in that case I think we should point to the relevant issue above all code
> changes we're making, we typically do that to make it easier to see our
changes.

Aside from being less ugly, depending on which changes they make, the renaming
solution is more robust. E.g., if we keep the current naming and they just add
an icon for drawable-xxhdpi folder, it won't cause any merge issues, but for
xxhdpi devices, it would be shown the Firefox logo again. Since none of them is
100% effective for all changing scenarios, I would go for the less ugly one :) 

And yes, I will add comments pointing for this issue

[Felix Dahlke, 2016-10-27 05:28:18]

On 2016/10/25 16:38:21, diegocarloslima wrote:
> On 2016/09/30 07:28:55, Felix Dahlke wrote:
> > I wonder if we shouldn't just replace icon_home_empty_firefox.png with ours,
> > that would be a less intrusive change, and more robust towards future
changes
> by
> > Mozilla (e.g. if they decide to use the icon elsewhere). On the other hand,
> it's
> > a bit ugly to have it named Firefox with an ABB logo inside.
> > 
> > No particularly strong opinion, we can also do it this way if you prefer
that.
> > But in that case I think we should point to the relevant issue above all
code
> > changes we're making, we typically do that to make it easier to see our
> changes.
> 
> Aside from being less ugly, depending on which changes they make, the renaming
> solution is more robust. E.g., if we keep the current naming and they just add
> an icon for drawable-xxhdpi folder, it won't cause any merge issues, but for
> xxhdpi devices, it would be shown the Firefox logo again. Since none of them
is
> 100% effective for all changing scenarios, I would go for the less ugly one :)


Hm, that's a good point, alright.

> And yes, I will add comments pointing for this issue

Waiting for this then.

[Felix Dahlke, 2016-11-17 18:38:52]

LGTM with two nits. Feel free to address those, or not.

https://codereview.adblockplus.org/29341383/diff/29361535/mobile/android/base...
File mobile/android/base/home/PanelAuthLayout.java (right):

https://codereview.adblockplus.org/29341383/diff/29361535/mobile/android/base...
mobile/android/base/home/PanelAuthLayout.java:56: // Changed icon. See
https://issues.adblockplus.org/ticket/2606
Nit: Makes more sense to me if you'd move this one line down, i.e. after the
upstream comment. But not important.

https://codereview.adblockplus.org/29341383/diff/29361535/mobile/android/base...
File mobile/android/base/home/PanelLayout.java (right):

https://codereview.adblockplus.org/29341383/diff/29361535/mobile/android/base...
mobile/android/base/home/PanelLayout.java:463: // Changed icon. See
https://issues.adblockplus.org/ticket/2606
Nit: I guess it would suffice to have this comment once, above the block (i.e.
moving it one line up). But I don't mind either way, up to you AFAICT.