Closed Bug 1256922 Opened 4 years ago Closed 4 years ago

Remove unused HardwareUtils.isLowMemoryPlatform()

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: bouzroudzeineb, Mentored)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 10 obsolete files)

The isLowMemoryPlatform() method of the HardwareUtils class is unused.

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

To fix this bug, you need to remove this method:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java#89-100

At least LOW_MEMORY_THRESHOLD_MB will be unused after that. Remove it too.

Then, you'll need to create a patch to upload - see
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
I'd really like to fix this bug, It's will my first bug
(In reply to bzrd_Sdn from comment #1)
> I'd really like to fix this bug, It's will my first bug

Great! Let me know if you need any help. As soon as you'll upload your first patch I'll assign the bug to you.
I have a problem to push the patch,I don't understand this section 
"To push to mozilla-central and other mozilla-hosted repositories you need committer access, and you must edit the file (your-local-hg-root)/.hg/hgrc (note, this is NOT your ~/.hgrc)" 
thank you. !
You can't push to mozilla-central without permission. Instead you'll need to create a patch file with "hg export" and upload it to this bug.
after having done this command hg export . > bug1256922.diff
what should I do ?
Attach the file to this bug. There should be a button at the top of this page.
Attached patch bug1256922.patch (obsolete) — Splinter Review
attachment for bug 1256922
sorry this patch is not valid
Attached patch bug1256922.diff (obsolete) — Splinter Review
please review the new  patch !
Thank you
Attachment #8731208 - Attachment is obsolete: true
Assignee: nobody → bouzroudzeineb
Status: NEW → ASSIGNED
Comment on attachment 8731233 [details] [diff] [review]
bug1256922.diff

Review of attachment 8731233 [details] [diff] [review]:
-----------------------------------------------------------------

Just use the "review?" flag on the attachment if you want the patch to be reviewed. :)

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ +23,5 @@
>      // has be in sync with Gecko's equivalent threshold (defined in
>      // xpcom/base/nsMemoryImpl.cpp) and should only be used in cases
>      // where we can't depend on Gecko to be up and running e.g. show/hide
>      // reading list capabilities in HomePager.
> +    

NIT: There are some empty spaces here.

@@ +85,5 @@
>      public static int getMemSize() {
>          return SysInfo.getMemSize();
>      }
>  
> +   

NIT: Here too.

::: testing/mozharness/test/hgrc
@@ +2,5 @@
> +username = bzrd_sdn <bouzroudzeineb@gmail.com>
> +
> +[defaults]
> +qnew = -Ue
> +

This should go into your local .hrc file and should not be in the tree or part of your patch. :)
ok! I'll do another patch
Attached patch bug1256922.diff (obsolete) — Splinter Review
attachment for bug1256922
Comment on attachment 8731323 [details] [diff] [review]
bug1256922.diff

Oh, I did miss this patch. Just set the "review" flag and I'll see it. :)
Attachment #8731323 - Flags: review?(s.kaspari)
Attachment #8731233 - Attachment is obsolete: true
Attachment #8731323 - Flags: review?(s.kaspari)
Attachment #8731323 - Flags: review+
Attachment #8731323 - Flags: review?(s.kaspari)
Attachment #8731323 - Flags: review+
Comment on attachment 8731323 [details] [diff] [review]
bug1256922.diff

Review of attachment 8731323 [details] [diff] [review]:
-----------------------------------------------------------------

The change itself looks good. But this patch still touches testing/mozharness/test/hgrc (even though there are no "visible" changed).
Attachment #8731323 - Flags: review?(s.kaspari) → feedback+
Attached patch patch.diff (obsolete) — Splinter Review
new patch for bug1256922
Attachment #8731323 - Attachment is obsolete: true
Attachment #8735803 - Flags: review?(s.kaspari)
Comment on attachment 8735803 [details] [diff] [review]
patch.diff

Review of attachment 8735803 [details] [diff] [review]:
-----------------------------------------------------------------

Almost! :-)

The hgrc is now gone but there are some new trailing whitespaces.

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ +23,5 @@
>      // has be in sync with Gecko's equivalent threshold (defined in
>      // xpcom/base/nsMemoryImpl.cpp) and should only be used in cases
>      // where we can't depend on Gecko to be up and running e.g. show/hide
>      // reading list capabilities in HomePager.
> +    

NIT: Trailing whitespaces

@@ +85,5 @@
>      public static int getMemSize() {
>          return SysInfo.getMemSize();
>      }
>  
> +   

NIT: Trailing whitespaces
Attachment #8735803 - Flags: review?(s.kaspari) → feedback+
Attached patch patch.diff (obsolete) — Splinter Review
new attachment for bug 1256922
Attachment #8735803 - Attachment is obsolete: true
Attachment #8735812 - Flags: review?(s.kaspari)
Comment on attachment 8735812 [details] [diff] [review]
patch.diff

Review of attachment 8735812 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thank you! :)
Attachment #8735812 - Flags: review?(s.kaspari) → review+
I tried to import this patch in order to push it to try and the repo but it seems to be a broken patch somehow:

> TheSilence:fx-team sebastian$ hg import 'https://bug1256922.bmoattachments.org/attachment.cgi?id=8735812'
> applying https://bug1256922.bmoattachments.org/attachment.cgi?id=8735812
> abort: bad hunk #1 @@ -19,17 +19,17 @@ public final class HardwareUtils {
> (18 17 17 17)

Is this a patch exported from mercurial?
yes
what should I do?
Attached patch bug1256922.diff (obsolete) — Splinter Review
I understand what is the problem, normally this new patch is good
Attachment #8736297 - Flags: review?(s.kaspari)
Attached patch bug1256922-0104.diff (obsolete) — Splinter Review
Attachment #8735812 - Attachment is obsolete: true
Attachment #8736297 - Attachment is obsolete: true
Attachment #8736297 - Flags: review?(s.kaspari)
Attachment #8737202 - Flags: review?(s.kaspari)
Comment on attachment 8737202 [details] [diff] [review]
bug1256922-0104.diff

Review of attachment 8737202 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ -23,5 @@
>      // has be in sync with Gecko's equivalent threshold (defined in
>      // xpcom/base/nsMemoryImpl.cpp) and should only be used in cases
>      // where we can't depend on Gecko to be up and running e.g. show/hide
>      // reading list capabilities in HomePager.
> -    private static final int LOW_MEMORY_THRESHOLD_MB = 384;

You should remove the comments too.
Attachment #8737202 - Flags: review?(s.kaspari) → feedback+
Attached patch patchBug1256922.diff (obsolete) — Splinter Review
new patch
Attachment #8737202 - Attachment is obsolete: true
Attachment #8737736 - Flags: review?(s.kaspari)
Comment on attachment 8737736 [details] [diff] [review]
patchBug1256922.diff

Review of attachment 8737736 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ +19,4 @@
>      private static final String LOGTAG = "GeckoHardwareUtils";
>  
>      // Minimum memory threshold for a device to be considered
>      // a low memory platform (see isLowMemoryPlatform). This value

Is this the correct patch? The comment is still there.
Attachment #8737736 - Flags: review?(s.kaspari)
Attached patch patch_bug1256922.diff (obsolete) — Splinter Review
sorry ! forgot some comments effectively
Attachment #8737736 - Attachment is obsolete: true
Attachment #8740376 - Flags: review?(s.kaspari)
Comment on attachment 8740376 [details] [diff] [review]
patch_bug1256922.diff

Review of attachment 8740376 [details] [diff] [review]:
-----------------------------------------------------------------

Did you remove the comments from the patch?

You'll need to remove the comments from the code and then update your commit and create a new patch.

The file you uploaded can't be applied:
> TheSilence:fx-team sebastian$ hg import 'https://bug1256922.bmoattachments.org/attachment.cgi?id=8740376'
> applying https://bug1256922.bmoattachments.org/attachment.cgi?id=8740376
> patching file mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
> Hunk #1 FAILED at 18
> 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java.rej
> abort: patch failed to apply

Also: I just saw that the patch doesn't contain a commit message and author line. Is this a patch exported from Mercurial?
Attachment #8740376 - Flags: review?(s.kaspari)
Attached patch mypatch_bug125622.diff (obsolete) — Splinter Review
new patch!
Attachment #8740376 - Attachment is obsolete: true
Attachment #8740398 - Flags: review?(s.kaspari)
new patch
Attachment #8740398 - Attachment is obsolete: true
Attachment #8740398 - Flags: review?(s.kaspari)
Attachment #8740399 - Flags: review?(s.kaspari)
Comment on attachment 8740399 [details] [diff] [review]
newpatch_bug1256922.diff

Review of attachment 8740399 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8740399 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/ef8a688b8b72
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.