Remove unused HardwareUtils.isLowMemoryPlatform()

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: bouzroudzeineb, Mentored)

Tracking

unspecified
Firefox 48
All
Android
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(1 attachment, 10 obsolete attachments)

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
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8731208 [details] [diff] [review]
bug1256922.patch

attachment for bug 1256922
(Assignee)

Comment 8

3 years ago
sorry this patch is not valid
(Assignee)

Comment 9

3 years ago
Created attachment 8731233 [details] [diff] [review]
bug1256922.diff
(Assignee)

Comment 10

3 years ago
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. :)
(Assignee)

Comment 12

3 years ago
ok! I'll do another patch
(Assignee)

Comment 13

3 years ago
Created attachment 8731323 [details] [diff] [review]
bug1256922.diff

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
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 16

3 years ago
Created attachment 8735803 [details] [diff] [review]
patch.diff

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+
(Assignee)

Comment 18

3 years ago
Created attachment 8735812 [details] [diff] [review]
patch.diff

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?
(Assignee)

Comment 21

3 years ago
yes
(Assignee)

Comment 22

3 years ago
what should I do?
(Assignee)

Comment 23

3 years ago
Created attachment 8736297 [details] [diff] [review]
bug1256922.diff

 I understand what is the problem, normally this new patch is good
Attachment #8736297 - Flags: review?(s.kaspari)
(Assignee)

Comment 24

3 years ago
Created attachment 8737202 [details] [diff] [review]
bug1256922-0104.diff
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+
(Assignee)

Comment 26

3 years ago
Created attachment 8737736 [details] [diff] [review]
patchBug1256922.diff

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)
(Assignee)

Comment 28

3 years ago
Created attachment 8740376 [details] [diff] [review]
patch_bug1256922.diff

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)
(Assignee)

Comment 30

3 years ago
Created attachment 8740398 [details] [diff] [review]
mypatch_bug125622.diff

new patch!
Attachment #8740376 - Attachment is obsolete: true
Attachment #8740398 - Flags: review?(s.kaspari)
(Assignee)

Comment 31

3 years ago
Created attachment 8740399 [details] [diff] [review]
newpatch_bug1256922.diff

new patch
Attachment #8740398 - Attachment is obsolete: true
Attachment #8740398 - Flags: review?(s.kaspari)
(Assignee)

Updated

3 years ago
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+

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ef8a688b8b72
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.