Closed Bug 1149780 Opened 9 years ago Closed 8 years ago

Enable code block in GeckoLoader that depends on SDK 17

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox40 affected, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox40 --- affected
firefox46 --- fixed

People

(Reporter: jchen, Assigned: raunaq.abhyankar, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 2 obsolete files)

The code block at [1] depends on SDK 17, which we have, so we should uncomment that block and remove the block below that uses reflection.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/mozglue/GeckoLoader.java?rev=48fc328377d4#163
Hey , i want to work on this please guide me about the reflection block that has to be removed
Hi, Aqib! First of all, you'll want to set up a Firefox for Android build environment. You can find instructions for that here: https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you want to look at the file mobile/android/base/mozglue/GeckoLoader.java, line 163. You want to uncomment the commented block. Then, you want to delete the try{} block below that has the reflection code. Finally, you want to rebuild Fennec with your changes to make sure everything still works.

Please join us in #mobile on irc.mozilla.org to ask questions if you run into any issues. Also feel free to say Hi to introduce yourself!
Not quite. Build.VERSION.SDK_INT applies to the device we're running on, not the SDK we're using to build.

If you delete the second code block, we'll only run on Jelly Bean MR1 and higher devices; we'll crash on the others.

If the commented-out code block is dramatically more efficient than what we run now, great; use a version conditional to choose which code to run, but the current code has to stick around to support older devices.

If it's not more efficient, then don't bother doing this work.
Well, the intent is to only run on 17+. Why will we crash on the others? AFAICT, we couldn't use | Build.VERSION.SDK_INT >= 17 | before because it was being optimized out, since the SDK version was lower than 17 (regardless of the actual version on the device). So instead, we used reflection. Now that our SDK version is higher than 17, we should no longer use reflection (and possibly switch to Versions.feature17Plus).

I think this bug makes for a good first bug, because it's simple to fix (but not meaningless), similar to other cleanup/refactor bugs that we use as good first bugs.
Hi, is this bug still open or were the changes deemed inefficient?
Attached patch bug-1149780-fix.patch (obsolete) — Splinter Review
Uncommented code block that depends on SDK 17 & deleted try-catch block that uses reflection.
Attachment #8706480 - Flags: review?(nchen)
Comment on attachment 8706480 [details] [diff] [review]
bug-1149780-fix.patch

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

Thank you for the patch! Could you make these changes below?

::: mobile/android/base/java/org/mozilla/gecko/mozglue/GeckoLoader.java
@@ +162,5 @@
>          f = context.getCacheDir();
>          putenv("CACHE_DIRECTORY=" + f.getPath());
>  
>          /* We really want to use this code, but it requires bumping up the SDK to 17 so for now
> +           we will use reflection. See https://bugzilla.mozilla.org/show_bug.cgi?id=811763#c11*/

You can delete this comment now

@@ +167,2 @@
>  
>          if (Build.VERSION.SDK_INT >= 17) {

You should change | Build.VERSION.SDK_INT >= 17 | to | Versions.feature17Plus |

@@ +172,5 @@
>              } else {
>                  Log.d(LOGTAG, "Unable to obtain user manager service on a device with SDK version " + Build.VERSION.SDK_INT);
>              }
>          }
> +        

Remove extra whitespace
Attachment #8706480 - Flags: review?(nchen) → feedback+
Attached patch bug-1149780-fix.patch (obsolete) — Splinter Review
Removed commented part and changed if statement. Also removed whitespace
Attachment #8706894 - Flags: review?(nchen)
Attachment #8706480 - Attachment is obsolete: true
Comment on attachment 8706894 [details] [diff] [review]
bug-1149780-fix.patch

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

Thanks!
Attachment #8706894 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> Comment on attachment 8706894 [details] [diff] [review]
> bug-1149780-fix.patch
> 
> Review of attachment 8706894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!

Is the patch correct? Or are there any more changes to be made?
Yep, it's ready to land. Thanks!

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6233777a63f
Assignee: nobody → raunaq.abhyankar
Status: NEW → ASSIGNED
Keywords: checkin-needed
Needs rebasing.

patching file mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java
Hunk #1 FAILED at 1294
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java.rej
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch bug1149780.patchSplinter Review
For check-in
Attachment #8708452 - Flags: review+
Attachment #8708452 - Flags: checkin?
Attachment #8706894 - Attachment is obsolete: true
Attachment #8708452 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/cf8d15f19bf8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.