437552 - Clean up whitespace-on-blank-lines problem in /PreferencePanes

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Chris Lawson (gone)]

There's a ton of whitespace on blank lines in Appearance.mm and a couple in Appearance.h. We should kill it.

There are something like three different patches in the queues right now that touch one or both of these two files, though, so let's wait until some of that stuff lands or we'll end up with lots of bitrot.

Comment 1

[Chris Lawson (gone)]

Attachment: style fix v1.0

Jeff, can you take a look at this? It's purely style fixes (and there might be some I missed, so feel free to be nitpicky).

Comment 2

[Jeff Dlouhy]

Comment on attachment 340867 [details] [diff] [review]
style fix v1.0

Ah, pretty code.

r=me.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 340867 [details] [diff] [review]
style fix v1.0

This is the old patches police; please choose your desired sr.

Comment 4

[Chris Lawson (gone)]

Attachment: fix v1.1, takes care of the rest of /PreferencePanes too

Comment 5

[Chris Lawson (gone)]

Turns out there's a bunch in the rest of /PreferencePanes, too, along with lots of other minor issues (tabs, inconsistent whitespace usage, etc.). This patch fixes all of it.

If you want me to break this up into separate patches (and bugs) for each pref pane, I can do that, too.

Comment 6

[Jeff Dlouhy]

Comment on attachment 350936 [details] [diff] [review]
fix v1.1, takes care of the rest of /PreferencePanes too

Things look good. I just found some trailing whitespace that was not removed.

(in patch file)

* 44% down line 1175
+  // The three options in the popup contains tags 0-2; set the pref according to the 

* 91% down line 2374
+// Enable and disable Mozilla's popup-blocking feature. We use a combination of 

* 91% down line 2402
+// Enable and disable FlashBlock. When enabled, an icon is displayed and the 

r=me

Comment 7

[Chris Lawson (gone)]

Comment on attachment 350936 [details] [diff] [review]
fix v1.1, takes care of the rest of /PreferencePanes too

I'll go ahead and nominate this for sr now, with the proviso that I spin a patch for checkin that addresses the review comments.

I expect this patch will probably rot when the Flashblock whitelist lands, so that's why I didn't bother spinning a fresh patch for sr.

Comment 8

[Stuart Morgan]

Comment on attachment 350936 [details] [diff] [review]
fix v1.1, takes care of the rest of /PreferencePanes too

Don't remove the blank line in dealloc; there's nothing wrong with separating the call to super.

I'm not sure I agree that we should be changing * spacing in these kind of patches, since we explicitly don't have a set style for that, but I guess it's not worth ripping out of the patch either.

sr=smorgan.

Comment 9

[Chris Lawson (gone)]

Attachment: fix v1.11 (for checkin)

(In reply to comment #8)

> I'm not sure I agree that we should be changing * spacing in these kind of
> patches, since we explicitly don't have a set style for that, but I guess it's
> not worth ripping out of the patch either.

Not trying to be argumentative here, but here's the quote from the style guide:

"We explicitly have no style rule regarding the placement of *. Aim to be consistent within a file in as much as the files are consistent."

Since the spacing in that file (and others, though most of the changes were in Appearance.mm) was generally in favour of (foo*) or |foo* |, and since several of the instances of |foo *| used a tab instead of a space, I figured we might as well go ahead and make the file consistent.

Transferring r/sr+ from the last patch. This one fixes Jeff and Stuart's comments and is ready for checkin.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

To enable the Flashblock whitelist (bug 371895) to land ASAP without any extra patch-wrangling, Chris is going to split this patch up into two parts: one for WebFeatures changes, and one for the rest of PreferencePanes.  We'll land the latter now and the former (plus any unbitrotting) after the whitelist.

Comment 11

[Chris Lawson (gone)]

Attachment: without WebFeatures.* changes [checked in]

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 352372 [details] [diff] [review]
without WebFeatures.* changes [checked in]

Checked this in on cvs trunk.

Comment 13

[Chris Lawson (gone)]

Attachment: WebFeatures.* changes

Here are the WebFeatures.* changes merged with the Flashblock whitelist landing.

Comment 14

[Chris Lawson (gone)]

Comment on attachment 353348 [details] [diff] [review]
WebFeatures.* changes

Smokey wanted someone to take a look at this before it lands, just as a sanity check.

Comment 15

[Christopher Henderson]

Comment on attachment 353348 [details] [diff] [review]
WebFeatures.* changes

- (int)popupIndexForCurrentTabFocusPref {
Brace needs to be put on a new line.

r+ with that change.

Comment 16

[Stuart Morgan]

Comment on attachment 353348 [details] [diff] [review]
WebFeatures.* changes

sr=smorgan

Comment 17

[Eiichi]

Smokey, do you forget to land attachment 353348 [details] [diff] [review]?

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thanks for catching that, Eiichi.  

Checked in the WebFeatures patch on cvs trunk with hendy's change.