Closed
Bug 1255394
Opened 8 years ago
Closed 8 years ago
Fix package of Restrictions class
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: sebastian, Assigned: Aallam.Mouaad, Mentored)
References
Details
(Whiteboard: [lang=java][good first bug])
Attachments
(1 file, 5 obsolete files)
68.16 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
In bug 1245044 we moved the Restrictions class from org.mozilla.gecko to org.mozilla.gecko.restrictions. But we did not update the package (It's still org.mozilla.gecko). Magically everything still works but Android Studio thinks the class is not imported. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/restrictions/Restrictions.java * Set the package to org.mozilla.gecko.restrictions * Fix all imports of the class To start, set up a build environment - you can see the instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build Then, you'll need to create a patch to upload - see https://wiki.mozilla.org/Mobile/Fennec/Android#Creating_commits_and_submitting_patches
Comment 1•8 years ago
|
||
Hello, I will try my best to resolve this bug today !
Reporter | ||
Comment 2•8 years ago
|
||
(In reply to guillaume.heras from comment #1) > Hello, I will try my best to resolve this bug today ! Okay. As soon as you attach your first patch I'll assign the bug to you. :)
Comment 3•8 years ago
|
||
Add : + .restrictions to the package + docs on all methods
Reporter | ||
Comment 4•8 years ago
|
||
Hi Guillaume! * Please use the "review" flag on the attachment so that the reviewer see's that you uploaded a patch for review * You uploaded a ZIP but you'll need to create a patch, see here: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 5•8 years ago
|
||
Hi Guillaume. Are you still interested on working on this bug?
Flags: needinfo?(guillaume.heras)
Assignee | ||
Comment 6•8 years ago
|
||
Hi Sebastian! I've created a quick fix for this bug, please check it :) !
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8731653 [details] [diff] [review] a fix for the bug 1255394 Review of attachment 8731653 [details] [diff] [review]: ----------------------------------------------------------------- Hi Mouaad. Please set the "review?" flag on the attachment if you are looking for someone to review your patch. Otherwise it's easy to miss. :) Did you build the code after your changes? This should not create a successful build. ::: mobile/android/base/java/org/mozilla/gecko/restrictions/Restrictions.java @@ +17,5 @@ > +import org.mozilla.gecko.Restrictions.restrictions.GuestProfileConfiguration; > +import org.mozilla.gecko.Restrictions.restrictions.Restrictable; > +import org.mozilla.gecko.Restrictions.restrictions.RestrictedProfileConfiguration; > +import org.mozilla.gecko.Restrictions.restrictions.RestrictionCache; > +import org.mozilla.gecko.Restrictions.restrictions.RestrictionConfiguration; The classes did not move and their package did not change. Instead all imports of the Restrictions class (in other classes) need to be updated.
Attachment #8731653 -
Flags: review-
Reporter | ||
Updated•8 years ago
|
Attachment #8729543 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Hello Sebastian, Oops my bad! That's definitely not the correct patch i wanted to upload ! please review this one :) . PS: Built, packaged, installed and ran the app after the changes (Android 6.0).
Attachment #8733403 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Mouaad from comment #8) > Created attachment 8733403 [details] [diff] [review] A thing i just noticed: is the order of the imports important ? example: is >import org.mozilla.gecko.restrictions.Restrictions; >import org.mozilla.gecko.restrictions.Restrictable; equals to >import org.mozilla.gecko.restrictions.Restrictable; >import org.mozilla.gecko.restrictions.Restrictions;
Reporter | ||
Comment 10•8 years ago
|
||
Comment on attachment 8733403 [details] [diff] [review] Quick working patch ! Review of attachment 8733403 [details] [diff] [review]: ----------------------------------------------------------------- This looks better but it seems like this patch does not contain the previous changes? Can you fold all the changes into one patch?
Attachment #8733403 -
Flags: review?(s.kaspari) → a11y-review+
Reporter | ||
Comment 11•8 years ago
|
||
(In reply to Mouaad from comment #9) > A thing i just noticed: is the order of the imports important ? Yeah, we try to alphabetize the imports. Just so that they won't be reordered by tools and IDEs from patch to patch.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → aallam.mouaad
Status: NEW → ASSIGNED
Comment 12•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #10) > This looks better but it seems like this patch does not contain the previous > changes? Can you fold all the changes into one patch? Hello again ! :) I will create another patch with the correct alphabetical order, meanwhile i didn't understand the part about the previous changes, which changes ? the ones in my first patch ? Regards,
Reporter | ||
Updated•8 years ago
|
Attachment #8733403 -
Flags: a11y-review+ → feedback+
Reporter | ||
Comment 13•8 years ago
|
||
(In reply to Mouaad from comment #12) > I will create another patch with the correct alphabetical order, meanwhile i > didn't understand the part about the previous changes, which changes ? the > ones in my first patch ? > Regards, Your patch from today doesn't include the package change in the Restrictions class.
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #13) > Your patch from today doesn't include the package change in the Restrictions > class. Do you mean this changes ? https://bugzilla.mozilla.org/attachment.cgi?id=8733403&action=diff#a/mobile/android/base/java/org/mozilla/gecko/restrictions/Restrictions.java_sec2
Reporter | ||
Comment 15•8 years ago
|
||
(In reply to Mouaad from comment #14) > (In reply to Sebastian Kaspari (:sebastian) from comment #13) > > Your patch from today doesn't include the package change in the Restrictions > > class. > Do you mean this changes ? > https://bugzilla.mozilla.org/attachment.cgi?id=8733403&action=diff#a/mobile/ > android/base/java/org/mozilla/gecko/restrictions/Restrictions.java_sec2 Oh, wow, I didn't see it this morning. Then everything's good after you upload your new patch I guess. :)
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8731653 -
Attachment is obsolete: true
Attachment #8733403 -
Attachment is obsolete: true
Attachment #8733591 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #15) > Oh, wow, I didn't see it this morning. Then everything's good after you > upload your new patch I guess. :) I added new patch :D i hope this one is perfect ! :D
Reporter | ||
Comment 18•8 years ago
|
||
Building the patch with |mach| I see that we'll need to update the generated JNI wrappers. Mouaad: Do you want to take care of that? If you build with mach then it should guide you through the process.
Flags: needinfo?(s.kaspari)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(s.kaspari) → needinfo?(Aallam.Mouaad)
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #18) > Building the patch with |mach| I see that we'll need to update the generated > JNI wrappers. > > Mouaad: Do you want to take care of that? If you build with mach then it > should guide you through the process. Yes of course, just guide me how to do it and i will take care of it ! :)
Flags: needinfo?(Aallam.Mouaad)
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #18) > Building the patch with |mach| I see that we'll need to update the generated > JNI wrappers. > > Mouaad: Do you want to take care of that? If you build with mach then it > should guide you through the process. Forgot to mention that i actually built/packaged/installed the changes i have done with mach ! maybe i missed something in the process ?
Assignee | ||
Comment 21•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #18) > Building the patch with |mach| I see that we'll need to update the generated > JNI wrappers. Oh! i just recompiled and got the error about JNI Code ! i run : > make -C objdir-frontend/mobile/android/base update-generated-wrappers And all has been compiled successfully What should i upload ?
Reporter | ||
Comment 22•8 years ago
|
||
Please test if everything's still working with the new generated code and then upload a patch with all changes. :)
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8734063 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #22) > Please test if everything's still working with the new generated code and > then upload a patch with all changes. :) I noticed that for me the app crashes after few moments from launching !
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #22) > Please test if everything's still working with the new generated code and > then upload a patch with all changes. :) I've tried to build and run using Android Studio, i got the following error: >java.lang.NoClassDefFoundError: Failed resolution of: Lcom/squareup/leakcanary/LeakCanary; >at org.mozilla.gecko.GeckoApplication.onCreate(GeckoApplication.java:147) Meanwhile the class is correctly imported: >import com.squareup.leakcanary.LeakCanary;
Reporter | ||
Comment 26•8 years ago
|
||
This seems to be independent from the change here. We have seen this before: bug 1255580. Do you see this only for gradle builds or for mach-based builds too? Does gradle clean or mach clobber help? Maybe answer those questions over in bug 1255580. :)
Reporter | ||
Comment 27•8 years ago
|
||
Oh, and that's important: This now changes the c++ code and this requires you to do a full (back-end) build. But this will require quite some time depending on your hardware: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build#I_want_to_work_on_the_back-end
Reporter | ||
Comment 28•8 years ago
|
||
Comment on attachment 8734063 [details] [diff] [review] Fixes+JNI @Jim: I wonder why running the generator script generates so many changes in GeneratedJNIWrappers.cpp. We just moved one Java class. Is this correct?
Attachment #8734063 -
Flags: feedback?(nchen)
Reporter | ||
Updated•8 years ago
|
Attachment #8733591 -
Attachment is obsolete: true
Attachment #8733591 -
Flags: review?(s.kaspari)
Comment 29•8 years ago
|
||
Comment on attachment 8734063 [details] [diff] [review] Fixes+JNI Review of attachment 8734063 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Sebastian Kaspari (:sebastian) from comment #28) > Comment on attachment 8734063 [details] [diff] [review] > Fixes+JNI > > @Jim: I wonder why running the generator script generates so many changes in > GeneratedJNIWrappers.cpp. We just moved one Java class. Is this correct? Looks okay. I think it's just the diff tool being dumb.
Attachment #8734063 -
Flags: feedback?(nchen) → feedback+
Reporter | ||
Comment 30•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ed1d2e69d3
Assignee | ||
Comment 31•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #27) > Oh, and that's important: This now changes the c++ code and this requires > you to do a full (back-end) build. I am trying for 2 days now to compile backend (takes around 1 hour) but i get errors each time.. i will keep trying to even i don't find the reason of errors easly. (In reply to Sebastian Kaspari (:sebastian) from comment #30) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ed1d2e69d3 Looks like it's done ! is everything good ?
Assignee | ||
Comment 32•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #27) > Oh, and that's important: This now changes the c++ code and this requires > you to do a full (back-end) build. Hi, any idea how to resolve this error ? https://pastebin.mozilla.org/8865141 (Line 17)
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 33•8 years ago
|
||
(In reply to Mouaad from comment #31) > (In reply to Sebastian Kaspari (:sebastian) from comment #27) > > Oh, and that's important: This now changes the c++ code and this requires > > you to do a full (back-end) build. > > I am trying for 2 days now to compile backend (takes around 1 hour) but i > get errors each time.. i will keep trying to even i don't find the reason of > errors easly. > > (In reply to Sebastian Kaspari (:sebastian) from comment #30) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ed1d2e69d3 > > Looks like it's done ! is everything good ? Yeah, this looks good. Let's land this. :)
Reporter | ||
Updated•8 years ago
|
Attachment #8734063 -
Flags: review?(s.kaspari) → review+
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
Reporter | ||
Comment 34•8 years ago
|
||
(In reply to Mouaad from comment #32) > Hi, any idea how to resolve this error ? > https://pastebin.mozilla.org/8865141 (Line 17) I can't see what's going wrong here. But just come over to #mobile (irc.mozilla.org) and some of the helpful folks there will know. :)
Comment 35•8 years ago
|
||
(In reply to Mouaad from comment #32) > (In reply to Sebastian Kaspari (:sebastian) from comment #27) > > Oh, and that's important: This now changes the c++ code and this requires > > you to do a full (back-end) build. > > Hi, any idea how to resolve this error ? > https://pastebin.mozilla.org/8865141 (Line 17) What is in your mozconfig file? Try adding "ac_add_options --enable-optimize" line to your mozconfig, and delete any "ac_add_options --disable-optimize" lines.
Comment 36•8 years ago
|
||
renamed 1255394 -> Fixes+JNI.diff applying Fixes+JNI.diff patching file mobile/android/base/java/org/mozilla/gecko/BrowserApp.java Hunk #1 FAILED at 49 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/BrowserApp.java.rej patching file mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPager.java Hunk #1 FAILED at 11 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPager.java.rej patching file mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java Hunk #1 FAILED at 9 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java.rej patching file mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java Hunk #1 FAILED at 8 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java.rej patching file mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java Hunk #1 FAILED at 18 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Fixes+JNI.diff has problems to apply
Flags: needinfo?(Aallam.Mouaad)
Keywords: checkin-needed
Reporter | ||
Comment 37•8 years ago
|
||
Mouaad: Can you rebase your patch against the latest changes? Seems like some of the files have changed in the meantime and the patch doesn't apply cleanly anymore.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(guillaume.heras)
Assignee | ||
Comment 38•8 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #35) > What is in your mozconfig file? Try adding "ac_add_options > --enable-optimize" line to your mozconfig, and delete any "ac_add_options > --disable-optimize" lines. I have think i already have tried this, anyway i bypassed the error by using: > ac_add_options --disable-crashreporter And everything worked like a charm (even the compiling took so long, really so long).
Flags: needinfo?(Aallam.Mouaad)
Assignee | ||
Comment 39•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #37) > Mouaad: Can you rebase your patch against the latest changes? Seems like > some of the files have changed in the meantime and the patch doesn't apply > cleanly anymore. Alright i work on it :)
Assignee | ||
Comment 40•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #37) > Mouaad: Can you rebase your patch against the latest changes? After |hg pull| and while rebasing i got an error about only for two files have been changed ! Anyway here is the result :)
Attachment #8734063 -
Attachment is obsolete: true
Attachment #8736268 -
Flags: review?(s.kaspari)
Mouaad, to answer your question from irc, you only need the .apk file – you can install it via `adb install <path-to-apk>`.
Assignee | ||
Comment 42•8 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #41) > Mouaad, to answer your question from irc, you only need the .apk file – you > can install it via `adb install <path-to-apk>`. Yes, works like a charm ! :D thank you
Reporter | ||
Comment 43•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48af4c126335
Reporter | ||
Updated•8 years ago
|
Attachment #8736268 -
Flags: review?(s.kaspari) → review+
Reporter | ||
Comment 44•8 years ago
|
||
NI to myself to land this once tree is open again.
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 45•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8afd00a026c41cb6bfbad5a3df8b7fac7f0dcaec Bug 1255394 - Restrictions: Fix java package. r=sebastian
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(s.kaspari)
Comment 46•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8afd00a026c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
•