I also agree we should move ahead with the proposed style/structure
changes.
Change: dropping aFoo prefixed arguments, always bracing
conditionals, "use strict" (JS only)
Where/When: New files and piecemeal on edited files. Any changes in
an existing file should be in a separate patch (patch 0). Do not
intermingle substantive and style changes in the same patch. I really
do not want to see bugs filed with patch 0 only. We still need to
focus on getting real feature work completed and not just curb our
coding OCD.
------------------------------------------------------------------------
I'm all for the newer style (I've definitely been doing this in
my JS), but I don't want us to spend too much time futzing around
updating existing code. +1 to using the new style in new files,
and I'd support "part 0" clean-up patches in bugs where
developers are going to be changing a lot of an existing file
anyway (and that's also a good way to audit the exiting code for
unnoticed problems).
Margaret
----- Original Message -----
> From: "Richard Newman" <[email protected]
<mailto:[email protected]>>
> To: [email protected]
<mailto:[email protected]>
> Sent: Thursday, February 27, 2014 9:01:37 AM
> Subject: On coding standards
>
> Over the past year or three, desktop JS has been moving away
from the older
> "toolkit-style" standard: dropping aFoo prefixed arguments,
always bracing
> conditionals, "use strict", etc.
>
> We've also done this to an extent in mobile: new Java code
tends not to have
> aFoo arguments, and tends not to use unbraced conditionals.
>
> The services code in Fennec (about 1/4 of the codebase) already
goes all the
> way, using 'modern' Java coding conventions throughout.
>
>
> I don't expect any pushback on always bracing conditionals, and
we're already
> dropping aFoo, so:
>
> I propose eliminating the use of mFoo/sBar in the rest of
Fennec. Here's why.
>
>
> 1. It doesn't add value, when it's wrong it's misleading, and
apparently
> nobody pays enough attention to it. For example, GeckoProfile
contains this
> line, which wesj wrote in 3c552ee3aa5e, bnicholson reviewed,
and nobody
> spotted for at least 6 months:
>
> private static GeckoProfile mGuestProfile = null;
>
> (I'm fixing that as part of Bug 707123.)
>
> If wesj and bnicholson -- two of our most awesome crew -- can
make this
> mistake, and nobody spotted it or nobody was affected by it,
then why
> bother?
>
>
> 2. It's unnecessarily verbose, considering that it adds no
value. At the
> point of definition we already know it's static: it has the
word 'static'
> next to it. The best you can do is match that, and the worst is
to get it
> wrong! At the point of use, Java already has conventions for
being explicit
> about what you're doing: Classname.staticMember, this.member.
>
>
> 3. If you get access wrong, you'll typically get a compiler
error. You can't
> access instance members from a static context. You can access
statics in
> scope (within a class definition) without qualification, and
that's OK. If
> you're worried about getting that wrong, use ClassName.foo
instead. The only
> other case that builds -- accessing a static member via an
instance -- is
> accompanied by a compiler warning and a yellow flag in Eclipse.
>
>
> 4. Speaking of Eclipse: IDEs make it obvious what everything
is, using text
> styling, colors, hints, tooltips, and the other affordances
available. I
> acknowledge that this doesn't apply to the process of code
review, but I'll
> be so bold as to assert that the set of bugs that might be
caught by saying
> "waitaminute, that's static!" is small, uncommon, and probably
overlaps with
> the set of bugs that requires you to *understand* the class
definition, not
> just pattern-match.
>
> (No disrespect to pattern-match reviewing: I do it all the time.)
>
>
>
> My proposal is to ditch this style for new files, fix up old
files as we go
> (just as we do for bracing and broken indents), and enjoy our
cheerful new
> world.
>
> Thoughts?
>
> -R
