Closed
Bug 1256922
Opened 7 years ago
Closed 7 years ago
Remove unused HardwareUtils.isLowMemoryPlatform()
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
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)
2.43 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 2•7 years ago
|
||
(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. !
Reporter | ||
Comment 4•7 years ago
|
||
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 ?
Reporter | ||
Comment 6•7 years ago
|
||
Attach the file to this bug. There should be a button at the top of this page.
attachment for bug 1256922
Assignee | ||
Comment 10•7 years ago
|
||
please review the new patch ! Thank you
Reporter | ||
Updated•7 years ago
|
Attachment #8731208 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → bouzroudzeineb
Status: NEW → ASSIGNED
Reporter | ||
Comment 11•7 years ago
|
||
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•7 years ago
|
||
ok! I'll do another patch
Assignee | ||
Comment 13•7 years ago
|
||
attachment for bug1256922
Reporter | ||
Comment 14•7 years ago
|
||
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)
Reporter | ||
Updated•7 years ago
|
Attachment #8731233 -
Attachment is obsolete: true
Attachment #8731323 -
Flags: review?(s.kaspari)
Attachment #8731323 -
Flags: review+
Reporter | ||
Updated•7 years ago
|
Attachment #8731323 -
Flags: review?(s.kaspari)
Attachment #8731323 -
Flags: review+
Reporter | ||
Comment 15•7 years ago
|
||
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•7 years ago
|
||
new patch for bug1256922
Attachment #8731323 -
Attachment is obsolete: true
Attachment #8735803 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 17•7 years ago
|
||
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•7 years ago
|
||
new attachment for bug 1256922
Attachment #8735803 -
Attachment is obsolete: true
Attachment #8735812 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 19•7 years ago
|
||
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+
Reporter | ||
Comment 20•7 years ago
|
||
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•7 years ago
|
||
yes
Assignee | ||
Comment 22•7 years ago
|
||
what should I do?
Assignee | ||
Comment 23•7 years ago
|
||
I understand what is the problem, normally this new patch is good
Attachment #8736297 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 24•7 years ago
|
||
Attachment #8735812 -
Attachment is obsolete: true
Attachment #8736297 -
Attachment is obsolete: true
Attachment #8736297 -
Flags: review?(s.kaspari)
Attachment #8737202 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 25•7 years ago
|
||
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•7 years ago
|
||
new patch
Attachment #8737202 -
Attachment is obsolete: true
Attachment #8737736 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 27•7 years ago
|
||
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•7 years ago
|
||
sorry ! forgot some comments effectively
Attachment #8737736 -
Attachment is obsolete: true
Attachment #8740376 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 29•7 years ago
|
||
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•7 years ago
|
||
new patch!
Attachment #8740376 -
Attachment is obsolete: true
Attachment #8740398 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 31•7 years ago
|
||
new patch
Attachment #8740398 -
Attachment is obsolete: true
Attachment #8740398 -
Flags: review?(s.kaspari)
Attachment #8740399 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 32•7 years ago
|
||
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+
Reporter | ||
Comment 33•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ef8a688b8b72f266782ed728071dd82dca1acf76 Bug 1256922 - Remove unused HardwareUtils.isLowMemoryPlatform(). r=sebastian
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ef8a688b8b72
Status: ASSIGNED → RESOLVED
Closed: 7 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
•