Closed Bug 1255394 Opened 4 years ago Closed 4 years ago

Fix package of Restrictions class

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

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)

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
Hello, I will try my best to resolve this bug today !
(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. :)
Attached file Bug_1255394.zip (obsolete) —
Add : 
+ .restrictions to the package
+ docs on all methods
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
Hi Guillaume. Are you still interested on working on this bug?
Flags: needinfo?(guillaume.heras)
Attached patch a fix for the bug 1255394 (obsolete) — Splinter Review
Hi Sebastian!
I've created a quick fix for this bug, please check it :) !
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-
Attachment #8729543 - Attachment is obsolete: true
Attached patch Quick working patch ! (obsolete) — Splinter Review
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)
(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;
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+
(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.
Assignee: nobody → aallam.mouaad
Status: NEW → ASSIGNED
(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,
Attachment #8733403 - Flags: a11y-review+ → feedback+
(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.
(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
(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. :)
Attached patch Patch respecting imports order (obsolete) — Splinter Review
Attachment #8731653 - Attachment is obsolete: true
Attachment #8733403 - Attachment is obsolete: true
Attachment #8733591 - Flags: review?(s.kaspari)
(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
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)
Flags: needinfo?(s.kaspari) → needinfo?(Aallam.Mouaad)
(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)
(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 ?
(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 ?
Please test if everything's still working with the new generated code and then upload a patch with all changes. :)
Attached patch Fixes+JNI (obsolete) — Splinter Review
Attachment #8734063 - Flags: review?(s.kaspari)
(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 !
(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;
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. :)
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
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)
Attachment #8733591 - Attachment is obsolete: true
Attachment #8733591 - Flags: review?(s.kaspari)
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+
(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 ?
(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)
(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. :)
Attachment #8734063 - Flags: review?(s.kaspari) → review+
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
(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. :)
(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.
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
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.
Flags: needinfo?(guillaume.heras)
(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)
(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 :)
(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>`.
(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
Attachment #8736268 - Flags: review?(s.kaspari) → review+
NI to myself to land this once tree is open again.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/8afd00a026c4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.