Issue 2350 - Remove Add-ons, Apps and Sync

Issue 2350 - Remove Add-ons, Apps and Sync

[René Jeschke, 2015-04-21 09:52:44]

*argh* ... this does not work^^

Will up a working solution soon, sry :-D

[René Jeschke, 2015-04-21 09:59:45]

So, had to remove the references to the (now) missing items from BrowserApp,
works now.

[René Jeschke, 2015-04-21 12:37:20]

And, just a remark for Sync:

There is a code line, that removes sync programmatically ... but it only queries
if 'inGuestMode', so I decided to remove it from the XMLs.

[Felix Dahlke, 2015-04-22 13:29:59]

http://codereview.adblockplus.org/5664054114779136/diff/5757334940811264/mobi...
File mobile/android/base/BrowserApp.java (right):

http://codereview.adblockplus.org/5664054114779136/diff/5757334940811264/mobi...
mobile/android/base/BrowserApp.java:2577: // Removed R.id.apps and R.id.addons
as of issue #2350
As in that other review - how about s/as of/as part of/ and s/issue
#2350/https://issues.adblockplus.org/ticket/2350?

The same would go for the other "removed" comments.

http://codereview.adblockplus.org/5664054114779136/diff/5757334940811264/mobi...
mobile/android/base/BrowserApp.java:2745: // Removed ADDONS and APPS as of issue
#2350
Tab, not spaces?

http://codereview.adblockplus.org/5664054114779136/diff/5757334940811264/mobi...
File mobile/android/base/resources/menu-large-v11/browser_app_menu.xml (right):

http://codereview.adblockplus.org/5664054114779136/diff/5757334940811264/mobi...
mobile/android/base/resources/menu-large-v11/browser_app_menu.xml:85: <!--
removed addons as of issue #2350 -->
Can't we just make these invisible instead of fully removing them? Then we
wouldn't have to touch BrowserApp.java.

[René Jeschke, 2015-04-23 15:05:00]

Done.

[Felix Dahlke, 2015-04-23 16:07:33]

http://codereview.adblockplus.org/5664054114779136/diff/5733935958982656/mobi...
File mobile/android/base/resources/menu-large-v11/browser_app_menu.xml (right):

http://codereview.adblockplus.org/5664054114779136/diff/5733935958982656/mobi...
mobile/android/base/resources/menu-large-v11/browser_app_menu.xml:88:
android:visible="false" />
The "visible" attribute generally seems to come between "icon" and "title"
below. Wouldn't hurt to do that here too I guess - minimally invasive change
that way. Otherwise I'd nit pick that there's usually no space before `/>` :P

[René Jeschke, 2015-04-23 20:27:41]

On 2015/04/23 16:07:33, Felix H. Dahlke wrote:
>
http://codereview.adblockplus.org/5664054114779136/diff/5733935958982656/mobi...
> File mobile/android/base/resources/menu-large-v11/browser_app_menu.xml
(right):
> 
>
http://codereview.adblockplus.org/5664054114779136/diff/5733935958982656/mobi...
> mobile/android/base/resources/menu-large-v11/browser_app_menu.xml:88:
> android:visible="false" />
> The "visible" attribute generally seems to come between "icon" and "title"
> below. Wouldn't hurt to do that here too I guess - minimally invasive change
> that way. Otherwise I'd nit pick that there's usually no space before `/>` :P

Changed it, but ... it's pretty much standard to use a whitespace before the />
in an empty element when there are also attributes. (Just look around the
Android dev pages)

... Mozilla just isn't very consistent when it comes to this, though, as they
even mix it in a single file. (and have a rough 60/40 ratio in the whole project
:D )

:P

[Felix Dahlke, 2015-04-23 20:34:12]

LGTM

On 2015/04/23 20:27:41, René Jeschke wrote:
> On 2015/04/23 16:07:33, Felix H. Dahlke wrote:
> >
>
http://codereview.adblockplus.org/5664054114779136/diff/5733935958982656/mobi...
> > File mobile/android/base/resources/menu-large-v11/browser_app_menu.xml
> (right):
> > 
> >
>
http://codereview.adblockplus.org/5664054114779136/diff/5733935958982656/mobi...
> > mobile/android/base/resources/menu-large-v11/browser_app_menu.xml:88:
> > android:visible="false" />
> > The "visible" attribute generally seems to come between "icon" and "title"
> > below. Wouldn't hurt to do that here too I guess - minimally invasive change
> > that way. Otherwise I'd nit pick that there's usually no space before `/>`
:P
> 
> Changed it, but ... it's pretty much standard to use a whitespace before the
/>
> in an empty element when there are also attributes. (Just look around the
> Android dev pages)
> 
> ... Mozilla just isn't very consistent when it comes to this, though, as they
> even mix it in a single file. (and have a rough 60/40 ratio in the whole
project
> :D )
> 
> :P

Yeah, I prefer the space too, however:
https://adblockplus.org/en/coding-style#consistency - if the surrounding code is
completely inconsistent it doesn't matter of course, but that didn't seem to be
the case here.

[Felix Dahlke, 2015-04-24 05:46:54]

Assuming this isn't pushed yet - I just realised it makes more sense to commit
removing Add-ons, Apps and Sync in three separate commits. We're quite unlike to
enable them all at the same time again. (Three reviews/commits linked to that
one issue will do.)

[Felix Dahlke, 2015-04-25 18:22:18]

On 2015/04/24 05:46:54, Felix H. Dahlke wrote:
> Assuming this isn't pushed yet - I just realised it makes more sense to commit
> removing Add-ons, Apps and Sync in three separate commits. We're quite unlike
to
> enable them all at the same time again. (Three reviews/commits linked to that
> one issue will do.)

Actually, I've split this up into three issues - shouldn't have lumped it
together in the first place. It's quite likely that we will revert these changes
individually...