Re: Coding style for C++ enums

[Seth Fowler]

I'd honestly prefer to see this discussion drag on a little longer. The
original email was on Friday, so given that most participants on this list
aren't actively debating C++ coding style on the weekend, we've actually
had less than one full working day of discussion on this issue so far.

Let me suggest again (especially since I'm not sure my previous email
reached this list yet) that rather than compromise on the "e" prefix, we
compromise on all caps for values, which is just as readable without
placing even more pressure on our 80-character line length restriction.

Thanks,
- Seth

On Mon, Apr 11, 2016 at 8:00 AM, Kartikaya Gupta  wrote:

> Based on all the feedback so far I think the best compromise is to use
> enum classes with the "e" prefix on the values. As was mentioned, the
> "e" prefix is useful to distinguish between enum values and function
> pointer passing at call sites, but is a small enough prefix that it
> shouldn't be too much of a burden.
>
> I realize not everybody is going to be happy with this but I don't
> want this discussion to drag on to the point where it's a net loss of
> productivity no matter what we end up doing. I'll update the style
> guide.
>
> Thanks all!
>
> kats
>
>
> On Sun, Apr 10, 2016 at 11:43 AM, smaug  wrote:
> > On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
> >>
> >> On Fri, Apr 8, 2016 at 8:35 PM, smaug  wrote:
> >>>
> >>> enum classes are great, but I'd still use prefix for the values.
> >>> Otherwise the values look like types - nested classes or such.
> >>> (Keeping my reviewer's hat on, so thinking about readability here.)
> >>
> >>
> >> In some cases I think it's extremely clear, like this:
> >>
> >>enum class Change { minus, plus };
> >>
> >> whose sole use is as a function parameter (to avoid a boolean
> >> parameter), like so:
> >>
> >>ChangeIndentation(*curNode->AsElement(), Change::plus);
> >>
> >> In cases where it might be mistaken for a class, it might make more
> >> sense to prefix the enum class name with "E".  I don't think this
> >> should be required as a general policy, though, because for some enums
> >> it might be clear enough without it.
> >>
> >
> > Indeed in your case it is totally unclear whether one is passing a
> function
> > pointer or enum value as param.
> > If the value was prefixed with e, it would be clear.
> >
> > Consistency helps with reviewing (and in general code readability) a
> lot, so
> > the less we have optional coding style rules, or
> > context dependent rules, the better.
> >
> >
> >
> >
> >
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Seth Fowler]

> All caps actually makes long names *longer* because you now need to
> add underscores to separate words, rather than using sentence case.
> For example eSentenceCase is the same length as SENTENCE_CASE, but if
> you tack on another word the "e" prefix is shorter. Presumably this
> line-wrapping issue is more pronounced with long names than short
> names, so this is actually an argument against all caps.
>

That's a great point. My focus is mostly on ensuring that the single word
cases (.e.g Color::BLUE), which in my experience are extremely common if
not the vast majority, don't get penalized. I actually think the issue
becomes less pronounced with very long names, because at a certain point
they're so long that extra characters don't matter much anyway in terms of
code formatting impact.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Seth Fowler]

I agree in the extreme with Jeff here. Foo::Bar, in an expression context, 
cannot be anything other than a value. For me, at least, I have never noticed 
this adding any overhead in reviews - it’s immediately obvious what’s 
happening. I suspect that if it *does* add overhead for you, you’ll find that 
overhead disappearing as you get more used to reading such code. Adding a 
prefix like eFoo or kFoo is just visual noise that is actively harmful, because 
in combination with our 80-character limit for lines, it pushes us into more 
awkward code formatting that makes things harder to read in a very real sense. 
Awkward line wrapping like that *definitely* makes it harder for me to review 
code in a very real way.

If we want to have some additional visual indication that we’re looking at an 
enum value, I suggest just using Foo::BAR, which is already in use in some 
parts of the code and which I think is extremely unambiguous.

Thanks,
- Seth


> On Apr 8, 2016, at 3:17 PM, Jeff Gilbert  wrote:
> 
> It's noisy in code you *do* understand, which is the bulk of the code
> we should be dealing with the majority of the time.
> 
> I do not parse this initially as a type because that generally doesn't
> make sense given context.
> Generally the names involved are also unlikely to be types, based on name 
> alone.
> 
> It would also add to our already-pungent code smell.
> 
> On Fri, Apr 8, 2016 at 3:03 PM, Mats Palmgren  wrote:
>> On 2016-04-08 23:03, Jeff Gilbert wrote:
>>> 
>>> Strong preference against eFoo, here. :)
>> 
>> 
>> Strong preference *for* eFoo, here. :)
>> 
>> If I see Bar::Foo anywhere in code I'm not familiar with, my brain
>> is likely to first parse that as a type before realizing that, hmm
>> that doesn't make sense in an expression context, and then I will
>> likely have to lookup what that silly Bar thing is just to be sure.
>> 
>> eFoo is unambiguous and utterly clear.
>> 
>> /Mats
>> 
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Switch to Google C++ Style Wholesale (was Re: Proposal to remove `aFoo` prescription from the Mozilla style guide for C and C++)

[Seth Fowler]

 On Jul 14, 2015, at 8:23 AM, Benjamin Smedberg benja...@smedbergs.us wrote:
 
 * no more ns prefix

Are people still creating new classes with an ’ns’ prefix? Surely this is 
something we can drop right away, at least for new code. Much of the codebase 
already does not use this style. We have namespaces now, after all.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

[Seth Fowler]

I brought this up on IRC, but I think this is worth taking to the list:

Replacing TemporaryRef with already_AddRefed has already caused at least one 
leak because already_AddRefed does not call Release() on the pointer it holds 
if nothing takes ownership of that pointer before its destructor runs. 
TemporaryRef did do that, and this seems like a strictly safer approach.

My question is: why doesn’t already_AddRefed call Release() in its destructor? 
The fact that it doesn’t seems to me to be a profoundly unintuitive footgun.

I don’t think we can trust reviewers to catch this, either. The fact that Foo() 
is declared as:

already_AddRefedBar Foo();

won’t necessarily be obvious from a call site, where the reviewer will just see:

Foo();

That call will leak, and if we don’t happen to hit that code path in our tests, 
we may not find out about it until after shipping it.

I’d prefer that already_AddRefed just call Release(), but if there are 
performance arguments against that, then we should be checking for this pattern 
with static analysis. I don’t think the situation as it stands should be 
allowed to remain for long.

- Seth

 On Jun 30, 2015, at 12:02 PM, Nathan Froyd nfr...@mozilla.com wrote:
 
 Bug 1161627 has landed on inbound, which converts all uses of
 mozilla::TemporaryRef to already_AddRefed and removes TemporaryRef.
 (already_AddRefed moved to MFBT several months ago, in case you were
 wondering about the spreading of XPCOM concepts.)  TemporaryRef was added
 for easier porting of code from other engines, but as it has not been used
 much for that purpose and it led to somewhat less efficient code in places,
 it was deemed a failed experiment.
 
 -Nathan
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Private members of ref counted classes and lambdas

[Seth Fowler]

 On Jun 4, 2015, at 11:17 AM, Daniel Holbert dholb...@mozilla.com wrote:
 
 You may be interested in this thread from a few months back:
 Proposal to ban the usage of refcounted objects inside C++ lambdas in
 Gecko
 https://groups.google.com/d/msg/mozilla.dev.platform/Ec2y6BWKrbM/xpHLGwJ337wJ
 
 (Not sure it arrived at a concrete conclusion, but you may run across
 some pitfalls/suggestions at least. I haven't used lambdas in C++, so I
 won't attempt to directly answer your question.)

My impression was that the conclusion was “refcounted objects are not banned 
inside C++ lambdas” - i.e., no policy change from the status quo - but we need 
to be aware of the pitfalls”. The pitfalls are discussed pretty thoroughly in 
the thread.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Use of 'auto'

[Seth Fowler]

 On Jun 2, 2015, at 1:59 PM, L. David Baron dba...@dbaron.org wrote:
 
 Remember that if 'auto' is forbidden, people might get around it by
 not using a variable at all (and instead repeating the expression)
 rather than by writing the type explicitly on the variable... and
 this doesn't provide type information.

This is a point that’s worth calling out explicitly. When you write this:

Foo(Bar());

You do not have to explicitly specify the return type of Bar(), and if you 
change both the return type of Bar() and the type of Foo()’s first parameter, 
you don’t have to update this callsite.

If you write this:

Baz val = Bar();
Foo(val);

Now you’ve been forced to explicitly write out the return type of Bar(), and if 
you change the type of Foo() and Bar(), you have to update this call site as 
well.

If you write this:

auto val = Bar();
Foo(val);

Then you’ve been able to pull the Bar() call into a separate statement without 
adding any additional coupling - you can change the type of Foo() and Bar() 
without updating this callsite, just as in the first case.

An additional advantage of auto is that |val|’s type is actually the type that 
Bar() returns (modulo references and the like), which is _not_ guaranteed when 
you write out |val|’s type explicitly. If Bar()’s return type changes, but the 
new type has an implicit conversion to Baz, you may end up performing an 
conversion which you didn’t intend, and the compiler won’t catch it. This won’t 
happen with auto.

I think there is definite value in auto’s reduced coupling and increased 
robustness to type changes in some cases. It can result in smaller patches 
which are easier to write, easier to review, bitrot less easily, and are easier 
to uplift, because fewer callsites need to be modified.

Perhaps this will have no bearing on the final rules we adopt - I think 
generally the policy that auto should be used only when the code remains 
readable is something we can all agree on - but I wanted to explicitly call out 
some advantages of auto that I think are worth keeping in mind when deciding 
policy.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A question about do_QueryInterface()

[Seth Fowler]

 On Apr 30, 2015, at 12:09 PM, Joshua Cranmer  pidgeo...@gmail.com wrote:
 
 do_QueryInterface is the equivalent of a type-checked downcast, e.g. 
 (ClassName)foo in Java.  (Regular C++ downcasts are not dynamically 
 type-checked).

do_QueryInterface is, in other words, essentially equivalent to dynamic_cast in 
C++, except that because it’s implemented manually people can do strange things 
if they want to. They almost never do, though, so dynamic_cast is a pretty good 
mental model.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used

[Seth Fowler]

I completely support this. Let the needless verbosity end!

- Seth

 On Apr 27, 2015, at 12:48 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
 
 Right now, our coding style requires that both the virtual and override
 keywords to be specified for overridden virtual functions.  A few things
 have changed since we decided that a number of years ago:
 
 1. The override and final keywords are now available on all of the
 compilers that we build with, and we have stopped supporting compilers that
 do not support these features.
 2. We have very recently gained the ability to run clang-based mass source
 code transformations over our code that would let us enforce the coding
 style [1].
 
 I would like to propose a change to our coding style, and run a mass
 transformation over our code in order to enforce it.  I think we should
 change it to require the usage of exactly one of these keywords per
 *overridden* function: virtual, override, and final.  Here are the
 advantages:
 
 * It is a more succinct, as |virtual void foo() override;| doesn't convey
 more information than |void foo() override;|.
 * It can be easily enforced using clang-tidy across the entire code base.
 * It makes it easier to determine what kind of function you are looking at
 by just looking at its declaration.  |virtual void foo();| means a virtual
 function that is not overridden, |void foo() override;| means an overridden
 virtual function, and |void foo() final;| means an overridden virtual
 function that cannot be further overridden.
 * It allows us to be in sync with the Google C++ Style on this issue [2].
 * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead.
 
 Please let me know what you think!
 
 [1] Facilitated by bug 904572.  Also see bug 1158776 for my first attempt
 at this.
 [2]
 http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
 
 Cheers,
 -- 
 Ehsan
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

We now throttle requestAnimationFrame for offscreen iframes

[Seth Fowler]

Hi all,

Bug 1145439 has landed, which means that we now throttle requestAnimationFrame
for offscreen iframes.

This should give us significant benefits in terms of CPU and energy usage for
pages with iframes that do animation - think HTML5 ads. One test, on
areweflashyet.com, showed a 50% improvement in CPU usage for both the content
and chrome processes when the iframe is scrolled offscreen, compared to the
same situation without throttling.

This behavior is allowed by the spec, but of course spec compatibility and web
compatibility are two different things. (Not to mention there could be bugs!) If
you notice any odd behavior that could be a result of this change, please file
it and block bug 1145439. Thanks!

- Seth

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Can we make try builds default to a different profile than Nightly/Aurora/Beta/Release builds?

[Seth Fowler]

I sometimes ask users to test a try build to verify that the build fixes their 
problem. This is an important tool, especially when the problem is hard to 
reproduce or when the definition of “fix” is a bit nebulous (as it sometimes is 
in performance-related bugs).

However, I don’t want to run the risk of causing users to lose data when they 
test these builds! That’s especially true in light of the fact that we 
sometimes make backwards incompatible changes to on-disk data structures 
between releases (like the recent IndexedDB changes), which could result in 
users being stuck on Nightly if they experiment with a try build.

We do have instructions available for changing profiles, and for backing up and 
restoring your profile, which would mitigate this issue to an extent:

https://support.mozilla.org/en-US/kb/profiles-where-firefox-stores-user-data?redirectlocale=en-USredirectslug=Profiles
 
https://support.mozilla.org/en-US/kb/profiles-where-firefox-stores-user-data?redirectlocale=en-USredirectslug=Profiles

In my opinion, though, these instructions are *way* too complex for 
less-technical users. Making a mistake when following these instructions could 
result in data loss in some cases, as well, which is exactly what I’m trying to 
prevent. And even for more experienced users, backing up and restoring a 
profile or switching profiles manually is tedious as implemented today.

I think one way we could reduce the burden on users would be to just make try 
builds default to a different profile than channel builds.

Can we do this?

Any better ideas?

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can we make try builds default to a different profile than Nightly/Aurora/Beta/Release builds?

[Seth Fowler]

My concern with an approach like this is that it’s opt-in, and developers may 
not necessarily keep this issue in mind every time they offer a build for 
someone to try.

I’ve seen other developers suggest that users experiment with try builds, but 
I’m not sure how widespread it is. Probably suggesting that users try to 
reproduce the problem on Nightly is much more widespread, and is subject to the 
same problems.

How can we reduce users’ risk of data loss in these scenarios, even if a 
developer doesn’t remember to go out of their way to protect them?

Maybe changing the default profile is the wrong approach, especially since it 
doesn’t cover the Nightly case. Maybe instead we need to be doing something 
like automatically backing up the user’s profile every time they switch builds, 
and keeping the last N backups. We’d then need to provide some extremely easy 
UI in Firefox itself (and not some external tool) to restore from one of these 
backups.

Possibly just even adding easy UI to Firefox itself to both backup and restore 
your profile would eliminate the need for the automatic backups, although I 
still vastly prefer the automatic backup idea as it reduces the need for 
developers to remind users to backup their profile.

- Seth

 On Apr 8, 2015, at 12:49 PM, Gavin Sharp ga...@gavinsharp.com wrote:
 
 I think you can get this fairly easily by just changing one of the
 values (Vendor or Name) in build/application.ini such that a different
 profile folder is used.
 
 Gavin
 
 On Wed, Apr 8, 2015 at 12:28 PM, L. David Baron dba...@dbaron.org wrote:
 On Wednesday 2015-04-08 12:08 -0700, Seth Fowler wrote:
 I think one way we could reduce the burden on users would be to just make 
 try builds default to a different profile than channel builds.
 
 Is there a simple patch one could push to change this default, and
 just include on any try pushes where you need this behavior?
 
 I'm a little nervous about making try builds differ from other
 trees, since that just increases the risk of bustage (or bugs in
 testing) that shows up in one place but not the other.
 
 -David
 
 --
 턞   L. David Baron http://dbaron.org/   턂
 턢   Mozilla  https://www.mozilla.org/   턂
 Before I built a wall I'd ask to know
 What I was walling in or walling out,
 And to whom I was like to give offense.
   - Robert Frost, Mending Wall (1914)
 
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform
 

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can we make try builds default to a different profile than Nightly/Aurora/Beta/Release builds?

[Seth Fowler]

 On Apr 8, 2015, at 8:37 PM, Nicholas Alexander nalexan...@mozilla.com wrote:
 
 It's quite important for Firefox for Android that try builds look and behave 
 exactly like Nightly builds.  On Android, it's not really possible to 
 move/copy/duplicate profiles, and it's rocket science to do it across 
 products (Release, Beta, ..., Nightly, custom).  So we need to preserve try 
 builds installing over-top of Nightly builds and vice-versa.  And we accept 
 that some migrations are one-way -- nature of the testing beast, I think.
 
 So whatever we do needs to be Desktop only, or otherwise account for Fennec.

Sounds like yet another reason to build support and UI for this stuff directly 
into the browser.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: mozilla::Pair is now a little more flexible

[Seth Fowler]

 On Mar 15, 2015, at 12:01 PM, Eric Rescorla e...@rtfm.com wrote:
 
 I'm not sure I want to get in a long argument about this, but I'm not 
 convinced
 this is good advice.

I don’t really care what we do - keep in mind, I had nothing to do with 
introducing mozilla::Pair - but I think that we should recommend the use of one 
thing, either std::pair or mozilla::Pair. If we choose to prefer std::pair, we 
should probably remove mozilla::Pair.

If you’re convinced that you have a good case for standardizing on std::pair, 
please file a bug about removing mozilla::Pair, and everyone interested in the 
matter can have a technical discussion about it there.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: mozilla::Pair is now a little more flexible

[Seth Fowler]

 On Mar 15, 2015, at 6:26 PM, Joshua Cranmer  pidgeo...@gmail.com wrote:
 In general, std::pair should be preferred over mozilla::Pair unless you need 
 the empty type optimization.

If that’s the case, perhaps we should rename it to e.g. mozilla::CompactPair? 
It’s current name strongly suggests that it should serve as a replacement for 
std::pair.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: mozilla::Pair is now a little more flexible

[Seth Fowler]

 On Mar 13, 2015, at 6:14 AM, Eric Rescorla e...@rtfm.com wrote:
 
 Sorry if this is a dumb question, but it seems like std::pair is fairly 
 widely used in our
 code base. Can you explain the circumstances in which you think we should be
 using mozilla::Pair instead?

It’s not at all a dumb question. This came up on IRC every time mozilla::Pair 
came up, so a lot of people are wondering about this.

I’m not the person that originally introduced mozilla::Pair, so I wouldn’t 
consider my answer to this question definitive, but I’ll give it a shot anyway.

mozilla::Pair is about avoiding implementation quality issues with std::pair. 
There are two quality issues in particular that have bit us in the past:

- Poor packing, particularly when one of the types stored in the pair has no 
members. In that situation the empty type should consume no space, but 
std::pair implementations sometimes don’t handle that case efficiently.

- Poor or non-existent support for move semantics. I don’t know specifically 
about the case of std::pair, but this is still biting people with other STL 
containers quite recently. Obviously the same code can have significantly 
different performance characteristics in some cases depending on move semantics 
support, so this is a serious problem.

Until we know that we can rely on high quality std::pair implementations 
everywhere, my recommendation would be to always use mozilla::Pair.

- Seth


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

PSA: mozilla::Pair is now a little more flexible

[Seth Fowler]

I thought I’d let everyone know that bug 1142366 and bug 1142376 have added 
some handy new features to mozilla::Pair. In particular:

- Pair objects are now movable. (It’s now a requirement that the underlying 
types be movable too. Every existing use satisfied this requirement.)

- Pair objects are now copyable if the underlying types are copyable.

- We now have an equivalent of std::make_pair, mozilla::MakePair. This lets you 
construct a Pair object with type inference. So this code:

 PairFoo, Bar GetPair() {
   return PairFoo, Bar(Foo(), Bar());
 }

Becomes:

 PairFoo, Bar GetPair() {
   return MakePair(Foo(), Bar());
 }

Nice! This can really make a big difference for long type names or types which 
have their own template parameters.

These changes should make Pair a little more practical to use. Enjoy!

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Chrome removed support for multipart/x-mixed-replace documents. We should too.

[Seth Fowler]

Chrome removed support for multipart/x-mixed-replace main resources in this 
issue:

https://code.google.com/p/chromium/issues/detail?id=249132 
https://code.google.com/p/chromium/issues/detail?id=249132

Here’s their explanation:

 This feature is extremely rarely used by web sites and is the source of a lot 
 of complexity and security bugs in the loader.  UseCounter stats from the 
 stable channel indicate that the feature is used for less than 0.1% of 
 page loads.


They made main resources that use multipart/x-mixed-replace trigger downloads 
instead of being displayed.

The observation that multipart/x-mixed-replace support introduces a lot of 
complexity is absolutely true for us as well. It’s a huge mess.

Looks like this patch landed in Chromium on June 13, 2013 and has stuck since 
then, so removing it has not resulted in a disaster for Chrome. With so few 
people using multipart/x-mixed-replace, and since now both IE and Chrome do not 
support it, I suggest that we remove support for it from the docloader as well.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Reversely iterating nsTArray

[Seth Fowler]

Fiddlesticks! I’d have immediate use for both of those things, that’s why I 
suggested them.

Also, the first suggestion is probably actually less typing than not doing it 
that way. You’re basically writing ‘template typename T’ at the front, where 
you lose some characters, but then you make it up every time you type ’T’ 
instead of ‘uint32_t’ or whatever you’d have used otherwise. =)

- Seth

 On Jan 27, 2015, at 11:47 PM, Kyle Huey m...@kylehuey.com wrote:
 
 On Wed, Jan 28, 2015 at 1:18 PM, Seth Fowler s...@mozilla.com 
 mailto:s...@mozilla.com wrote:
 Sounds good! +1 from me.
 
 Bike shedding:
 
 - Make Range() and ReverseRange() templates, so you can use them with any 
 type that supports the appropriate operators. This also implies removing 
 ‘Integer’ from their names, I think.
 
 - It’d be nice to add a constructor that supports specifying both the 
 beginning and the end points, and another constructor that further supports 
 specifying a stride.
 
 YAGNI*.  Someone can implement those once they need them.
 
 - Kyle
 
 * You aren't gonna need it 

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Reversely iterating nsTArray

[Seth Fowler]

Yes, that’s a good idea! I like this even better than the original proposal.

- Seth

 On Jan 28, 2015, at 1:59 AM, Xidorn Quan quanxunz...@gmail.com wrote:
 
 On Wed, Jan 28, 2015 at 7:39 PM, Cameron McCormack c...@mcc.id.au 
 mailto:c...@mcc.id.au wrote:
 
 Xidorn Quan:
 I asked a question in #developers that what is the best way to reversely
 iterating nsTArray, and there are some suggestions:
 
 For cases where we don’t need to know the index of the array, can we
 support something like:
 
  for (e : array.ReverseIterator()) { ... }
 
 or:
 
  for (e : ReverseIterator(array)) { ... }
 
 (I notice that boost::adaptors::reversed is something like this.)
 
 There’s a danger that it’s not clear what happens if you modify the
 array while using the iterator, but it’s probably no worse than using
 the indexes that would come from ReverseIntegerRange.
 
 
 Hmm. It reminds me that we might not need a ReverseIntegerRange at all. We
 can have a Range, and a Reversed, then combine them to achieve
 ReversedRange. We can make Reversed works with nsTArray as well :)
 
 - Xidorn
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org mailto:dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform 
 https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Reversely iterating nsTArray

[Seth Fowler]

Sounds good! +1 from me.

Bike shedding:

- Make Range() and ReverseRange() templates, so you can use them with any type 
that supports the appropriate operators. This also implies removing ‘Integer’ 
from their names, I think.

- It’d be nice to add a constructor that supports specifying both the beginning 
and the end points, and another constructor that further supports specifying a 
stride.

- Seth


 On Jan 27, 2015, at 6:24 PM, Xidorn Quan quanxunz...@gmail.com wrote:
 
 I asked a question in #developers that what is the best way to reversely
 iterating nsTArray, and there are some suggestions:
 
 tbsaunde uint32_t count = array.Length(); for (uint32_t i = length - 1; i
  length; i--)
 smaug iterate from length() to 1 and index using i - 1
 jcranmer for (uint32_t i = array.Length(); i--  0; ) { }
 
 tbsaunde's method should work fine, but not intuitive. smaug's looks fine
 to me, but could cause some problem if I use i instead of i-1 by mistake.
 jcranmer's... I don't think it is a good idea, anyway. None of them looks
 prefect to me.
 
 
 As we have supported range-based for loop in our code, I purpose that we
 have something like ReverseIntegerRange, and use this with range-based loop
 to iterate the index. So we can use, for example: for (auto i :
 ReverseIntegerRange(array.Length()))
 
 Maybe we can also add IntegerRange to benefit from the integer type
 dedution.
 
 How does it sound?
 
 - Xidorn
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Non-unified builds no longer occurring on central/inbound and friends

[Seth Fowler]

It wouldn’t be true of, say, a mach argument specifying directories that should 
be built non-unified.

Not that it matters nearly as much now that we’ve made the decision not to 
support non-unified builds.

 On Jan 15, 2015, at 3:36 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
 
 On 2015-01-15 5:57 PM, Seth Fowler wrote:
 What’s a bit unfortunate about these approaches is that you can accidentally 
 qref them into your patch. I’ve managed to do so repeatedly.
 
 That's true of any local change, right?
 

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Non-unified builds no longer occurring on central/inbound and friends

[Seth Fowler]

What’s a bit unfortunate about these approaches is that you can accidentally 
qref them into your patch. I’ve managed to do so repeatedly.

- Seth

 On Jan 15, 2015, at 2:52 PM, Mike Hommey m...@glandium.org wrote:
 
 On Thu, Jan 15, 2015 at 02:11:16PM -0500, Benjamin Kelly wrote:
 For what its worth, you can still verify individual directories by changing
 moz.build to use SOURCES instead of UNIFIED_SOURCES.
 
 On Thu, Jan 15, 2015 at 02:21:25PM -0500, Ehsan Akhgari wrote:
 Switch the UNIFIED_SOURCES variable to SOURCES in the moz.build file. That
 still works.
 
 Instead of that, set FILES_PER_UNIFIED_FILE to 1.
 
 Mike
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: multipart/x-mixed-replace images will soon be restricted to MJPEG

[Seth Fowler]

 On Jan 2, 2015, at 7:16 AM, L. David Baron dba...@dbaron.org wrote:
 IMHO, I haven't seen is a weak argument. When we remove features
 that are exposed to the web in some form, it's always a good idea to
 gather some telemetry first so that we know what the actual impact
 will probably be (there is some bias to the Telemetry audience
 already, of course). Let's see that we have data instead of
 assumptions and do not run into surprises. People have been known to
 do crazy things in some corners of the web.
 
 I don't think that' necessary for features that aren't supported
 across other browser engines, which I believe is the case here.

That is true. IE does not support this feature at all.

I completely concur that in general we should gather telemetry before removing 
a feature that’s exposed to the web, but I just don’t see it as adding much 
value for this *particular* case.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

PSA: multipart/x-mixed-replace images will soon be restricted to MJPEG

[Seth Fowler]

Right now, ImageLib provides very general support for multipart/x-mixed-replace 
images. Each part may contain a different image format (we may even switch 
between raster and vector images from one part to the next), individual parts 
may be animated, and each part may have a different size.

This has major costs for us. Supporting existing images changing their size 
requires that layout and content code be aware of multipart/x-mixed-replace 
images, and requires us to use a design that’s suboptimal for the common case 
of MJPEG webcam content. Because these changes in size, type, and animation 
state prevent many invariants we’d like to have in ImageLib from holding, we 
either can’t assert some things we’d like to assert, or the assertions are 
quite tricky to write, which leads to more bugs and more maintenance headaches. 
There’s also just more and uglier code to maintain: over time hacks to 
multipart/x-mixed-replace have popped up in many places in Gecko, inside and 
outside ImageLib, and I’d like to remove them. And despite all of this effort, 
it’s not clear to me that our support is actually very robust, because outside 
of our limited test suite as far as I can tell nobody uses 
multipart/x-mixed-replace images for anything except MJPEG!

I’ve had to change how multipart/x-mixed-replace is implemented in ImageLib to 
support other, more important, features. As part of this refactoring, I plan to 
pare down our support to the minimum required for MJPEG webcams:

(1) The individual parts may only be JPEG images. Other formats will not be 
supported.
(2) All of the parts must have the same intrinsic size as the first part - in 
other words, size changes will not be supported.

In case it’s not clear, there’ll be no effect on other uses of 
multipart/x-mixed-replace elsewhere in the browser. These changes are specific 
to images and ImageLib.

Making these changes will allow me to totally encapsulate MJPEG support inside 
ImageLib. Other code will see an MJPEG as just another type of animated image. 
Virtually all of the ugly consequences of our current multipart/x-mixed-replace 
image support will fall away.

If anyone has any concerns about this change, please let me know. We should be 
fairly safe in doing this since non-MJPEG multipart/x-mixed-replace images are 
essentially never used, and multipart/x-mixed-replace itself is not universally 
supported anyway - IE does not even support MJPEG, much less the more general 
features that we provide. There also does not seem to be any specification that 
requires support for multipart/x-mixed-replace images at all, although HTML5 
does specify multipart/x-mixed-replace support for documents.

Thanks,
- Seth

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: Change the coding style guide to allow an 'o' prefix to indicate out-params

[Seth Fowler]

 On Dec 4, 2014, at 11:28 AM, Robert O'Callahan rob...@ocallahan.org wrote:
 
 I think this would be a slight improvement but the place where I really want 
 out-parameters to be visible is at the caller, not the callee.

Agreed! The simplest way to achieve that in C++, though, is to use pointer 
arguments (so the ‘' operator will generally get used at the callsite). The 
problem is that since pointers are overloaded and have several other uses 
besides out-params, the intention isn’t clear at the method definition. (And 
for longer methods, it can even be unclear in the implementation code.)

The ‘o’ prefix nicely complements the practice of using ‘’ at the callsite, so 
we get clarity at the caller and the callee.

 I think that the cost of introducing this convention and dealing with the 
 inconsistency of it being incompletely applied across the codebase for the 
 foreseeable future probably outweighs the benefits.

Well, converting all existing code to use this convention overnight definitely 
wouldn’t be worth it. And just adding this to the style guide will give us very 
little benefit *right now*. But I think if we add this to the coding style 
guide and convert things gradually in a pragmatic fashion - when we write new 
code, when we touch old code that doesn’t use the convention, and perhaps when 
we encounter an unclear method definition in the course of our work - a year or 
two down the road it may be common enough that we get significant benefits from 
the change.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: Change the coding style guide to allow an 'o' prefix to indicate out-params

[Seth Fowler]

 On Dec 4, 2014, at 11:02 AM, Eric Rescorla e...@rtfm.com wrote:
 In contrast, Seth's suggestion would be an extremely clear indication
 that a parameter is an outparam.
 
 Yes, and because it's just a convention and not compiler enforced it can
 also be wrong.

I don’t know of any realistic, usable way we could enforce this via the 
compiler in C++, though. I agree that in theory the ‘o’ prefix could be wrong, 
but code review provides us some degree of protection against these kinds of 
issues, and I don’t think we should let the perfect be the enemy of the good 
here. (After all, in theory the ‘a’ prefix we use for indicating arguments 
could be wrong, but in practice I have never seen a problem with that.)

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Proposal: Change the coding style guide to allow an 'o' prefix to indicate out-params

[Seth Fowler]

I’d like to change the coding style guide to let us make out-params more 
obvious by using an ‘o’ prefix for their name instead of an ‘a’. For example,

nsresult Modify(int aCount, size_t aSize, char* oResult);

This will make it clear just from the declaration of a function or method which 
parameters are out-params. XPCOM requires us to use out-params a lot, and I’ve 
found that determining when it’s happening often requires me to look at the 
code for the method, especially since we frequently use pointer arguments for 
efficiency or because the argument is optional.

Seems to me like a substantial gain in readability for little or no cost.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Proposal: Standardize initializer list formatting in our coding style guide

[Seth Fowler]

So I noticed that we don’t say anything about initializer list formatting in 
our coding style guide. I’d like to propose that we standardize this formatting:

Foo::Foo(int aBar, char aBaz)
  : mBar(aBar)
  , mBaz(aBaz)
{
  ….
}

In other words, we should list items in the initializer list one per line. Each 
item should be indented two spaces, with the ‘:’ and each ‘,’ vertically 
aligned and a space between the ‘:’/‘,’ and the initializer.

If the entire constructor declaration can fit on one line in 80 characters, we 
could relax this rule. In other words, this seems reasonable to me:

Foo(int aBar) : mBar(aBar) { }

I suspect that none of this is controversial at all, since this is already the 
defacto standard for newer code that I see written in much of the codebase. If 
there’s a reasonable level of consensus, I’ll edit the coding style guide to 
add this rule.

Thanks,
- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: Standardize initializer list formatting in our coding style guide

[Seth Fowler]

Yes, looks like it is! It’s not explicitly spelled out though - someone 
searching for “initializer list”, say, won’t find it. At a minimum it seems 
worth adding an explicit mention in the text.

- Seth

 On Dec 3, 2014, at 2:03 PM, Nicholas Nethercote n.netherc...@gmail.com 
 wrote:
 
 On Thu, Dec 4, 2014 at 8:54 AM, Seth Fowler s...@mozilla.com wrote:
 So I noticed that we don’t say anything about initializer list formatting in 
 our coding style guide. I’d like to propose that we standardize this 
 formatting:
 
 Foo::Foo(int aBar, char aBaz)
  : mBar(aBar)
  , mBaz(aBaz)
 {
  ….
 }
 
 This is already present in the example at
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes:
 
 class MyClass
  : public X // when deriving from more than one class, put each on its own 
 line
  , public Y
 {
 public:
  MyClass()
: mVar(0)
, mVar2(0)
  {
 ...
  }
  ...
 };
 
 Nick

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: power use on Yosemite

[Seth Fowler]

On Nov 3, 2014, at 1:44 PM, rviti...@mozilla.com wrote:

 In particular Facebook, which practically appears in any top 10 list, had 
 (has?) a serious power bug that caused FF to render a hidden spinning wheel. 
 Because of this single bug any power benchmark performed by the press, which 
 was likely going to be based on the top N most visited sites on the web, was 
 likely going to be skewed significantly to our loss.

This is bug 962594; I pushed in a fix last week. Debugging the problem revealed 
that some simple architectural changes could let us solve not only this bug but 
the entire category of related bugs (there are more; see for example bug 
987212) in a very clean way. It will take some time to get the pieces into 
place, but we should be much more efficient in our handling of non-visible 
animated images soon.

Thanks for identifying these problems and pushing to get them fixed, Roberto!

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

PSA: You can now use Maybe with the Auto helpers for Mutex and Monitor

[Seth Fowler]

In bug 1091921, we got support for using Maybe with the Auto helpers for Mutex 
and Monitor - things like MutexAutoLock and MonitorAutoEnter. This supports a 
pattern for optionally acquiring a RAII resource that I first saw used in the 
JavaScript engine, and which I’ve found very useful since. For anyone who 
hasn’t seen it, the basic pattern looks like this:

 MaybeExpensiveRAIIResource resource;
 if (resourceIsNeeded) {
   resource.emplace();
 }

This constructs an ExpensiveRAIIResource on the stack only if 
|resourceIsNeeded| is true.

So bug 1091921 lets us use this pattern with Mutexes and Monitors. I’m sure 
people will find lots of situations where this is useful, and indeed it’s 
already being used in some places.

Any time you have parallelism, though, you need to exercise caution. I 
encourage anyone who wants to use this to start by adding assertions to their 
code like Mutex’s |AssertCurrentThreadOwns| or Monitor’s 
|AssertCurrentThreadIn| anywhere they have methods that expect another method 
to do their synchronization for them. That’s good practice in any case, and 
will help ensure that you don’t make a mistake when using Maybe in this way.

Enjoy!

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The worst piece of Mozilla code

[Seth Fowler]

Thank you Bobby and Josh for all your work to improve ImageLib!

I’m pushing hard on making it better still (with a lot of help from folks like 
Timothy Nikkel and Michael Wu). Hopefully next time we try to decide what the 
worst piece of Mozilla code is, ImageLib won’t be a candidate. =)

- Seth

On Oct 17, 2014, at 12:25 AM, Bobby Holley bobbyhol...@gmail.com wrote:

 On Fri, Oct 17, 2014 at 1:03 AM, Josh Matthews j...@joshmatthews.net
 wrote:
 
 I'm not certain that the image/src/ code is as bad as you make out any
 more. bholley certainly is no longer the expert there; I took over a bunch
 of his work to clean it up a year or two ago, and Seth is the benevolent
 dictator now and has done some good cleanup work on it as well.
 
 
 Yes, I stepped down as an ImageLib peer a little under three years ago. And
 when comparing the initial states of ImageLib and XPConnect when I
 inherited them, ImageLib was a dream to work with. ;-)
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: using LLDB to debug Gecko

[Seth Fowler]

Really nice Cameron, thanks for doing this!

Now if only I could get LLDB to work with GUD in emacs. If anyone has managed 
that, I'd love to hear about it. There was some elisp in the LLDB repo at one 
time but last I checked it had been removed and the last version that existed 
didn't work with the current version of emacs.

- Seth

On Jan 21, 2014, at 2:09 AM, Cameron McCormack c...@mcc.id.au wrote:

 Hi,
 
 For those of you who are using LLDB -- either by choice, or against your will 
 after upgrading to OS X 10.9 -- I've added some Gecko-specific LLDB 
 functionality to an .lldbinit file in the tree.  I've mainly started with 
 porting across the commands from the in-tree .gdbinit that were still useful.
 
 You can read about it here:
 
  http://mcc.id.au/blog/2014/01/lldb-gecko
 
 Let me know if you have any ideas for further commands and type summaries we 
 could add!
 
 Cameron
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Mozilla style guide issues, from a JS point of view

[Seth Fowler]

Chiming in a little late:

On Jan 6, 2014, at 6:35 PM, Joshua Cranmer  pidgeo...@gmail.com wrote:

 I find prefixing member variables with 'm' to be useful, although I dislike 
 using it in POD-ish structs where all the members are public.

Fully agreed, and IMO the style guide should be changed to include this.

 The use of 'a' for arguments is where I am least consistent, especially as I 
 extremely dislike it being used for an outparam return value

That also bothers me. I'd support adding an 'o' prefix for outparams.

 I've never found much use for the 's', 'g', and 'k' prefixes, although that 
 may just as well be because I've never found much use for using those types 
 of variables in the first place (or when I do, it's because I'm being 
 dictated by other concerns instead, e.g., type traits-like coding or C++11 
 polyfilling).

I don't see the use in distinguishing between 's' and 'g'. They're both 
potentially dangerous globally-shared data, and that's the most important 
information the reader should know. (Except if they're immutable, of course, 
but then presumably they'd have a 'k' prefix.) We could classify them both as 
'g', but considering the number of bugs I've seen stemming from unprotected 
access to these kinds of variables, I would support a more verbose and 
distinctive prefix like 'unsafe'.

- Seth

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Mozilla style guide issues, from a JS point of view

[Seth Fowler]

On Jan 7, 2014, at 3:22 PM, Cameron McCormack c...@mcc.id.au wrote:

 Patrick McManus wrote:
 Typically I have to choose between
  1] 80 columns
  2] descriptive and non-abbreviated naming
  3] displaying a logic block without scrolling
 
 to me, #1 is the least valuable.
 
 Thoroughly agree.

I also agree with this.

Perhaps a compromise might be that we should aim for 80 columns except for 
cases where this would hurt readability, but have a hard limit at 100 columns. 
Given how verbose C++ is, an 80 column limit (for me) often ends up hurting 
readability more than it helps.

- Seth

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: try: -p all considered harmful?

[Seth Fowler]

On Sep 28, 2012, at 6:28 PM, Boris Zbarsky bzbar...@mit.edu wrote:

 The point is that the patches that _do_ need to land urgently are blocked on 
 lack of resources because people are wasting too many cycles with unnecesary 
 try pushes.

This sounds like a notion of priority might be helpful.

- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform