Closed Bug 1148028 Opened 5 years ago Closed 4 years ago

Remove MOZ_ANDROID_SHARE_OVERLAY #define

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: mcomella, Assigned: martianwars, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Since we're shipping the share overlay, this isn't useful anymore.
@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
Flags: needinfo?(kalpeshk2011)
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?
Flags: needinfo?(kalpeshk2011) → needinfo?(vivekb.balakrishnan)
Kalpesh,
Yes it is similar :)
Thanks for working on this
Flags: needinfo?(vivekb.balakrishnan)
Assignee: nobody → kalpeshk2011
Attached patch overlayfix.patch (obsolete) — Splinter Review
I hope it is okay. Could you tell me the tree command to test it?
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8690581 - Flags: review?(vivekb.balakrishnan)
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.
Attachment #8690581 - Flags: review?(vivekb.balakrishnan) → review-
Attached patch overlayflag.patch (obsolete) — Splinter Review
Hey I reworked the entire patch. I hope it is correct now.
Attachment #8690581 - Attachment is obsolete: true
Attachment #8691869 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8691869 [details] [diff] [review]
overlayflag.patch

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

Kalpesh,
thanks LGTM.

@Nick/mcomella,
super review please..
Attachment #8691869 - Flags: review?(vivekb.balakrishnan)
Attachment #8691869 - Flags: review?(nalexander)
Attachment #8691869 - Flags: review?(michael.l.comella)
Attachment #8691869 - Flags: review+
@vivek,
What can I work on next? Preferably something more challenging, like the highlight text bug.
Flags: needinfo?(vivekb.balakrishnan)
^^
Flags: needinfo?(vivekb.balakrishnan)
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.
Attachment #8691869 - Flags: review?(nalexander)
Attachment #8691869 - Flags: review?(michael.l.comella)
Attachment #8691869 - Flags: review-
(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.
I hope it's okay now.
Attachment #8691869 - Attachment is obsolete: true
Attachment #8692889 - Flags: review?(nalexander)
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.
Flags: needinfo?(nalexander)
Nick, sorry I didn't see the second link so ignore my earlier comment. Which would be a good starting point?
Comment on attachment 8692889 [details] [diff] [review]
overlayflag.patch

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

Great work!
Attachment #8692889 - Flags: review?(nalexander) → review+
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/a39eb15b6fd4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: needinfo?(vivekb.balakrishnan)
You need to log in before you can comment on or make changes to this bug.