Upcoming changes to our C++ Coding Style

2018-11-21 Thread Ehsan Akhgari 
(In case the formatting doesn't survive transit, a copy of this note can be
found here:
https://docs.google.com/document/d/1CTaWucldHxEri5BUB1kL4faF4prwPlBa31yHFd-l8uc
.)


TL;DR

To improve developer productivity, we are planning to a) start
automatically formatting all Gecko C++ code with clang-format, and b)
switch to the Google C++ style
 and retire the Mozilla
C/C++ coding style
.
These changes are being tested and verified with a small part of the
codebase before rolling out to the entire tree.

Background

Mozilla has been moving towards using more automated tools for checking and
rewriting code. For example, eslint  (JS), checkstyle
 (Java), flake8
 (Python) are now Tier-1 in the CI. We
also deployed C/C++ static analysis as part of the development process.

In parallel, tooling to reformat C & C++ is now used in a wide variety of
projects like Chromium, LLVM and MongoDB. Mozilla has historically had a
coding style but it was never really enforced through tooling. In addition,
the Mozilla style is incomplete and used only in our projects; by contrast
the Google style is very complete, well supported in automated tooling, and
widely used by projects inside and outside of Google such as Chromium,
Tensorflow, and MySQL.

Because of the lack of checks and automation, we now have significant
inconsistencies in the style of our codebase.  On top of that, we have been
spending a lot of time talking about and adjusting whitespace. This wastes
human time when writing, reviewing, and discussing code.

Last but not least, being able to automatically format whitespace in our
C/C++ code is helpful for other types of automated rewrites so we can fix
wide-scale problems in our codebase using static analysis tools.  Right now
such fixes are difficult or impossible because of inconsistencies in the
Mozilla C++ coding style and lack of support in clang-format and other
tools. Addressing this issue is the first step in unblocking those future
improvements.

Goals

During patch authoring, code reviews, and mailing list discussions, Mozilla
developers have traditionally spent a lot of time on minor issues that
could easily be fixed through automated processes. Our Engineering
Operations and Product Integrity groups have helped to improve developer
productivity in this area through tools such as static analysis
, linting via Phabricator
reviewbot , and codebase
reformats

using clang-format. We will build on this foundation by applying coding
style checks and automated rewrites throughout the authoring process
(pre-commit, commit, review, landing).

Details

We’re announcing the following changes to our coding style and its
enforcement through our commit policy:


   1.

   We will check conformance with coding style rules upon commit, review,
   and landing so that issues can be easily addressed or fixed through
   automation. The preference will be to enforce style issues as early as
   possible (before landing) to avoid late surprises.

   2.

   We will migrate to the Google C++ coding style to encourage more
   consistent code and re-use a wider array of existing tools.  As part of
   this migration, we will retire the existing Mozilla C/C++ Coding Style. The
   intention behind retiring the current Mozilla style is to just stop
   maintaining it as an active coding style going forward, but the
   documentation for this coding style will be kept intact.



   1.

   We will automatically enforce restrictions on formatting of whitespace
   (such as indentation and braces). For stylistic features other than that
   (such as naming of functions and #include order), Google C++ style will be
   permitted but not initially enforced, and consistency with surrounding code
   should take precedence.


These changes have been approved both by Firefox senior engineering
leadership and the Firefox Technical Leadership Module
.  This work
is being tracked in bug 1188202
.

When?

On Monday this week, we pushed a patch
 to an integration
branch to rewrite a section of the tree (dom/media) in the Google C++ style
using clang-format.  This first step is intended to be a smoke test step,
designed to test the process of doing the rewrite and test the waters in a
small controlled environment with a small team of developers who have
volunteered to help us test this change. We haven’t found any major
surprises yet.

Assuming that

Re: Signals in Firefox

2018-11-21 Thread Mike Hommey 
On Wed, Nov 21, 2018 at 10:22:38AM -0500, Nathan Froyd wrote:
> On Wed, Nov 21, 2018 at 4:45 AM David Teller  wrote:
> > What is our policy on using Unix signals on Firefox? I am currently
> > reviewing a patch by external contributors that involves inotify's
> > signal API, and I assume it's a bad idea, but I'd like to ask around
> > first before sending them back to the drawing board.
> 
> I don't think we have a policy, per se; certainly we already have uses
> of signals in the JS engine's wasm implementation and the Gecko
> profiler.  But in those cases, signals are basically the only way to
> do what we want.  If there were alternative ways to accomplish those
> tasks besides signals, I think we would have avoided signals.
> 
> inotify looks like it has a file descriptor-based interface which
> seems perfectly usable.  Not being familiar with inotify beyond
> reading http://man7.org/linux/man-pages/man7/inotify.7.html, is there
> a reason to prefer the signal interface versus the file descriptor
> interface?  We use the standard gio/gtk event loop, so hooking up the
> returned file descriptor from inotify_init should not be onerous.
> widget/gtk/nsAppShell.cpp even contains some code to crib from:
> 
> https://searchfox.org/mozilla-central/source/widget/gtk/nsAppShell.cpp#275-281

I'd go one step further. We use Gio from libglib, is there a reason not
to use the GFileMonitor API, which wraps inotify?

https://developer.gnome.org/gio/stable/GFileMonitor.html

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

Re: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Boris Zbarsky 

On 11/21/18 2:22 PM, Daniel Veditz wrote:

"opener" doesn't exist


It does in WebKit's proposed changes and in our implementation of them.


You'd specify a target
name other than "_blank" to indicate it's a context you care about


This seems backwards.  What matters is whether the context should care 
about _you_, not whether you care about it.  If you want to open a 
guaranteed-new context that can then communicate with you, how should 
you do that, exactly?  Making up target names (which you have no way to 
guarantee are unique) seems like a bad approach to that.


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

Re: Signals in Firefox

2018-11-21 Thread David Teller 
Thanks for the suggestions.

Given that they are on an academic deadline and they have already
implemented the feature using straight inotify and a monitor thread, I'd
favor a lesser refactoring with just removing the signals.

Cheers,
 David

On 21/11/2018 22:06, Mike Hommey wrote:
> On Wed, Nov 21, 2018 at 10:22:38AM -0500, Nathan Froyd wrote:
>> On Wed, Nov 21, 2018 at 4:45 AM David Teller  wrote:
>>> What is our policy on using Unix signals on Firefox? I am currently
>>> reviewing a patch by external contributors that involves inotify's
>>> signal API, and I assume it's a bad idea, but I'd like to ask around
>>> first before sending them back to the drawing board.
>>
>> I don't think we have a policy, per se; certainly we already have uses
>> of signals in the JS engine's wasm implementation and the Gecko
>> profiler.  But in those cases, signals are basically the only way to
>> do what we want.  If there were alternative ways to accomplish those
>> tasks besides signals, I think we would have avoided signals.
>>
>> inotify looks like it has a file descriptor-based interface which
>> seems perfectly usable.  Not being familiar with inotify beyond
>> reading http://man7.org/linux/man-pages/man7/inotify.7.html, is there
>> a reason to prefer the signal interface versus the file descriptor
>> interface?  We use the standard gio/gtk event loop, so hooking up the
>> returned file descriptor from inotify_init should not be onerous.
>> widget/gtk/nsAppShell.cpp even contains some code to crib from:
>>
>> https://searchfox.org/mozilla-central/source/widget/gtk/nsAppShell.cpp#275-281
> 
> I'd go one step further. We use Gio from libglib, is there a reason not
> to use the GFileMonitor API, which wraps inotify?
> 
> https://developer.gnome.org/gio/stable/GFileMonitor.html
> 
> Mike
> 
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Daniel Veditz 
On Wed, Nov 21, 2018 at 7:08 AM Alex Gaynor  wrote:

> Do we have any sense of how large the breakage will be, and do we have any
> docs for developers who are impacted? (I assume rel=opener is the fix?)
>

"opener" doesn't exist, and we shouldn't need it. You'd specify a target
name other than "_blank" to indicate it's a context you care about -- only
"_blank" gets the new behavior.

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

Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Andrea Marchesini 
*Summary*: WebKit is experimenting an interesting feature: target=_blank on
anchor and area elements implies ref=noopener.
https://trac.webkit.org/changeset/237144/webkit/

*Bug*: https://bugzilla.mozilla.org/show_bug.cgi?id=1503681

*Link to standard*: https://github.com/whatwg/html/issues/4078

*Platform coverage*: everywhere.

*Estimated or target release*: 66 - after 1 cycle with this feature enabled
in nightly and beta only.

*Preference behind which this will be implemented*
:dom.targetBlankNoOpener.enabled

*Is this feature enabled by default in sandboxed iframes?* yes.

*DevTools bug*: no special support for devtools. Maybe we could dispatch an
Intervention report via Reporting API.

*Do other browser engines implement this?* Only safari at the moment:
https://trac.webkit.org/changeset/237144/webkit

*web-platform-tests*: none, yet, but I can convert my mochitest in a WPT
easily.

*Is this feature restricted to secure contexts?* no needs.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Signals in Firefox

2018-11-21 Thread David Teller 
Dear platformers,

What is our policy on using Unix signals on Firefox? I am currently
reviewing a patch by external contributors that involves inotify's
signal API, and I assume it's a bad idea, but I'd like to ask around
first before sending them back to the drawing board.

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

Re: Signals in Firefox

2018-11-21 Thread Nathan Froyd 
On Wed, Nov 21, 2018 at 4:45 AM David Teller  wrote:
> What is our policy on using Unix signals on Firefox? I am currently
> reviewing a patch by external contributors that involves inotify's
> signal API, and I assume it's a bad idea, but I'd like to ask around
> first before sending them back to the drawing board.

I don't think we have a policy, per se; certainly we already have uses
of signals in the JS engine's wasm implementation and the Gecko
profiler.  But in those cases, signals are basically the only way to
do what we want.  If there were alternative ways to accomplish those
tasks besides signals, I think we would have avoided signals.

inotify looks like it has a file descriptor-based interface which
seems perfectly usable.  Not being familiar with inotify beyond
reading http://man7.org/linux/man-pages/man7/inotify.7.html, is there
a reason to prefer the signal interface versus the file descriptor
interface?  We use the standard gio/gtk event loop, so hooking up the
returned file descriptor from inotify_init should not be onerous.
widget/gtk/nsAppShell.cpp even contains some code to crib from:

https://searchfox.org/mozilla-central/source/widget/gtk/nsAppShell.cpp#275-281

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

Re: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Anne van Kesteren 
On Wed, Nov 21, 2018 at 4:08 PM Alex Gaynor  wrote:
> Do we have any sense of how large the breakage will be, and do we have any
> docs for developers who are impacted? (I assume rel=opener is the fix?)

The "fix" would be to use target=someuniquename.

And I don't think there's data, other than Safari folks not having
heard anything yet.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Alex Gaynor 
I'm very excited about this -- in my experience very few developers know
about the dangers of target=_blank.

Do we have any sense of how large the breakage will be, and do we have any
docs for developers who are impacted? (I assume rel=opener is the fix?)

Yay!

Alex

On Wed, Nov 21, 2018 at 3:29 AM Andrea Marchesini 
wrote:

> *Summary*: WebKit is experimenting an interesting feature: target=_blank on
> anchor and area elements implies ref=noopener.
> https://trac.webkit.org/changeset/237144/webkit/
>
> *Bug*: https://bugzilla.mozilla.org/show_bug.cgi?id=1503681
>
> *Link to standard*: https://github.com/whatwg/html/issues/4078
>
> *Platform coverage*: everywhere.
>
> *Estimated or target release*: 66 - after 1 cycle with this feature enabled
> in nightly and beta only.
>
> *Preference behind which this will be implemented*
> :dom.targetBlankNoOpener.enabled
>
> *Is this feature enabled by default in sandboxed iframes?* yes.
>
> *DevTools bug*: no special support for devtools. Maybe we could dispatch an
> Intervention report via Reporting API.
>
> *Do other browser engines implement this?* Only safari at the moment:
> https://trac.webkit.org/changeset/237144/webkit
>
> *web-platform-tests*: none, yet, but I can convert my mochitest in a WPT
> easily.
>
> *Is this feature restricted to secure contexts?* no needs.
> ___
> 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: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Ehsan Akhgari 
On Wed, Nov 21, 2018 at 3:55 PM Boris Zbarsky  wrote:

> On 11/21/18 2:22 PM, Daniel Veditz wrote:
> > "opener" doesn't exist
>
> It does in WebKit's proposed changes and in our implementation of them.
>
> > You'd specify a target
> > name other than "_blank" to indicate it's a context you care about
>
> This seems backwards.  What matters is whether the context should care
> about _you_, not whether you care about it.  If you want to open a
> guaranteed-new context that can then communicate with you, how should
> you do that, exactly?
>

Would it be OK if the answer to that question be "use window.open()"?

It would really be nice if browsers could converge on a definition of what
these conditions are going to be, and I would be really happy the more
restrictive the cases we hand out window.opener references can be.  Having
a programmatic way (i.e. an API call) as the only access point to this
functionality has the nice side benefit that if the browser wants to
provide interventions in the creation of opener references, we would have
only a single API call to worry about, rather than having to keep track of
things like tabs opened from links.  In fact, one motivation beyond this
intent thread is one future anti-tracking intervention that we've been
discussing.

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

Re: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Boris Zbarsky 

On 11/21/18 11:50 PM, Ehsan Akhgari wrote:

Would it be OK if the answer to that question be "use window.open()"?


Can one do noreferrer with window.open()?

Also, if your thing doing the navigation is a , not , then 
window.open is pretty hard to use for that.  Then again, target="_blank"> is not that common...


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

Re: Intent to implement: implicit ref=noopener for target=_blank on anchor and area elements

2018-11-21 Thread Ehsan Akhgari 
On Wed, Nov 21, 2018 at 11:55 PM Boris Zbarsky  wrote:

> On 11/21/18 11:50 PM, Ehsan Akhgari wrote:
> > Would it be OK if the answer to that question be "use window.open()"?
>
> Can one do noreferrer with window.open()?
>

Yes, by passing 'noopener' in the features argument:
https://html.spec.whatwg.org/multipage/window-object.html#apis-for-creating-and-navigating-browsing-contexts-by-name:disowned-its-opener
.


> Also, if your thing doing the navigation is a , not , then
> window.open is pretty hard to use for that.  Then again,  target="_blank"> is not that common...
>

That's true, this wouldn't cover the form submission use case.

Which reminds me, it's impossible to block opener reference creation upon
form submission right now as far as I can tell.  This is actually a bug in
the spec.  <
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm:the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name>
calls into "rules for choosing a browsing context" passing only two
arguments, omitting the third one (noopener) <
https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name
>.

I wonder if it makes sense to make a similar change here, to make  imply noopener behaviour and then if that proves to be Web
compatible, propose to change the spec to pass false there?

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