Fix indentation in LoginManagerPrompter.js

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
General
P4
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: liuche, Assigned: Stanislas Daniel Claude Dolcini)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 44
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Instead of 4-space, we should make this 2-space.

(This should wait until we have everything that we might to uplift done.)
(Assignee)

Comment 1

2 years ago
Shall I replace every tab by two spaces ? Is it the coding convention here instead of tabs/4spaces ?

Comment 2

2 years ago
(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.
(Assignee)

Comment 3

2 years ago
Created attachment 8675924 [details] [diff] [review]
bug_1147197.patch

Should fix it 400 lines updated :)
Attachment #8675924 - Flags: review+

Comment 4

2 years ago
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 5

2 years ago
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?
(Assignee)

Comment 6

2 years ago
Mmmh one week or two, I'm going to update it, and rebase it.
(Assignee)

Comment 7

2 years ago
It seems that file has been deleted :( So I guess this ticket is no longer valid.

Comment 8

2 years ago
(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
(Assignee)

Comment 9

2 years ago
Created attachment 8676953 [details] [diff] [review]
bug_1147197.patch

Maybe that one works ?
Attachment #8675924 - Attachment is obsolete: true
Attachment #8675924 - Flags: review?(margaret.leibovic)
Attachment #8676953 - Flags: review?(margaret.leibovic)

Comment 10

2 years ago
(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 :)

Updated

2 years ago
Attachment #8676953 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 11

2 years ago
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...

Comment 12

2 years ago
(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).
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 14

2 years ago
Created attachment 8678506 [details]
The file

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

Comment 15

2 years ago
(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.
(Assignee)

Comment 16

2 years ago
Created attachment 8679380 [details] [diff] [review]
bug_1147197.patch

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 17

2 years ago
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+

Comment 18

2 years ago
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
(Assignee)

Comment 19

2 years ago
Created attachment 8679731 [details] [diff] [review]
bug_1147197.patch

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 20

2 years ago
Comment on attachment 8679731 [details] [diff] [review]
bug_1147197.patch

Thanks!
Attachment #8679731 - Flags: review?(margaret.leibovic) → review+

Comment 21

2 years ago
Thanks again for your patch. Let me know if you need help finding more bugs to work on!
Keywords: checkin-needed
(Assignee)

Comment 22

2 years ago
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 :)

Comment 23

2 years ago
(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

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b2824d5f145
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.