Enable code block in GeckoLoader that depends on SDK 17

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

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

Tracking

Trunk
Firefox 46
All
Android
Points:
---

Firefox Tracking Flags

(firefox40 affected, firefox46 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

4 years ago
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.

Comment 5

4 years ago
Hi, is this bug still open or were the changes deemed inefficient?
(Assignee)

Comment 6

3 years ago
Posted 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+
(Assignee)

Comment 8

3 years ago
Posted 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+
(Assignee)

Comment 10

3 years ago
(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
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
For check-in
Attachment #8708452 - Flags: review+
Attachment #8708452 - Flags: checkin?
Attachment #8706894 - Attachment is obsolete: true
Attachment #8708452 - Flags: checkin?

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf8d15f19bf8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.