559833 - Should fix incorrect indentation of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[ISHIKAWA, Chiaki]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: 

nsHelperAppDlg.js does not follow the basic indentation rule, namely two-space indentation in many places.
Also, it has incorrect emacs mode-line that seems to set incorrect
indentation spaces.

This indentation problem was noticed while a problem
discussed in

"Bug 429827  - Download manager does not warn when its download location does not exist or is write protected"

was being fixed.

This incorrect indentation and the incorrect mode-line interferes
with editing for bug fix, and should be fixed.


We should re-indent this code once for all after a major bug (or two) is
fixed.


Reproducible: Always

Comment 1

[ISHIKAWA, Chiaki]

I waited for the patch for Bug 506987 to be in the trunk.
That seems to have happened now (several days ago).

So I would like to post the patch to change the indentation throughly for nsHelperAppDlg.js first, and then later post the bug fix to 429827 against the properly indented code.

The first patch I am going to post is 
 - the indentation patch (produced by hg diff -U 8) and 
 - the same patch ignoring the whitespace changes (produced by 
   hg extdiff  -w -o -i -o -b -o -Npru)

The second patch above is just for showing the change is cosmetic.
 - Removing the bogus mode line near the beginning (incorrect tab setting 4 should read two, but I simply removed it.)
 - and commenting out the code (which is inside #if 0, #endif anyway) to
   close the hanging "{" which interfered with the proper indentation using
   emacs earlier.

The indentation is now based on  using two spaces tab (js-indent-level customized to 2) using the latest js-mode of Emacs 23.2 distribution.

TIA.

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Clean up indentation problem (hg diff -U 8)

Clean up indentation which was incorrect according to javascript coding style.
Produced by "hg diff -U8"

Comment 3

[ISHIKAWA, Chiaki]

Attachment: The same patch but ignoring the whatspace changes.

Produced by "hg extdiff -o -w -o -i -o -b -o -Npru".
I needed to use extdiff since built-in diff of mercurial doesn't seem work well for ignoring whitespace changes.

TIA.

Comment 4

[:Paolo Amadini]

Comment on attachment 446236 [details] [diff] [review]
Clean up indentation problem (hg diff -U 8)

Thanks for the cleanup, here are my comments.

>-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Rather than removing the line, include the standard mode lines for Emacs and
vim that have been recently formalized in the Mozilla Coding Style Guide:

https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#Mode_Line

Note that, as far as I can tell, using "Mode: C++" should be fine for JS files too.

>   return aDirectory.exists() && aDirectory.isDirectory() &&
>-         aDirectory.isWritable();
>+    aDirectory.isWritable();

For better readability, line continuations should be aligned with the start of
the expression in the first line, thus the first line is the correct one. There
are other instances of line continuations in the file, you should check them out.
Since we're only fixing whitespace, don't worry if aligning the operators still
results in lines that are longer than 80 characters.

In addition to the fixes above, please remove all the whitespace at end-of-line,
and watch out for TABs, they should not be present in the file (that's what the
Emacs mode line including "indent-tabs-mode: nil" achieves).

Comment 5

[ISHIKAWA, Chiaki]

Dear Paolo, and others.

I modified the patch according to your suggestion.

Mode line uses the suggested emacs mode line (although I have a sneaking
suspicion that in the future, maybe we may want to use JavaScript or something.)

For continuations, I tried to follow your suggestions, especially concerning the
member referencing using ".", but do let me know if I misunderstood the guideline.

The whitespaces at end-of-line were identified and eliminated using
whitespace-mode of emacs.

Tabs were removed by using untabify command of emacs. 

I am posting the new patches now.

TIA

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Clean up indentation problem (hg diff -U 8)

Newly created (suggestions from Paolo included) patch.
Created using by hg diff -U 8

Comment 7

[ISHIKAWA, Chiaki]

Attachment: The same patch but ignoring the whitespace changes.

The same patch as previous one, but now ignoring the whitespace changes.

Comment 8

[:Paolo Amadini]

Comment on attachment 446756 [details] [diff] [review]
Clean up indentation problem (hg diff -U 8)

>-      this._showTimer = Components.classes["@mozilla.org/timer;1"]
>-                                  .createInstance(nsITimer);
>+    this._showTimer = Components.classes["@mozilla.org/timer;1"]
>+      .createInstance(nsITimer);

The first alignment is the correct one, with the two dots on the
right side of the equal sign starting in the same column.

The rest of the patch looks fine to me; you can now post the updated
version with the fixes above, and request a final review from Shawn.

Comment 9

[ISHIKAWA, Chiaki]

(In reply to comment #8)
> (From update of attachment 446756 [details] [diff] [review])
> >-      this._showTimer = Components.classes["@mozilla.org/timer;1"]
> >-                                  .createInstance(nsITimer);
> >+    this._showTimer = Components.classes["@mozilla.org/timer;1"]
> >+      .createInstance(nsITimer);
> 
> The first alignment is the correct one, with the two dots on the
> right side of the equal sign starting in the same column.
> 
> The rest of the patch looks fine to me; you can now post the updated
> version with the fixes above, and request a final review from Shawn.

Thank you, Paolo, for spotting the incorrect indentation.

I checked carefully over my patch, and found similar incorrect indentations
regarding the "." operator to my embarrassment. 

I re-checked carefully, and produced the following patches. Hopefully this should be it. I request a final review from Shawn.

TIA.

Comment 10

[ISHIKAWA, Chiaki]

Attachment: Clean up indentation problem (hg diff -U 8)

Clean up indentation problem. (hg diff -U 8)

Comment 11

[ISHIKAWA, Chiaki]

Attachment: The same patch but ignoring the whitespace changes.

The preceding patch, but the diff is generated using
hg extdiff -o -w -o -i -o -b -o -Npru

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 447330 [details] [diff] [review]
Clean up indentation problem (hg diff -U 8)

Paolo's review is sufficient for this patch.

Comment 13

[:Paolo Amadini]

Comment on attachment 447330 [details] [diff] [review]
Clean up indentation problem (hg diff -U 8)

Looks good! You can now add the "checkin-needed" keyword to the bug, then we should wait until the patch is checked in.

Comment 14

[ISHIKAWA, Chiaki]

(In reply to comment #13)

Thank you, Paolo.
I have added "checkin-needed" to keywords. (It took me a while to figure this out. There are so many rules and procedures in bugzilla before a bug is fixed!)

TIA

Comment 15

[ISHIKAWA, Chiaki]

Ahem, I thought I set checkin-needed in Keywords field.
But it is now showing up in the web data.
I am retrying. If things didn't work as I expected, I may need someone'shelp.
Hmm...

Comment 16

[Justin Wood (:Callek)]

Pushed as: http://hg.mozilla.org/mozilla-central/rev/ceeb7737c4bb