Closed
Bug 1266199
Opened 8 years ago
Closed 8 years ago
Rename BrowserApp.searchEngineManager -> mSearchEngineManager
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: mcomella, Assigned: bouzroudzeineb, Mentored)
Details
(Whiteboard: [lang=java][good first bug])
Attachments
(1 file, 4 obsolete files)
3.98 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
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/Suggested_workflow#Creating_commits_and_submitting_patches You can find more miscellaneous information at: https://wiki.mozilla.org/Mobile/Fennec/Android To fix this bug, rename the searchEngineManager variable in the BrowserApp class to mSearchEngineManager since it doesn't match the naming conventions in that file. If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC Thanks and happy coding! ^_^
patch for the bug 1266199
Attachment #8743839 -
Flags: review?(michael.l.comella)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → bouzroudzeineb
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8743839 [details] [diff] [review] patch.diff Review of attachment 8743839 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java @@ +293,5 @@ > private final DynamicToolbar mDynamicToolbar = new DynamicToolbar(); > private final ScreenshotObserver mScreenshotObserver = new ScreenshotObserver(); > > @NonNull > + private SearchEngineManager mSearchEngineManager; // Contains reference to Context - DO NOT LEAK! You'll have to update where this is used too. It's best if you use an IDE for renames as it will take care of those changes for you. ::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java @@ -85,5 @@ > public static int getMemSize() { > return SysInfo.getMemSize(); > } > > - public static boolean isLowMemoryPlatform() { This change is unrelated, please remove it from the patch. Also, this method isn't present in my local revision of the code – make sure you have the latest changes from upstream.
Attachment #8743839 -
Flags: review?(michael.l.comella) → feedback+
new patch for the bug 1266199 normally everything is ok ! Thanks you!
Attachment #8743839 -
Attachment is obsolete: true
Attachment #8744585 -
Flags: review?(michael.l.comella)
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 8744585 [details] [diff] [review] newpatch.diff Review of attachment 8744585 [details] [diff] [review]: ----------------------------------------------------------------- Great! This looks good to me! Unfortunately though, your patch doesn't build because the code underneath changed – can you pull the latest changes from fx-team and rebase your patch? I fixed the patch locally and made a push to our try test servers (above). Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8744585 -
Flags: review?(michael.l.comella) → review+
Reporter | ||
Comment 5•8 years ago
|
||
Here's the try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53e928722a1c also, your patch does not appear to have the correct header – can you make sure you have a comment summary and your login?
new patch
Attachment #8744585 -
Attachment is obsolete: true
Attachment #8747272 -
Flags: review?(michael.l.comella)
Attachment #8747272 -
Flags: checkin?(michael.l.comella)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8747272 [details] [diff] [review] patch_bug1266199.diff Review of attachment 8747272 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately, this patch still did not apply locally – did you `hg qpop && hg pull && hg up && hg qps`, then fix your changes, and `hg qref` them? Note that `hg qref` will overwrite your previous changes so be sure you merged correctly! If you need a backup of your old patch, it should be available on bugzilla. Also, your patch is still missing some commit information (e.g. author, commit summary) – did you follow the instructions at [1]? By the way, we don't use the checkin flag so you only need to specify the review flag. [1]: 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
Attachment #8747272 -
Flags: review?(michael.l.comella)
Attachment #8747272 -
Flags: feedback+
Attachment #8747272 -
Flags: checkin?(michael.l.comella)
to download the new version I use hg pull --rebase and hg update.yes I follow this instruction but when I do hg qnew..... I have this error (unresolved merge conflicts ) what should I do in this case? thank you
Reporter | ||
Comment 9•8 years ago
|
||
(In reply to bzrd_Sdn from comment #8) > to download the new version I use hg pull --rebase and hg update.yes I > follow this instruction but when I do hg qnew..... I have this error > (unresolved merge conflicts ) what should I do in this case? > thank you It sounds like your rebase may be failing with merge conflicts – did you see any errors when running rebase? I'm not sure how to help without more details because with patch queues, I used a different workflow. Typically, I'd pop my patches, pull, and reapply my patches, in which case a .rej file will be returned and you can merge those changes in by hand. If you jump on irc, we'd better be able to help you out.
Reporter | ||
Comment 10•8 years ago
|
||
By the way, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions.
Assignee | ||
Comment 11•8 years ago
|
||
I've redone everything I don't know if it's good for you or not thank you
Attachment #8747272 -
Attachment is obsolete: true
Attachment #8749678 -
Flags: review?(michael.l.comella)
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8749678 [details] [diff] [review] patch.diff Review of attachment 8749678 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately, I still can't get your patch to apply locally – it must be improperly formatted (e.g. I don't know why build/macosx/universal/mozconfig is included in your patch). Our mozreview process [1] is much less error prone (i.e. you push the commits directly to the review server rather than dealing with patch files that need to be in specific formats) and the way we recommend long-term users create commits. If you're still stuck, I'd recommend trying that out. [1]: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html
Attachment #8749678 -
Flags: review?(michael.l.comella) → feedback-
Assignee | ||
Comment 13•8 years ago
|
||
I try to make other patch. thank you
Attachment #8749678 -
Attachment is obsolete: true
Attachment #8751766 -
Flags: review?(michael.l.comella)
Reporter | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=125c8f223e87
Reporter | ||
Comment 15•8 years ago
|
||
Comment on attachment 8751766 [details] [diff] [review] patch_bug1266199.diff Review of attachment 8751766 [details] [diff] [review]: ----------------------------------------------------------------- I made a push to our try test servers (above). Once the push goes green, you can add a "Need more information from" for me to land the patch. I had to fix the patch locally (there were some rebasing issues) since I didn't want to make this bug have any more back-and-forth than it had to. If we used the normal process, instead of adding a need info flag for me, you'd add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results. Also, your commit message should be of the form, "Bug ### - Message about the bug. r=reviewer" – I'll fix this one locally too. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8751766 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 16•8 years ago
|
||
hello hi I followed the steps on (https://www.mozilla.org/en-US/about/governance/policies/commit/) in order to access to test server. Can you please vouch my request please. thank's https://bugzilla.mozilla.org/show_bug.cgi?id=1274038
Reporter | ||
Comment 17•8 years ago
|
||
(In reply to bzrd_Sdn from comment #16) > Can you please vouch my request please. thank's All done! By the way, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions.
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/bf8d65788939
Keywords: checkin-needed
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf8d65788939
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 20•8 years ago
|
||
ok! thank's
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
•