Closed Bug 1266199 Opened 8 years ago Closed 8 years ago

Rename BrowserApp.searchEngineManager -> mSearchEngineManager

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

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)

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! ^_^
Attached patch patch.diff (obsolete) — Splinter Review
patch for the bug 1266199
Attachment #8743839 - Flags: review?(michael.l.comella)
Assignee: nobody → bouzroudzeineb
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+
Attached patch newpatch.diff (obsolete) — Splinter Review
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)
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+
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?
Attached patch patch_bug1266199.diff (obsolete) — Splinter Review
new patch
Attachment #8744585 - Attachment is obsolete: true
Attachment #8747272 - Flags: review?(michael.l.comella)
Attachment #8747272 - Flags: checkin?(michael.l.comella)
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
(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.
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.
Attached patch patch.diff (obsolete) — Splinter Review
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)
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-
I  try to make other patch.
thank you
Attachment #8749678 - Attachment is obsolete: true
Attachment #8751766 - Flags: review?(michael.l.comella)
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+
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
(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.
https://hg.mozilla.org/mozilla-central/rev/bf8d65788939
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
ok! thank's
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: