Last Comment Bug 849525 - Clean up mailnews/subscribe.{js|xul}
: Clean up mailnews/subscribe.{js|xul}
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: Philip Chee
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-09 08:10 PST by Philip Chee
Modified: 2013-05-03 22:04 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1v1 fix whitespace. (22.84 KB, patch)
2013-03-09 08:22 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch v2.1 Port stuff from Thunderbird. (7.50 KB, patch)
2013-03-09 08:59 PST, Philip Chee
mnyromyr: review+
Details | Diff | Splinter Review
Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr [check-in comment 9] (7.80 KB, patch)
2013-03-21 01:37 PDT, Philip Chee
philip.chee: review+
mnyromyr: superreview+
Details | Diff | Splinter Review
Patch v3.1 aRow rs=bustage fix [check-in comment 11]. (1.14 KB, patch)
2013-05-03 22:02 PDT, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review

Description Philip Chee 2013-03-09 08:10:38 PST
I noticed a lot of \tabs and trailing whitespace in our version of subscribe.{js|xul} compared to Thunderbird. I've opened this bug to do two things:
1. Fix whitespace issues.
2. Port some minor tweaks from Thunderbird's version.
Comment 1 Philip Chee 2013-03-09 08:22:45 PST
Created attachment 723086 [details] [diff] [review]
Part 1v1 fix whitespace.

requesting r+moa from module owner.
Comment 2 Philip Chee 2013-03-09 08:54:40 PST
> Created attachment 723086 [details] [diff] [review]
> Part 1v1 fix whitespace.

To clarify, this patch:
1. Fixes whitespace issues including \tabs, trailing whitespace, and
2. Some comment only fixes (punctuation, capitalization)

3. Bug 407956 changed the code but someone forgot to change the following comment.
> -    // if the "subscribed" atom is in the list of properties
> +    // If the "subscribed" string is in the list of properties
Comment 3 Philip Chee 2013-03-09 08:59:42 PST
Created attachment 723092 [details] [diff] [review]
Patch v2.1 Port stuff from Thunderbird.

Part 2:

1. Adapt fix for TB Bug 441705 (SetUpServerMenu is no longer used).

2. Simplify code in function SubscribeOnLoad() without (I hope) having changed the logic.
2.1. Removed the try/catch block. I've no idea if this is still necessary.
2.2. Fold the remainder of SetUpServerMenu() into SubscribeOnLoad().

3. Port the fix from TB Bug 360510 (Make subsription checkboxes accessible).

> -                                    <treecell properties="Subscribed-?Subscribed Subscribable-?Subscribable"/>
> +                                    <treecell value="?Subscribed"
> +                                              properties="Subscribed-?Subscribed Subscribable-?Subscribable"/>

4. Also remove some useless dump() statments.
Comment 4 Karsten Düsterloh 2013-03-15 16:05:22 PDT
(In reply to Philip Chee from comment #2)
> To clarify, this patch:
> 1. Fixes whitespace issues including \tabs, trailing whitespace, and

I'm not convinced that this is useful enough to justify the change history.
Do you have a striking reason apart from the code looking ugly currently?

I'd be okay with 2+3.

+    // If the "subscribed" string is in the list of properties
+    // we are subscribed.

Shouldn't that have a comma behind "properties", then?
Comment 5 Karsten Düsterloh 2013-03-15 16:21:08 PDT
Comment on attachment 723092 [details] [diff] [review]
Patch v2.1 Port stuff from Thunderbird.

r=me by inspection.
Comment 6 Philip Chee 2013-03-16 01:46:37 PDT
>> To clarify, this patch:
>> 1. Fixes whitespace issues including \tabs, trailing whitespace, and
> I'm not convinced that this is useful enough to justify the change history.

There isn't much Hg change history to worry about:
http://hg.mozilla.org/comm-central/filelog/93e89494abfd/suite/mailnews/subscribe.js

> Do you have a striking reason apart from the code looking ugly currently?
Well the \tabs make it difficult (for me) to figure out the indentation.

Also TB did some clean up in this file and others in 2007 see:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mail/base/content/subscribe.js&rev=1.10&mark=1.10
"remove tabs and some trailing white space from thunderbird's xul and js files. purely white space changes."

Which makes trying to diff the TB and SM versions to see what changes are significant rather more painful.

> +    // If the "subscribed" string is in the list of properties
> +    // we are subscribed.
> 
> Shouldn't that have a comma behind "properties", then?
Fixed locally.
Comment 7 Philip Chee 2013-03-21 01:37:22 PDT
Created attachment 727563 [details] [diff] [review]
Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr [check-in comment 9]

This patch:
Port some improvements from Thunderbird.
Comment only fixes (punctuation, capitalization).
Remove some useless dump() statments.

But doesn't have the whitespace changes in an earlier patch.

Carrying forward r+. Requesting moa/sr
Comment 8 Karsten Düsterloh 2013-04-08 15:11:21 PDT
Comment on attachment 727563 [details] [diff] [review]
Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr [check-in comment 9]

(In reply to Philip Chee from comment #6)
> >> To clarify, this patch:
> >> 1. Fixes whitespace issues including \tabs, trailing whitespace, and
> > I'm not convinced that this is useful enough to justify the change history.
> 
> There isn't much Hg change history to worry about

Okay, indeed, granted.
So, if you really feel pain enough, feel free to whitespace-clean-up the entire file, but include correcting the bracing style to "own line" then as well.

moa=me, plus rs=me for the cleanup.
Comment 9 Philip Chee 2013-04-10 08:19:53 PDT
Pushed
http://hg.mozilla.org/comm-central/rev/34a222f3e8cb (whitespace fixes)
http://hg.mozilla.org/comm-central/rev/d04c9f8a73ad (port Thunderbird improvements)
Comment 10 Jens Hatlak (:InvisibleSmiley) 2013-05-03 09:02:04 PDT
Missed at least one occurrence of "row" -> "aRow":
http://mxr.mozilla.org/comm-central/source/suite/mailnews/subscribe.js#327

-> Trying to check a box results in a message in the error console:

Error: ReferenceError: row is not defined
Source File: chrome://messenger/content/subscribe.js
Line: 327

(Reported by Hartmut Figge on m.s.seamonkey)
Comment 11 Philip Chee 2013-05-03 22:02:35 PDT
Created attachment 745491 [details] [diff] [review]
Patch v3.1 aRow rs=bustage fix [check-in comment 11].

> Missed at least one occurrence of "row" -> "aRow":
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/subscribe.js#327
> 
> -> Trying to check a box results in a message in the error console:
> 
> Error: ReferenceError: row is not defined
> Source File: chrome://messenger/content/subscribe.js
> Line: 327
> 
> (Reported by Hartmut Figge on m.s.seamonkey)

Pushed a bustage fix. rs=bustage

http://hg.mozilla.org/comm-central/rev/0dc685246f2b

Note You need to log in before you can comment on or make changes to this bug.