1148028 - Remove MOZ_ANDROID_SHARE_OVERLAY #define

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Since we're shipping the share overlay, this isn't useful anymore.

Comment 1

[Vivek Balakrishnan[:vivek]]

@Kalpesh,

Would you be interested in this bug?
This involves clean up all the definitions in http://mxr.mozilla.org/mozilla-central/search?string=MOZ_ANDROID_SHARE_OVERLAY

Comment 2

[Kalpesh Krishna [:martianwars]]

Alright I'll work on it. :) Is it similar to the bug to remove MOZ_ANDROID_FIREFOX_ACCOUNT_PROFILES flag? If not, how do I proceed?

Comment 3

[Vivek Balakrishnan[:vivek]]

Kalpesh,
Yes it is similar :)
Thanks for working on this

Comment 4

[Kalpesh Krishna [:martianwars]]

Attachment: overlayfix.patch

I hope it is okay. Could you tell me the tree command to test it?

Comment 5

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8690581 [details] [diff] [review]
overlayfix.patch

Review of attachment 8690581 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

This patch is not doing the right thing. Sorry, I am going to give you a r-
The features that was built behind the "MOZ_ANDROID_SHARE_OVERLAY" flag is in production now, which makes the flag redundant.
The idea behind this bug is to remove the flag while retaining the features.
Please let me know if this is not clear.

This is similar to the  bug 1207307 you solved

Regarding try server, just pick all android stuff from try chooser.
BTW, can you please try clobber building your patch.
I have  a nagging suspicion that it may not compile.

I have not review all the files, but I hope to see an updated patch soon :)

::: mobile/android/base/moz.build
@@ -633,5 @@
>      ANDROID_RES_DIRS += [ 'crashreporter/res' ]
>  
> -if CONFIG['MOZ_ANDROID_SHARE_OVERLAY']:
> -    gbjar.sources += [
> -        'overlays/OverlayConstants.java',

This is not right. We don't want to remove the Share Overlay related java files. we want to remove only the "MOZ_ANDROID_SHARE_OVERLAY" flag.

Comment 6

[Kalpesh Krishna [:martianwars]]

Attachment: overlayflag.patch

Hey I reworked the entire patch. I hope it is correct now.

Comment 7

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8691869 [details] [diff] [review]
overlayflag.patch

Review of attachment 8691869 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,
thanks LGTM.

@Nick/mcomella,
super review please..

Comment 8

[Kalpesh Krishna [:martianwars]]

@vivek,
What can I work on next? Preferably something more challenging, like the highlight text bug.

Comment 9

[Kalpesh Krishna [:martianwars]]

^^

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8691869 [details] [diff] [review]
overlayflag.patch

Review of attachment 8691869 [details] [diff] [review]:
-----------------------------------------------------------------

One nit, one substantive change.  Marking as r- just so the r's balance out; it'll be clearer to release management that way.

::: mobile/android/base/moz.build
@@ +632,5 @@
>      gbjar.sources += [ 'CrashReporter.java' ]
>      ANDROID_RES_DIRS += [ 'crashreporter/res' ]
>  
> +gbjar.sources += [
> +    'overlays/OverlayConstants.java',

Let's fold this into the main sources list.

::: mobile/android/services/manifests/SyncAndroidManifest_activities.xml.in
@@ +77,1 @@
>          <activity

This was an #ifndef, so this block was never included, so we want to delete it entirely.  That leads to a cascade of other deletions: we can kill `SendTabActivity`, which lets us delete `ClientRecordArrayAdapter`.  Lets leave it at that, and pick up the other bits in https://bugzilla.mozilla.org/show_bug.cgi?id=957845 and its dependents.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Kalpesh Krishna [:martianwars] from comment #9)
> ^^

Kalpesh: I'd like to steer you towards a larger project.  We maintain a short list of such projects at https://bugzilla.mozilla.org/buglist.cgi?quicksearch=prod%3Aandroid%20sw%3A%22mentor%20project%22&list_id=12707349 but looking at that list, they don't seem like great fits for you.

I know there are a lot of open tickets around the Search Activity, which is likely to be an area of investment in 2016.  You might be interested in some of the dependent tickets listed at https://bugzilla.mozilla.org/showdependencytree.cgi?id=1017135&hide_resolved=1.

Comment 12

[Kalpesh Krishna [:martianwars]]

Attachment: overlayflag.patch

I hope it's okay now.

Comment 13

[Kalpesh Krishna [:martianwars]]

Hey Nick,
I would like to do a bigger project as well. I saw some of the projects on the list and liked the one Vivek is working on. However if you feel that none of them is an ideal fit for me, I'd work on the Search Activity. Do you have a [meta] bug for the same? Which ticket would be a good starting point?

It will be better if the whole project comes under a [meta] bug. I'll have a clearer image of the final goal in mind.

Comment 14

[Kalpesh Krishna [:martianwars]]

Nick, sorry I didn't see the second link so ignore my earlier comment. Which would be a good starting point?

Comment 15

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a39eb15b6fd41717dc962d1f66cec6d2220e26c8
Bug 1148028 - Remove MOZ_ANDROID_SHARE_OVERLAY. r=vivek,nalexander

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8692889 [details] [diff] [review]
overlayflag.patch

Review of attachment 8692889 [details] [diff] [review]:
-----------------------------------------------------------------

Great work!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a39eb15b6fd4