Upcoming on Bugzilla: reorganized & redesigned bug page

[Kohei Yoshino]

Hello,


As you know, bugzilla.mozilla.org has evolved to 
meet the needs of various projects, and the current bug detail page seems cluttered 
with so many fields. Making changes to a familiar UI may confuse some of you, but we 
feel like it’s time to get it improved for a more streamlined workflow. Here’s a 
brief summary of our plan:


Phase 1: Reorganized bug fields— The bug fields will soon be reorganized 
to place similar fields in 
related groups. For example, the Dependencies, Regressions, URL and See Also fields will 
move to the new References section. See this screenshot 
for how the new bug 
detail page will look like.


Phase 2: Redesigned bug page— We’ll be introducing a 2-column layout 
to make it easier to have 
a look at both bug fields and comments at the same time. The new design will also be 
optimized for mobile, so you can join a conversation comfortably on your phone!


Phase 3: Further UX enhancements— There are still many more things to do: 
simplifying the timeline, making it easier to edit bug ID and other complicated 
fields, and ultimately implementing real-time updates. Bugzilla will be pretty 
modernized at this point.


So don’t be surprised to one day see something different on your bug! Aside from regular 
BMO change logs being posted on the tools-bmo list 
, we’ll keep you posted on this 
dev-platform list with any major change that may affect you. Wanna chat? Join the #bmo 
channel on Mozilla Slack.


Thanks!


Kohei, for the Bugzilla team

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

Re: Intent to ship: Add wildcard to Access-Control-Expose-Headers, Access-Control-Allow-Methods, and Access-Control-Allow-Headers

[Kershaw Jang]

Hi Ehsan,

Thanks for correcting this. Yes, this should be shipped in Firefox 70.

Regards,
Kershaw

On Sat, Jul 20, 2019 at 2:40 AM Ehsan Akhgari 
wrote:

> Thanks Kershaw for fixing this.  I was curious why you expect this to ship
> in Firefox 71 but not 70?
>
> On Tue, Jul 16, 2019 at 1:56 PM Kershaw Jang  wrote:
>
>>  Hi everyone,
>>
>> *Summary*:
>> Allow more wildcards in CORS headers when used without credential.
>> So, the new syntax for these http headers is:
>> Access-Control-Expose-Headers = #field-name / wildcard
>> Access-Control-Allow-Methods  = #method / wildcard
>> Access-Control-Allow-Headers  = #field-name / wildcard
>>
>> *Bug*:
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1309358
>>
>> *Link to standard*:
>> https://fetch.spec.whatwg.org/#http-new-header-syntax
>>
>> *Platform coverage*:
>> all platforms.
>>
>> *Estimated or target release*:
>> Firefox 71.
>>
>> *Preference behind which this will be implemented*:
>> No preference.
>>
>> *DevTools bug*:
>> No.
>>
>> *Do other browser engines implement this?*
>> Yes, both Blink and WebKit support this feature.
>>
>> *web-platform-tests*:
>> We already have existing wpt tests for this.
>>
>> *Is this feature restricted to secure contexts?*
>> No.
>>
>> Please note that I already landed this patch in nightly since I was not
>> aware of that I should send this email first. If anyone has any concerns,
>> feel free to file another bug to back this out.
>>
>> Thanks,
>> Kershaw
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>
>
> --
> Ehsan
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: accumulating most JS scripts' data as UTF-8, then directly parsing as UTF-8 (without inflating to UTF-16)

[Jeff Walden]

On 7/20/19 4:41 PM, Nicholas Nethercote wrote:
> This is excellent news. Do you have any measurements showing perf effects?

Perf changes should have first become visible with the landing of bug 1554362.  
There was one quasi-automatically-reported perf improvement, on the Sheets 
raptor test:

https://bugzilla.mozilla.org/show_bug.cgi?id=1554362#c19

If our tests do enough script-heavy things for the long-run memory improvements 
to be clearly visible, or if they run enough scripts that improvements to 
parsing are visible in big-picture takes, other improvements should be visible. 
 I dropped a bunch of time on trying to finagle perfherder into revealing more 
changes a month ago, but I didn't get anywhere and ended up dropping it.

The overall change is assuredly not Sheets-specific in any fashion, tho, so 
certainly more than just Sheets is affected by this.



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

Re: non-const reference parameters in new and older code

[Jeff Gilbert]

I could grudgingly get on board with that, though I feel that there
are sort of two levels of mutability I use: casual and essential.
Essential is protected by constness, whereas casual is sort of
everyday minor changes, but changes I don't want to allow in `const`
code, thus don't want `mutable`. It's subjective, but has been useful
to handle mutability trade-offs. (since we don't have multiple
constnesses, Views and factories are sometimes alternatives)

I really really like the non-null annotation reference provides, since
pointers (especially in headers without diving into source code) are
ambiguously nullable.

On Mon, Jul 22, 2019 at 7:29 AM David Teller  wrote:
>
> I believe in least surprise for the caller of an API. This seems to
> match with the Google style, as you describe it: any parameter which may
> be mutated in any manner should be passed as pointer, rather than reference.
>
> Cheers,
>  David
>
> On 22/07/2019 08:43, Karl Tomlinson wrote:
> > https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> > has a simple rule to determine when reference parameters are
> > permitted:
> > "Within function parameter lists all references must be const."
> > This is consistent with Mozilla's previous coding style:
> > "Use pointers, instead of references for function out parameters,
> > even for primitive types." [1]
> > However, across Gecko there are different interpretations of what
> > "out" parameter means.
> >
> > The Google style considers a parameter to be an out parameter if
> > any of its state may be mutated by the callee.
> > In some parts of Gecko, a parameter is considered an out parameter
> > only if the callee might make wholesale changes to the state of
> > parameter.  Well before the announcement to switch to Google style,
> > this interpretation was discussed in 2017 [2], with subsequent
> > discussion around which types were suitable as non-const reference
> > parameters.
> >
> > I'm asking how should existing surrounding code with some
> > different conventions affect when is it acceptable to follow
> > Google style for reference or pointer parameters?
> >
> ___
> 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: NotNull and pointer parameters that may or may not be null

[Jeff Gilbert]

FWIW RefPtr already behaves as MaybeNull for assert-on-deref, making
them functionally Maybe. (though they don't represent a contract
that it's nullable)

For non-outvar-args, I tend to use references to document non-null,
though I do so even for many non-const uses. (balancing what is and
isn't `mutable`/non-const is hard. Casual vs important side-effects)
References obviously should not be null, which makes the non-null
contract pretty obvious, though there's no assert for `*(T*)foo`. Like
you say though, null-derefs are self-asserting. :)

Outvars are always pointers for me, non-null unless documented
otherwise. (I should probably be using NotNull, but again,
self-asserting)

I agree that it's often not clear when args are actually nullable,
with the exception of dom::Nullable coming out of bindings code. Maybe
we should move Nullable to mfbt? I like the idea of annotating these
contracts. (Possibly also a NullableRefPtr? I occasionally use
Maybe> for this, but the double-deref it requires is sort of
awkward)

If we want a pure annotation, we could always `#define NULLABLE`!

On Mon, Jul 22, 2019 at 12:00 AM Karl Tomlinson  wrote:
>
> Google style requires pointers for parameters that may be mutated
> by the callee, which provides that the potential mutation is
> visible at the call site.  Pointers to `const` types are
> permitted, but recommended when "input is somehow treated
> differently" [1], such as when a null value may be passed.
>
> Comments at function declarations should mention
> "Whether any of the arguments can be a null pointer." [2]
>
> Some Gecko code has instead sometimes used a different
> reference/pointer convention to identify parameters which can be
> null.  With Google style, there is the question of whether to use
> a different mechanism to indicate whether or not pointer parameter
> may be null.
>
> If a method signature wants a pointer to an object of a particular
> type, then I would usually expect to need to provide a pointer to
> an object, unless documented otherwise.  I wonder whether in the
> general case, possibly-null pointers are more the exception than
> the rule?  Does use of NotNull add much value if most pointer
> parameters are assumed not null, but not explicitly wrapped?
>
> I wonder whether the reverse kind of wrapping would be more
> useful?  A MaybeNull type was previously contemplated but
> considered more complex and less obviously useful [3].
>
> A NotNull wrapping of the parameter would catch nulls when set in
> debug builds.  A MaybeNull wrapping could catch nulls when
> dereferenced.  Assertions don't really seem necessary to me
> because we are almost certain to find out anyway if code that
> expects non-null receives a null pointer.  Behavior is actually
> undefined, but I don't recall ever seeing a small null offset
> dereference bug rated worse than sec-low.  The primary benefit of
> a wrapping would be to annotate expectations and perhaps even
> require more though about what those are.
>
> mozilla::NotNull was introduced in 2016 [4] but hasn't seen much
> adoption.  I found very few header files that use NotNull
> consistently, but looked at the ones with most usage.
>
> At the time that NotNull usage was introduced to
> image/DecoderFactory.h [5], it was applied to eight parameters.
> Two pointer parameters did not get wrapped with NotNull.  Of those
> the `const char* aMimeType` parameter on the public method was
> assumed non-null by the implementation.  The other parameter was
> for a private method that permits `RasterImage* aImage` to be null.
> Since then, two additional `IDecodingTask** aOutTask` pointer
> parameters have been added, each of which is assumed non-null.
>
> intl/Encoding.h [6] has seven uses of NotNull in return types.
> In the C++ API exposed, I found five instances of pointers in
> return values where NotNull was not applied: each used nullptr as
> a sentinel.
>
> It seems that NotNull is particularly useful for Encoding because
> sometimes there may be a sentinel value and other times not.
> A MaybeNull alternative would be at least as useful in this case.
>
> I'm inclined to skip NotNull unless there is a special reason to
> use it.  Anyone have reason to feel differently?
>
> [1]
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> [2]
> https://google.github.io/styleguide/cppguide.html#Function_Comments
> [3]
> https://groups.google.com/d/msg/mozilla.dev.platform/_jfnwDvcvN4/rl6c3TcFAQAJ
> [4]
> https://groups.google.com/d/msg/mozilla.dev.platform/6M_afibWhBA/MCmrYktaBgAJ
> [5]
> https://searchfox.org/mozilla-central/rev/f6528fc8520d47e507877da3dda798ab57385be2/image/DecoderFactory.h#93
> [6]
> https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/intl/Encoding.h
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___

Re: cross-language LTO enabled on nightly for all platforms

[Bobby Holley]

Thanks! Nathan also clarified on IRC that this runs only on the "shippable"
variant of CI builds.

On Mon, Jul 22, 2019 at 8:58 AM Nathan Froyd  wrote:

> On Mon, Jul 22, 2019 at 11:45 AM Bobby Holley 
> wrote:
> > Can you confirm which types of builds enable this? Does --enable-release
> turn it on?
>
> If you really want to build this locally, you can add `export
> MOZ_LTO=cross` in your mozconfig.  `--enable-release` does not
> automatically enable LTO (cross-language or otherwise).
>
> Thanks,
> -Nathan
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: cross-language LTO enabled on nightly for all platforms

[Nathan Froyd]

On Mon, Jul 22, 2019 at 11:45 AM Bobby Holley  wrote:
> Can you confirm which types of builds enable this? Does --enable-release turn 
> it on?

If you really want to build this locally, you can add `export
MOZ_LTO=cross` in your mozconfig.  `--enable-release` does not
automatically enable LTO (cross-language or otherwise).

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

Re: cross-language LTO enabled on nightly for all platforms

[Bobby Holley]

Thanks for the hard work getting this over the line!

Can you confirm which types of builds enable this? Does --enable-release
turn it on?

On Mon, Jul 22, 2019 at 6:56 AM Nathan Froyd  wrote:

> Hi all,
>
> We now have link-time optimization (LTO) between Rust and C++ code
> enabled on Nightly for all platforms (bug 1486042 [1]).  There have
> been some concerns about potential slowdowns when crossing the C++ <=>
> Rust boundary due to non-inlineable function calls, and Stylo needed
> to implement some gnarly code copying between C++ and Rust to obtain
> good performance.  With cross-language LTO enabled, such concerns and
> hacks should become a thing of the past.
>
> It is worth explicitly noting that enabling this feature does not seem
> to have made much difference on our performance tests: if you are
> doing performance work, you should *not* need to enable this feature.
> (Which is a good thing, as it massively increases the amount of time
> needed to link libxul...)  The primary benefit at the moment is not
> having to implement code on both the C++ and Rust side as Stylo did
> and to eliminate concerns that crossing the language boundary induces
> a performance issue.
>
> Please note that if you attempt to build your own cross-language
> LTO-enabled binary on OS X, your binary will be broken in interesting
> ways due to bugs in Xcode.  Work to error out much earlier in that
> configuration is happening in bug 1563204 [2].
>
> There were a number of people involved in this effort: Michael
> Woerister did the Rust-side work [3].  David Major did the initial
> landing for Win64 only [4].  Michael fixed some issues in bindgen that
> prevented Win32 from working correctly [5].  And Mike Hommey tied up
> loose ends and pushed things over the line while I was away for the
> last two weeks.  Thanks to all involved!
>
> -Nathan
>
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1486042
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1563204
> [3] https://github.com/rust-lang/rust/issues/49879
> [4] https://bugzilla.mozilla.org/show_bug.cgi?id=1512723
> [5] https://github.com/rust-lang/rust-bindgen/pull/1558
> ___
> 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: non-const reference parameters in new and older code

[David Teller]

I believe in least surprise for the caller of an API. This seems to
match with the Google style, as you describe it: any parameter which may
be mutated in any manner should be passed as pointer, rather than reference.

Cheers,
 David

On 22/07/2019 08:43, Karl Tomlinson wrote:
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> has a simple rule to determine when reference parameters are
> permitted:
> "Within function parameter lists all references must be const."
> This is consistent with Mozilla's previous coding style:
> "Use pointers, instead of references for function out parameters,
> even for primitive types." [1]
> However, across Gecko there are different interpretations of what
> "out" parameter means.
> 
> The Google style considers a parameter to be an out parameter if
> any of its state may be mutated by the callee.
> In some parts of Gecko, a parameter is considered an out parameter
> only if the callee might make wholesale changes to the state of
> parameter.  Well before the announcement to switch to Google style,
> this interpretation was discussed in 2017 [2], with subsequent
> discussion around which types were suitable as non-const reference
> parameters.
> 
> I'm asking how should existing surrounding code with some
> different conventions affect when is it acceptable to follow
> Google style for reference or pointer parameters?
> 
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

[desktop] Bugs logged by Desktop Release QA in the last 7 days

[Mihai Boldan]

Hello,

Here's the list of new issues found and filed by the Desktop Release QA 
team in the last 7 days.
Additional details on the team's priorities last week, as well as the 
plans for the current week are available at: https://tinyurl.com/y49fgq3g.

Bugs logged by Desktop Release QA in the last 7 days:

Firefox: Preferences
NEW - https://bugzil.la/1565998 - Cookies from Local files lack 
path/name in the Manage Cookies modal


Firefox: Bookmarks & History
* NEW - https://bugzil.la/1566745 - Bookmark dropdown - creating 
multiple New folders pushes tree out of bounds


Firefox: Toolbars and Customization
* NEW - https://bugzil.la/1566786 - Entering Customize menu while reload 
button is in transition makes the icon disappear


Firefox: Theme
* NEW - https://bugzil.la/1567408 - [Win 8][Dark Theme] - Increase 
contrast for the Add new tab button


Core: DOM: Core & HTML
* NEW - https://bugzil.la/1567382 - Droping html-data inside the Drop 
Stuff Here section returns a Not Matching result


Core: Graphics
* NEW - https://bugzil.la/1567468 - [Aarch64] Drawboard is black on 
tinkercad 3D Design
* NEW - https://bugzil.la/1567478 - [Aarch64] Blinking artifacts on 
gamearter webgl aircraft game


Toolkit: Add-ons Manager
* NEW - https://bugzil.la/1566071 - Addon can be re-enabled inside the 
about:addons section after disable->deletion
* NEW - https://bugzil.la/1567432 - [Win] Using High Contrast makes 
ellipsis (3-dot) icon disappear in about:addons

page

Toolkit: Password Manager
* RESOLVED WONTFIX - https://bugzil.la/1566335 - Import Saved logins 
close button (x) lacks padding


Cloud Services: Server: Firefox Accounts
* NEW - https://bugzil.la/1566767 - Firefox Accounts - add option to 
disconnect multiple devices/sessions at once


Firefox Build System: General
* NEW - https://bugzil.la/1567158 - Can't locate Firefox Nightly if 
installed using .pkg file and having multiple firefox builds installed


mozilla/blurts-server
* OPEN - https://github.com/mozilla/blurts-server/issues/1132 - 
Hyperlink for “ I just found out I’m in a data breach. What do I do 
next?” takes you to a different part of the site
* OPEN - https://github.com/mozilla/blurts-server/issues/1133 - Firefox 
Monitor logo is missing from the confirmation email


This is available as a Bugzilla bug list as well: 
https://tinyurl.com/y5zttzxm.


Regards,
Mihai Boldan










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

cross-language LTO enabled on nightly for all platforms

[Nathan Froyd]

Hi all,

We now have link-time optimization (LTO) between Rust and C++ code
enabled on Nightly for all platforms (bug 1486042 [1]).  There have
been some concerns about potential slowdowns when crossing the C++ <=>
Rust boundary due to non-inlineable function calls, and Stylo needed
to implement some gnarly code copying between C++ and Rust to obtain
good performance.  With cross-language LTO enabled, such concerns and
hacks should become a thing of the past.

It is worth explicitly noting that enabling this feature does not seem
to have made much difference on our performance tests: if you are
doing performance work, you should *not* need to enable this feature.
(Which is a good thing, as it massively increases the amount of time
needed to link libxul...)  The primary benefit at the moment is not
having to implement code on both the C++ and Rust side as Stylo did
and to eliminate concerns that crossing the language boundary induces
a performance issue.

Please note that if you attempt to build your own cross-language
LTO-enabled binary on OS X, your binary will be broken in interesting
ways due to bugs in Xcode.  Work to error out much earlier in that
configuration is happening in bug 1563204 [2].

There were a number of people involved in this effort: Michael
Woerister did the Rust-side work [3].  David Major did the initial
landing for Win64 only [4].  Michael fixed some issues in bindgen that
prevented Win32 from working correctly [5].  And Mike Hommey tied up
loose ends and pushed things over the line while I was away for the
last two weeks.  Thanks to all involved!

-Nathan

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1486042
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1563204
[3] https://github.com/rust-lang/rust/issues/49879
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=1512723
[5] https://github.com/rust-lang/rust-bindgen/pull/1558
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: Blocking Worker/SharedWorker with non-JS MIME type

[Boris Zbarsky]

On 7/22/19 6:22 AM, Tom Schuster wrote:

This was also discussed at https://github.com/whatwg/html/issues/3255.
It seems like Chrome does NOT plan on shipping this at the moment.


Does "at the moment" mean they are open to shipping it in the future if 
we ship it and don't run into web compat issues, or that they are not 
planning to ship at all?  What are Safari's plans here?  What is the 
proposed path to interop?



We didn't dig too deeply into this data, but one idea was
that a lot of worker scripts are actually 404 text/html error pages.


This is something telemetry could easily measure, yes?  Only record 
worker script types for responses that would actually get processed 
(i.e. not HTTP 4xx responses).  Is there a reason not to do that before 
shipping this change?


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

Intent to ship: Blocking Worker/SharedWorker with non-JS MIME type

[Tom Schuster]

In Firefox 70 we plan to start blocking Worker and SharedWorker
scripts served with non-JavaScript MIME types. We have similarly been
blocking importScripts() since version 67.

Bug to turn on by default: https://bugzilla.mozilla.org/show_bug.cgi?id=1523706
Pref: security.block_Worker_with_wrong_mime

This was also discussed at https://github.com/whatwg/html/issues/3255.
It seems like Chrome does NOT plan on shipping this at the moment.

However we are optimistic that we can ship this, because in our data
there are more importScripts with a wrong MIME type than worker
scripts. We didn't dig too deeply into this data, but one idea was
that a lot of worker scripts are actually 404 text/html error pages.

Telemetry: https://mzl.la/2y805sN (Compare worker_load with importScript_load)

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

NotNull and pointer parameters that may or may not be null

[Karl Tomlinson]

Google style requires pointers for parameters that may be mutated
by the callee, which provides that the potential mutation is
visible at the call site.  Pointers to `const` types are
permitted, but recommended when "input is somehow treated
differently" [1], such as when a null value may be passed.

Comments at function declarations should mention
"Whether any of the arguments can be a null pointer." [2]

Some Gecko code has instead sometimes used a different
reference/pointer convention to identify parameters which can be
null.  With Google style, there is the question of whether to use
a different mechanism to indicate whether or not pointer parameter
may be null.

If a method signature wants a pointer to an object of a particular
type, then I would usually expect to need to provide a pointer to
an object, unless documented otherwise.  I wonder whether in the
general case, possibly-null pointers are more the exception than
the rule?  Does use of NotNull add much value if most pointer
parameters are assumed not null, but not explicitly wrapped?

I wonder whether the reverse kind of wrapping would be more
useful?  A MaybeNull type was previously contemplated but
considered more complex and less obviously useful [3].

A NotNull wrapping of the parameter would catch nulls when set in
debug builds.  A MaybeNull wrapping could catch nulls when
dereferenced.  Assertions don't really seem necessary to me
because we are almost certain to find out anyway if code that
expects non-null receives a null pointer.  Behavior is actually
undefined, but I don't recall ever seeing a small null offset
dereference bug rated worse than sec-low.  The primary benefit of
a wrapping would be to annotate expectations and perhaps even
require more though about what those are.

mozilla::NotNull was introduced in 2016 [4] but hasn't seen much
adoption.  I found very few header files that use NotNull
consistently, but looked at the ones with most usage.

At the time that NotNull usage was introduced to
image/DecoderFactory.h [5], it was applied to eight parameters.
Two pointer parameters did not get wrapped with NotNull.  Of those
the `const char* aMimeType` parameter on the public method was
assumed non-null by the implementation.  The other parameter was
for a private method that permits `RasterImage* aImage` to be null.
Since then, two additional `IDecodingTask** aOutTask` pointer
parameters have been added, each of which is assumed non-null.

intl/Encoding.h [6] has seven uses of NotNull in return types.
In the C++ API exposed, I found five instances of pointers in
return values where NotNull was not applied: each used nullptr as
a sentinel.

It seems that NotNull is particularly useful for Encoding because
sometimes there may be a sentinel value and other times not.
A MaybeNull alternative would be at least as useful in this case.

I'm inclined to skip NotNull unless there is a special reason to
use it.  Anyone have reason to feel differently?

[1]
https://google.github.io/styleguide/cppguide.html#Reference_Arguments
[2]
https://google.github.io/styleguide/cppguide.html#Function_Comments
[3]
https://groups.google.com/d/msg/mozilla.dev.platform/_jfnwDvcvN4/rl6c3TcFAQAJ
[4]
https://groups.google.com/d/msg/mozilla.dev.platform/6M_afibWhBA/MCmrYktaBgAJ
[5]
https://searchfox.org/mozilla-central/rev/f6528fc8520d47e507877da3dda798ab57385be2/image/DecoderFactory.h#93
[6]
https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/intl/Encoding.h
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

non-const reference parameters in new and older code

[Karl Tomlinson]

https://google.github.io/styleguide/cppguide.html#Reference_Arguments
has a simple rule to determine when reference parameters are
permitted:
"Within function parameter lists all references must be const."
This is consistent with Mozilla's previous coding style:
"Use pointers, instead of references for function out parameters,
even for primitive types." [1]
However, across Gecko there are different interpretations of what
"out" parameter means.

The Google style considers a parameter to be an out parameter if
any of its state may be mutated by the callee.
In some parts of Gecko, a parameter is considered an out parameter
only if the callee might make wholesale changes to the state of
parameter.  Well before the announcement to switch to Google style,
this interpretation was discussed in 2017 [2], with subsequent
discussion around which types were suitable as non-const reference
parameters.

I'm asking how should existing surrounding code with some
different conventions affect when is it acceptable to follow
Google style for reference or pointer parameters?

The announcement of changes to Mozilla style [3] says
> Google C++ style will be permitted but not initially enforced,
> and consistency with surrounding code should take precedence.
and
> we have decided that by allowing the usage of the Google C++
> Style in new areas, we are taking an incremental step towards
> that ultimate goal [Google C++ Style)] by allowing (not
> requiring) people to write code immediately after our switch
> that is consistent with our newly adopted coding style [...]
> The intention here is not that people intermix Google C++ Style
> and Mozilla Style code on a per function or even a per file
> basis but rather that they be free to do so when starting new,
> self-contained work, or to convert an entire module.

Parameters cross module boundaries.

For example, WebIDL bindings use non-const references and pointers
to distinguish between C++ representations of non-nullable and
nullable interface and callback types [4].
Classes of many modules need to break from Google style to
provide appropriate calling conventions to WebIDL bindings.
Should this mean that such classes need to be consistent with the
WebIDL binding convention when declaring other methods just
because they need to implement some methods for WebIDL bindings?

If conventions should traverse modules boundaries in this way,
then very little new code would follow the new style.  Is that the
expectation or is it preferable to aim to follow the new style
where possible?

A recent post [5] appears to propose a slightly different
interpretation:
> In other words, we should default to using Google C++ style when
> doing so would not be massively more disruptive or inconsistent
> than the alternative. So we're not boiling the ocean over mFoo,
> but preferring the explicit integer types and citing the Google
> style guide is a reasonable thing to do.

Declaring new methods to follow Google parameter conventions is
not in general massively more disruptive or even inconsistent than
the alternative.  Even when working with similar types, the mix of
pointer/reference usage and the existence of other situations
where references are not suitable [6] means that conversions
between references and pointers are already regularly required.

I can imagine reasons why the reference/pointer convention between
nullable and non-nullable types is something that may have been
particularly useful for implementing WebIDL interfaces but that
distinction is not necessarily the best reference/pointer
convention in general, and so choices made for WebIDL bindings
should not necessarily influence other code.  Consistency is good,
but that argument can be applied in either direction.

There has been similar conflict between reference parameter
conventions in Blink.  Google and Chromium styles forbid mutable
reference parameters.  WebKit introduced mutable reference
parameters to help in wiping out phantom null checks [7] but this
change was not completed (in Blink at least).  Code that has moved
from Chromium into Blink has not changed style [8] and
blink/core/layout/ng has chosen Google style rather than legacy
WebKit style so as to highlight side effects in callees [9].
People using mutable reference parameters are typically pleased
with their choice, and people using only const reference
parameters are pleased with their choice [10].  Blink style was
recently (March 20) changed to accept either style, to reflect
reality [11].  No single choice between the conventions has yet
been made for Blink.

When reading over a call site and surrounding code, I find
knowledge of mutability of arguments necessary first for high
level understanding.  Missing a mutation often means completely
misunderstanding.  Whether arguments may be null or not is usually
a detail secondary in understanding of the function of the code.

Disclaimer: I was asked by a reviewer to post about this topic,
but this post has my own