Closed Bug 1245044 Opened 4 years ago Closed 4 years ago

Move Restrictions class to restrictions package

Categories

(Firefox for Android Graveyard :: Family Friendly Browsing, defect)

All
Android
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: malayaleecoder, Mentored)

References

Details

(Whiteboard: [lang=java][lang=C++][good next bug])

Attachments

(1 file, 2 obsolete files)

The Restrictions is our entry point to check whether the normal/guest/restricted profile is allowed to use certain features:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Restrictions.java

We want to move this class from the root to the "restrictions" package where the other code is located:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/restrictions

Note that this class is an entry point for JNI calls (Methods annotated @WrapForJNI). After moving the class mach will update the GeneratedJNIWrappers.cpp:
https://dxr.mozilla.org/mozilla-central/source/widget/android/GeneratedJNIWrappers.cpp
This may or may not require you to update some c++ code using those methods.

For testing, this class lists features that can be restricted:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/parentalcontrols/nsIParentalControlsService.idl
Hi Sebastian.
   I would like to work on this bug. I will read through and upload the patch. Please let me know if I can work on this bug.

Thanks
Flags: needinfo?(s.kaspari)
(In reply to shatur from comment #1)
> Hi Sebastian.
>    I would like to work on this bug. I will read through and upload the
> patch. Please let me know if I can work on this bug.

Yeah, you can work on this bug! Just attach a patch and flag me for review as soon as it's ready. :)
Flags: needinfo?(s.kaspari)
Attached patch Bug1245044_v1.diff (obsolete) — Splinter Review
Hello Sebastian!

I have moved the Restriction.java to restriction folder and have attached an initial patch for this bug. On running "./mach build" after committing the above changes, I did not find GeneratedJNIWrappers.cpp to be changed. Do tell me if I made any mistake and please guide me for further improvements.

Cheers!
Flags: needinfo?(s.kaspari)
Attachment #8715184 - Flags: review?(s.kaspari)
Assignee: nobody → malayaleecoder
Flags: needinfo?(s.kaspari)
(In reply to malayaleecoder from comment #3)
> I have moved the Restriction.java to restriction folder and have attached an
> initial patch for this bug. On running "./mach build" after committing the
> above changes, I did not find GeneratedJNIWrappers.cpp to be changed. Do
> tell me if I made any mistake and please guide me for further improvements.

This is interesting. I pushed the patch to try to see if the tests still pass:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec8ef87d63db
The builds are failing:
> gmake[5]: *** No rule to make target `java/org/mozilla/gecko/Restrictions.java', needed by `gecko-browser.jar'. Stop.

You'll need to update this moz.build file:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#502
Attachment #8715184 - Flags: review?(s.kaspari)
Hello Sebastian,

I did change moz.build and the build ran successfully. But still did not see any changes in GeneratedJNIWrappers.cpp. Should I attach the patch?

Cheers!
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Yeah, go ahead, I'll push it to try again and then let's see what the tests say. :)
Flags: needinfo?(s.kaspari)
Attached patch Bug1245044_v2.diff (obsolete) — Splinter Review
Please have a look.

Thanks!
Attachment #8715184 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Are you running |mach build| locally to confirm that the build is working?

Because even with the new patch I see errors when building locally.
Flags: needinfo?(s.kaspari)
Yes, I am. Is that the wrong way?
I am having a hard time pushing to the try server from my system.
Please guide me on what I have to do subsequently.
Thanks.
Flags: needinfo?(s.kaspari)
(In reply to malayaleecoder from comment #11)
> Yes, I am. Is that the wrong way?

No, that's correct. However with the current patch applied this should not build. Are you sure you have those changes applied when building?
Flags: needinfo?(s.kaspari)
@Sebastian : I am very sure that the changes have been applied. After reading your comment, I ran ./mach build and it built the entire code successfully. What to do?
Flags: needinfo?(s.kaspari)
Hm,

* Can you share your mozconfig file? https://pastebin.mozilla.org/

* Does |mach package| and |mach install| create and install an application on your phone?
Flags: needinfo?(s.kaspari)
Sebastian, please give me some more time, I think I have got a few missing files and configurations in by directory. Will fix it and ping you.

Thank You.
Flags: needinfo?(s.kaspari)
Sure, let me know if you need any further help or just ask the friendly folks in #mobile (https://wiki.mozilla.org/IRC)
Flags: needinfo?(s.kaspari)
Did you have any success?
Flags: needinfo?(malayaleecoder)
Hello Sebastian,

I had messaged you in IRC, think you did not see that, I am currently having my examinations going on for two more days. Will fix it for sure after that. Hope it is not a problem.

Cheers.
Flags: needinfo?(malayaleecoder) → needinfo?(s.kaspari)
Sure, no problem.
Flags: needinfo?(s.kaspari)
Push to try please :)
Attachment #8718015 - Attachment is obsolete: true
Sebastian, Hope it's fine.
Adding checkin-needed :)
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
(In reply to malayaleecoder from comment #22)
> Sebastian, Hope it's fine.
> Adding checkin-needed :)

Yep, thanks! :)
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/779bebe283f8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1255394
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.