Closed Bug 931244 Opened 11 years ago Closed 10 years ago

Move BrowserApp.mBrowserToolbar listeners setting code to a helper function

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: mcomella, Assigned: alexandru.chiriac)

Details

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

Attachments

(1 file, 7 obsolete files)

Currently the code to set listeners on BrowserApp.mBrowserToolbar is in BrowserApp.onCreate [1], making the method harder to read quickly than it needs to be.

These functions should be moved to a private helper function with a name such as BrowserApp.setBrowserToolbarListeners, to help improve readability.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=f1b97193d162#451
Hi, I'd like to work on this if that's possible. I'm really new to working on large projects like this and might need a bit of guidance.
Hey, Derek! Welcome to Bugzilla! I've assigned you to the bug so you can get working whenever you are ready.

Have you already set up a build environment? If not, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

The link in comment 0 points you to MXR, our code search utility [1], which can be used to do searches on the Mozilla code base. The file that we're specifically dealing with is in "<path-to-mozilla-central>/mobile/android/base/BrowserApp.java", which is visible in an MXR search at the top of the page.

The bug description in comment 0 should give you a good basis of what you need to change. Once these changes are made, compile the code, run it, make sure everything still works as expected, and post the patch (see [2] on how to create a patch) for review (I can be your reviewer).

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! ^_^

[1]: https://mxr.mozilla.org/mozilla-central/
[2]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee: nobody → d.roc16
Status: NEW → ASSIGNED
Whiteboard: [mentor=mcomella][lang=java][good-first-bug] → [mentor=mcomella][lang=java][good first bug]
Hey, Derek.

Are you still working on this?
Flags: needinfo?(d.roc16)
Derek, I'm going to unassign you so someone else can take the bug if they're interested.

If you are still interested in working on it, please let me know!
Assignee: d.roc16 → nobody
Flags: needinfo?(d.roc16)
May I please get it assigned to work on it, Looks like a simple re-factoring. Happy to get started with this.

Thanks
Assignee: nobody → gillh11
Take a look at comment 2 if you need some suggestions on where to start and let me know if you have any questions!

Thanks and good luck! :)
It looks like It needs duplicating a function and editing it abit, If I am right
Which function do you think needs to be duplicated? I'm not sure that I see that here. comment 0 mentions what I think needs to happen.
Hey, are you still working on this?
Flags: needinfo?(gillh11)
Yes I am working this, should be solved soon
Flags: needinfo?(gillh11)
Yes I am working this, should be solved soon
Hi, just a quick question? I am currently working on the code is there a way I am able to do some testing through an IDE whereby I am able to isolate the problem and create a few test cases. Is there already an online IDE for this provided by mozilla or should I create everything myself. Also how do I submit the code I have written.
(In reply to gillh11 from comment #12)
> Hi, just a quick question? I am currently working on the code is there a way
> I am able to do some testing through an IDE whereby I am able to isolate the
> problem and create a few test cases.

You can write UI tests using the Robocop framework [1], though I believe our coverage should be comprehensive enough to ensure your changes do not break any functionality. If you want to write additional tests or run the suite, check out [1]. However, we do not have a unit test suite if that's the testing functionality you were looking for.

> Is there already an online IDE for this
> provided by mozilla or should I create everything myself.

Unfortunately, we don't yet have official IDE support for developing for Firefox for Android, though Eclipse support is in the pipeline (see bug 853045). There have been success stories with IntelliJ IDEA as well [2], though if you try to set it up, you're likely on your own. Most of us use coding-oriented text editors (e.g. vim, emacs, sublime). 

> Also how do I
> submit the code I have written.

comment 2 generally explains some initial steps, but to specifically answer your question, you can generate a patch with mercurial - see [3] - attach it to the bug, flag me for review, and once it passes review, add the "checkin-needed" keyword to get it checked into version control.

Please let me know if you have more questions! :)

[1]: https://wiki.mozilla.org/Auto-tools/Projects/Robocop
[2]: http://perplexedturnip.wordpress.com/2013/07/31/bludgeoning-intellij-idea-into-being-useful-for-mobile-firefox-development/
[3]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Hello Michael, I have spent few weeks just trying to generate a patch but it is causing issues and doesn't allow me to put the code in a patch, I can try once again, but are there any alternatives. I have done something and I would like you to have a look at it.
Is it possible to email you the code for your review
Is it possible to email you the code for your review
If you explain to me the issues you've been having, I can help step you through the process. While I feel it's better for you to get the experience generating the patches, I suppose I can take the code as an email attachment. However, just know that this won't always be the case with other reviewers.

If you choose to try generating the patch yourself, it would probably be easiest to communicate via IRC [1]. My nickname is "mcomella" and you can find me in the channel "#mobile" typically Monday-Friday, 10am-6pm PST.

[1]: https://wiki.mozilla.org/IRC
(In reply to Michael Comella (:mcomella) from comment #17)
> If you explain to me the issues you've been having, I can help step you
> through the process. While I feel it's better for you to get the experience
> generating the patches, I suppose I can take the code as an email
> attachment. However, just know that this won't always be the case with other
> reviewers.
> 
> If you choose to try generating the patch yourself, it would probably be
> easiest to communicate via IRC [1]. My nickname is "mcomella" and you can
> find me in the channel "#mobile" typically Monday-Friday, 10am-6pm PST.
 Okay That is fine. I have managed to generate a patch and should send it to you by Monday for a review
I need your review on the work I have done so It is not finished yet but near to completion so I need to know when I create a patch how do I include the code into the patch file.
The official instructions are at [1]. My summary is below:

You'll want to create an ~/.hgrc file with the information as described in [1]. Once that is set up, you can type `hg qnew <patch_name>` from your mozilla source tree to create a new patch and add all of the current changes to that file. `hg qref` will update the topmost patch with any new changes that have been made since the last `qnew` or `qref`.

If you're having a specific difficulty, feel free to ask questions!

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Okay Thanks I have made the patch I will send them to you on IRC on monday.
The preferred way to send the code is to attach the patch to this bug (here on Bugzilla) by clicking the "Add an attachment" link and uploading the patch file. Would it be possible to do that instead?
Attached patch Browser.patch (obsolete) — Splinter Review
Moved the method initialisers in the original onCreate class to an independent class
Attachment #8373229 - Flags: feedback?
Attachment #8373229 - Flags: feedback? → feedback?(gillh11)
Attached patch Browserapp.patch (obsolete) — Splinter Review
Moved the method initialisers in the original onCreate class to an independent class
Attachment #8373229 - Attachment is obsolete: true
Attachment #8373229 - Flags: feedback?(gillh11)
Attachment #8373234 - Flags: review?(gillh11)
Attachment #8373234 - Flags: feedback?(gillh11)
Attached patch Browser.patch (obsolete) — Splinter Review
Attachment #8373236 - Flags: review?(gillh11)
Attachment #8373236 - Flags: feedback?
Unfortunately, it does not appear your patches are correctly formatted for bugzilla (and thus they're unreviewable). Did you create the ~/.hgrc file as specified in [1]?

Additionally, when you're ready for me to review the code, you should flag me for review. To find my email, you can use auto-completion: type ":mcomella" and at some point, my email should show up as an auto-completion option.
You can click the "diff" button on your uploaded patches to ensure they were formatted properly.
Attachment #8373234 - Attachment is obsolete: true
Attachment #8373234 - Flags: review?(gillh11)
Attachment #8373234 - Flags: feedback?(gillh11)
Comment on attachment 8373236 [details] [diff] [review]
Browser.patch

Also, unless the HG patch format is not easily human-readable, it appears that your changesets are not in the mozilla source tree (as the parent changeset is 0).

If that's the case, you'll want to use [1] to properly set up the repository.

If you need help stepping through anything, feel free to ask. Again, it'd probably be easiest to do on IRC.

[1]: https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial
Attachment #8373236 - Flags: review?(gillh11)
Attachment #8373236 - Flags: feedback?
(In reply to Michael Comella (:mcomella) from comment #28)
Yes, I created ~/hgrc file, How will the patch look like at the end, would it have a string of numbers? I followed that link provided. however, I will try again.
Ensure your file name is prepended with a dot: "~/.hgrc".

You can see an example patch here: https://bug910859.bugzilla.mozilla.org/attachment.cgi?id=8338994
(In reply to Michael Comella (:mcomella) from comment #30)
> Ensure your file name is prepended with a dot: "~/.hgrc".
> 
I am getting a following error when I try enter hg diff
'less.exe' is not defined as an internal or external command,operable program or batch file
What is causing these errors?
That sounds like you're using a regular console window instead of the mozilla-build environment.
> (In reply to Michael Comella (:mcomella) )
Update: I am still working on this project, I will post a patch soon.
Hello Michael, I will send another patch soon. I have been working on this bug.
Flags: needinfo?(michael.l.comella)
Attachment #8388519 - Flags: review?(michael.l.comella)
Attachment #8388519 - Flags: checkin?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Comment on attachment 8388519 [details] [diff] [review]
Moved the method initialiser to an independent class

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

The file in the patch is called, "bug.java" so I'm not sure if this is an hg issue or if you've added the code to a new file. Can you make sure you're working directly in BrowserApp.java and creating the patch file via your hg checkout?

::: bug.java
@@ +477,5 @@
>          if (mBrowserSearch == null) {
>              mBrowserSearch = BrowserSearch.newInstance();
>              mBrowserSearch.setUserVisibleHint(false);
>          }
> +        

nit: We like to remove excess whitespace from the files (certain newlines, whitespace at the end of a line). There are several throughout the file that you've added - do you mind removing them? You can add 

[color]
diff.trailingwhitespace = red_background

to your ~/.hgrc to highlight these spaces when running `hg diff -p`.

@@ +480,5 @@
>          }
> +        
> +        setBrowserToolbarListeners();
> +
> +        //These functions have been moved to a private helper function BrowserApp.setBrowserToolbarListeners to help improve readability

No need to comment out this code - just remove it.

@@ +643,1 @@
>  		mBrowserToolbar.setOnActivateListener(new BrowserToolbar.OnActivateListener() {

nit: This indentation does not appear to be correct.

@@ +2721,5 @@
>                                           appLocale,
>                                           previousSession);
>      }
>  }
> +s
\ No newline at end of file

Unnecessary character - I don't think this compiles.
Attachment #8388519 - Flags: review?(michael.l.comella)
Attachment #8388519 - Flags: review-
Attachment #8388519 - Flags: checkin?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #36)
I have been working directly from BrowserApp.java. I renamed the file as I was working on the code at two places with two different ideas. But I have use hg command. Is the patch the way you want it. I will remove the access code. The character must have been added when I was looking at the file. I should send another patch by tomorrow
(In reply to Michael Comella (:mcomella) from comment #36)
I have been working directly from BrowserApp.java. I renamed the file as I was working on the code at two places with two different ideas. But I have use hg command. Is the patch the way you want it. I will remove the access code. The character must have been added when I was looking at the file. I should send another patch by tomorrow
(In reply to gillh11 from comment #38)
> (In reply to Michael Comella (:mcomella) from comment #36)
> I have been working directly from BrowserApp.java. I renamed the file as I
> was working on the code at two places with two different ideas.

You can manage multiple hg patches to do that work with less frequent errors.

> But I have
> use hg command. Is the patch the way you want it.

Unfortunately, the patch does not apply because it's looking to make changes to a file called bug.java, which does not exist. You have to make your changes to the existing files in order to get a properly formatted patch.

Also, make sure there's an extra newline at the end of the file too (or that your editor does this automatically, but looking at the previous patch, I do not think this is the case).
Attached patch latest.patch (obsolete) — Splinter Review
Method initializer to an independent class
Attachment #8390507 - Flags: review?(michael.l.comella)
Attachment #8390507 - Flags: checkin?(michael.l.comella)
Comment on attachment 8390507 [details] [diff] [review]
latest.patch

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

You should only need to add the review flag when you want a patch reviewed. Honestly, I'm not sure what the checkin flag is for. :P

The patch does not apply because your BrowserApp.java file is in the root of the repository, rather than mobile/android/base/BrowserApp.java. Can you make sure you're editing the file where it already exists in the repository? Make sure that the patch you're submitting is the only patch applied (use `hg qapplied` to view and use `hg qfold` to combine them if there are multiple) - I think you may have created multiple patches because the code to set the BrowserToolbar listeners appears to be missing. Also, your credentials (name & email) do not appear to be in the patch. Can you make sure to set up your ~/.hgrc like [1]?

Additionally, it doesn't seem to apply even when the paths are corrected - can you make sure you have the latest pull of the repository? You should be able to do this by `hg pull && hg up`. You'll have to fix the merge conflicts.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: BrowserApp.java
@@ +477,5 @@
>          if (mBrowserSearch == null) {
>              mBrowserSearch = BrowserSearch.newInstance();
>              mBrowserSearch.setUserVisibleHint(false);
>          }
> +        

nit: Can you remove the excess whitespace you've added, and the excess whitespace that may have already existed on lines that you've changed?

@@ +481,5 @@
> +        
> +        setBrowserToolbarListeners();
> +	
> +	    // Intercept key events for gamepad shortcuts
> +	    mBrowserToolbar.setOnKeyListener(this);

This should also be in the toolbar listener method.

@@ +483,5 @@
> +	
> +	    // Intercept key events for gamepad shortcuts
> +	    mBrowserToolbar.setOnKeyListener(this);
> +	
> +	    if (mTabsPanel != null) {

nit: indentation.

I don't see this code in my local version of the repository - perhaps it was removed?

@@ +2677,5 @@
>                                           appLocale,
>                                           previousSession);
>      }
>  }
> +

Now there appears to be an extra newline at the end of the file - try removing it. Sorry for the confusion!
Attachment #8390507 - Flags: review?(michael.l.comella)
Attachment #8390507 - Flags: review-
Attachment #8390507 - Flags: checkin?(michael.l.comella)
Attached patch latest.patch (obsolete) — Splinter Review
Name and email are both included in the patch,
Path is mobile/android/base/BrowserApp.Java.
This should work.
Thanks
Attachment #8391482 - Flags: review?(michael.l.comella)
Comment on attachment 8391482 [details] [diff] [review]
latest.patch

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

The comment on this patch is misleading and can be more detailed: you moved the BrowserToolbar listener initialization to a helper method. A class is a separate construct that you define with with the `class` keyword. Please update the comment with `hg qref -e`.

It may be my fault for not reviewing this quickly enough (sorry!) but this patch fails to apply. Do you mind pushing it onto the latest changes again?

Also, did you compile and run this? With the spelling error (see below), I don't believe this should compile... Please make sure you test your code!

::: mobile/android/base/BrowserApp.java
@@ +487,5 @@
>              mBrowserSearch = BrowserSearch.newInstance();
>              mBrowserSearch.setUserVisibleHint(false);
>          }
>  
> +        setBrowserToolbarListners();

nit: Spelling - `setBrowserToolbarListeners();`

@@ +659,5 @@
> +                mDoorHangerPopup.enable();
> +            }
> +        });
> +		
> +    }

nit: Add a newline below this line and remove the newline above it.
Attachment #8391482 - Flags: review?(michael.l.comella) → review-
Hello, I would like to work on this bug, if I'm allowed to, of course.

Thanks, 
Alex
Unfortunately, this bug is currently assigned. The assignee contacted me several days ago via email so I believe they're still working on it, but I'll double-check:

Are you still working on this?
Flags: needinfo?(gillh11)
Alexandru, are you still interested in working on this?
Flags: needinfo?(alexandru.chiriac)
Hi Michael, I'm still interested in working on this bug (the Fennec development environment is set up).
Flags: needinfo?(alexandru.chiriac)
Okay, Alexandru, you've been assigned. comment 2 should give you some information on what needs to be changed, and how you can contact me if you need any assistance (though you can always drop a comment in the bug!).

I apologize, gillh11, but that means I'm unassigning you from the bug (due to inactivity). Let me know if you would like help picking out another bug to work on.
Assignee: gillh11 → alexandru.chiriac
Flags: needinfo?(gillh11)
Noticed and removed some whitespaces accidentally added in the first patch.
Attachment #8407397 - Attachment is obsolete: true
Attachment #8407397 - Flags: review?(michael.l.comella)
Attachment #8407529 - Flags: review?(michael.l.comella)
Comment on attachment 8407529 [details] [diff] [review]
Improved BrowserApp.onCreate() method readability by moving BrowserApp.mBrowserToolbar listeners to a private helper method.

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

Nice, lgtm!

Do you know about the "checkin-needed" keyword?
Attachment #8407529 - Flags: review?(michael.l.comella) → review+
Sure, I will add the 'checkin-needed' keyword.
Keywords: checkin-needed
Great, thanks for your help! :)
Attachment #8391482 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/10e08afa522a
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=java][good first bug] → [mentor=mcomella][lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/10e08afa522a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=java][good first bug][fixed-in-fx-team] → [mentor=mcomella][lang=java][good first bug]
Target Milestone: --- → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.