Closed Bug 1147197 Opened 10 years ago Closed 9 years ago

Fix indentation in LoginManagerPrompter.js

Categories

(Firefox for Android Graveyard :: General, defect, P4)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: liuche, Assigned: dolcinis)

References

Details

Attachments

(1 file, 4 obsolete files)

bug_1147197.patch
28.86 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
Instead of 4-space, we should make this 2-space. (This should wait until we have everything that we might to uplift done.)
Shall I replace every tab by two spaces ? Is it the coding convention here instead of tabs/4spaces ?
(In reply to Stanislas Daniel Claude Dolcini from comment #1) > Shall I replace every tab by two spaces ? Is it the coding convention here > instead of tabs/4spaces ? Yes! 2-space indentation is our convention for JS code.
Attached patch bug_1147197.patch (obsolete) — — Splinter Review
Should fix it 400 lines updated :)
Attachment #8675924 - Flags: review+
Comment on attachment 8675924 [details] [diff] [review] bug_1147197.patch When uploading a patch, you should request review with the review? flag, and then the reviewer will grant the review+ :) I'll try to find a time to look this over later today. Thanks for the patch!
Attachment #8675924 - Flags: review+ → review?(margaret.leibovic)
Comment on attachment 8675924 [details] [diff] [review] bug_1147197.patch This patch didn't apply for me locally. When did you last update your tree? Did you have any other patches applied?
Mmmh one week or two, I'm going to update it, and rebase it.
It seems that file has been deleted :( So I guess this ticket is no longer valid.
(In reply to Stanislas Daniel Claude Dolcini from comment #7) > It seems that file has been deleted :( So I guess this ticket is no longer > valid. Are you sure? I still see it here: http://hg.mozilla.org/mozilla-central/file/d43374e69703/mobile/android/components/LoginManagerPrompter.js
Attached patch bug_1147197.patch (obsolete) — — Splinter Review
Maybe that one works ?
Attachment #8675924 - Attachment is obsolete: true
Attachment #8675924 - Flags: review?(margaret.leibovic)
Attachment #8676953 - Flags: review?(margaret.leibovic)
(In reply to Stanislas Daniel Claude Dolcini from comment #9) > Created attachment 8676953 [details] [diff] [review] > bug_1147197.patch > > Maybe that one works ? Hm, this still isn't working. How did you generate this patch? I'm not sure what directions you were following, but we have a new tool called MozReview that makes uploading patches for review much easier. I would encourage you to read this page to generate a patch for review: http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html You'll have to follow some steps to configure the tool, but all you need is bugzilla credentials, and you already have those :)
Attachment #8676953 - Flags: review?(margaret.leibovic)
Well the diffviewer in bugzilla seems to recognize the patch though... I'm using Tortoise Hg against r265985 and creating a patch from that. I'm on windows so I can't use the command line...
(In reply to Stanislas Daniel Claude Dolcini from comment #11) > Well the diffviewer in bugzilla seems to recognize the patch though... I'm > using Tortoise Hg against r265985 and creating a patch from that. I'm on > windows so I can't use the command line... Were you able to build Firefox for Android on Windows? I didn't think that was supported... If you can't use the command line I'm afraid it will be hard to do Firefox for Android development. I think most people on Windows use a Linux VM, or you should be able to install some sort of terminal application (it's been a long time since I've developed on Windows, so I forget what the options are).
I actually never tried since it's Js and you can't really compile that. I just cloned mozilla central and got the android code :) I can install a VM. But I want to understand what's wrong as it doesn't make much sense.
Attached file The file (obsolete) —
I attached the whole file so you can see the changes. Hopefully that will fix it :) I know it's not a conventional way, But I really want to get involved and fix stuff in JS
(In reply to Stanislas Daniel Claude Dolcini from comment #13) > I actually never tried since it's Js and you can't really compile that. I > just cloned mozilla central and got the android code :) Correct that you don't "compile" JS, but you do need to build Firefox for Android in order to test your changes. What you're telling me here is that you didn't test this patch to make sure it works properly. If you're going to contribute to Firefox for Android, you need to be able to test your changes. I know that this seems silly for a simple whitespace change like this, but the point of [good first bugs] like this is to get contributors ramped up to making more significant changes, which will definitely require verifying your change does what's expected. > I can install a VM. But I want to understand what's wrong as it doesn't make > much sense. I suspect your patch contains Windows line endings, which causes it to fail in my Unix environment.
Attached patch bug_1147197.patch (obsolete) — — Splinter Review
I set up a dev environnement and made this patch. Hopefully this one works.
Attachment #8676953 - Attachment is obsolete: true
Attachment #8678506 - Attachment is obsolete: true
Attachment #8679380 - Flags: review?(margaret.leibovic)
Comment on attachment 8679380 [details] [diff] [review] bug_1147197.patch Review of attachment 8679380 [details] [diff] [review]: ----------------------------------------------------------------- This is great, thanks! Sorry for the trouble about the build system, but it will pay off now that you have a dev environment set up :)
Attachment #8679380 - Flags: review?(margaret.leibovic) → review+
One last thing: you should update this patch to have a commit message for check-in. Here are some details about how to format a commit message: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions Basically, you want to include a bug number, what you changed, and a reviewer (me, in this case). After that, we can mark this bug with the "checkin-needed" keyword, and someone will check it in.
Assignee: nobody → dolcinis
Hopefully everything is fine. Vim messed with my head, and I had to redo it 10 times...
Attachment #8679380 - Attachment is obsolete: true
Attachment #8679731 - Flags: review?(margaret.leibovic)
Comment on attachment 8679731 [details] [diff] [review] bug_1147197.patch Thanks!
Attachment #8679731 - Flags: review?(margaret.leibovic) → review+
Thanks again for your patch. Let me know if you need help finding more bugs to work on!
Keywords: checkin-needed
Indeed I do. The idea is to fix as much bugs as I can In order to : 1. Be useful 2. Get better at JS/BugFixing 3. Get a nice mark 4. Get something to write on my C.V. 5. Be able to fix bugs for another open source project (http://trac.wildfiregames.com/) So anything you can find me(Not too hard, cause I won't probably be able to do it) We could finish the let var thingy. You haven't answered on that ticket :)
(In reply to Stanislas Daniel Claude Dolcini from comment #22) > Indeed I do. The idea is to fix as much bugs as I can In order to : > > 1. Be useful > 2. Get better at JS/BugFixing > 3. Get a nice mark > 4. Get something to write on my C.V. > 5. Be able to fix bugs for another open source project > (http://trac.wildfiregames.com/) > > So anything you can find me(Not too hard, cause I won't probably be able to > do it) > We could finish the let var thingy. You haven't answered on that ticket :) I just replied in that ticket! If you're looking for JS bugs, this is a good tool to help you filter for JS-specific bugs: http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&js=1 We only use JS for some Firefox for Android development (most is in Java), so if you have a hard time finding bugs here, you should also look at contributing to desktop Firefox, since that UI is written completely with JS: http://www.joshmatthews.net/bugsahoy/?ff=1&js=1
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b2824d5f145
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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: