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)
Tracking
(firefox40 affected, firefox46 fixed)
RESOLVED
FIXED
Firefox 46
People
(Reporter: jchen, Assigned: raunaq.abhyankar, Mentored)
References
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 2 obsolete files)
2.60 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Hey , i want to work on this please guide me about the reflection block that has to be removed
Reporter | ||
Comment 2•9 years ago
|
||
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!
Comment 3•9 years ago
|
||
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.
Reporter | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
Uncommented code block that depends on SDK 17 & deleted try-catch block that uses reflection.
Attachment #8706480 -
Flags: review?(nchen)
Reporter | ||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
Removed commented part and changed if statement. Also removed whitespace
Attachment #8706894 -
Flags: review?(nchen)
Reporter | ||
Updated•8 years ago
|
Attachment #8706480 -
Attachment is obsolete: true
Reporter | ||
Comment 9•8 years ago
|
||
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•8 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?
Reporter | ||
Comment 11•8 years ago
|
||
Yep, it's ready to land. Thanks! Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6233777a63f
Comment 12•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 13•8 years ago
|
||
For check-in
Attachment #8708452 -
Flags: review+
Attachment #8708452 -
Flags: checkin?
Updated•8 years ago
|
Attachment #8706894 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8708452 -
Flags: checkin?
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cf8d15f19bf8
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf8d15f19bf8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•