Re: Visual Studio Code integration with `clangd` for C/C++ development

[Botond Ballo]

On Tue, Sep 15, 2020 at 6:55 PM Jean-Yves Avenard 
wrote:

> This broke several features for me (and I use VSCode all the time). One in
> particular was the inability to switch between code and header (Ctrl-K
> Ctrl-O).
>

clangd supports this, but it's under a custom command name (as it's not
part of the Language Server Protocol).

If you go to Keyboard Shortcuts, and search for the command
"clangd.switchheadersource", you can bind your preferred shortcut to it.


> Finding symbol definition broke under many cases too.
>

This is one we'd have to examine on a case-by-case basis. Some of them may
be upstream issues in clangd, but some may be issues in our setup (e.g.
related to unified builds).

Andi, do you have a suggestion for how to track these? Should we encourage
people to file Bugzilla tickets for them, which we can then triage (and if
appropriate, we can file upstream clangd issues)?

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

Re: Visual Studio Code integration with `clangd` for C/C++ development

[Jean-Yves Avenard]

Hi.

I don't know if that's related, but when recently VSCode started to show
clangd as a recommended extension for the project. Which I installed.

This broke several features for me (and I use VSCode all the time). One in
particular was the inability to switch between code and header (Ctrl-K
Ctrl-O).
Finding symbol definition broke under many cases too.

So I ended up uninstalling the clangd extension.

Is this a known issue?

Jean-Yves


On Fri, Sep 11, 2020 at 3:47 PM Andi-Bogdan Postelnicu 
wrote:

> First of all you will need to get VSCode and mozilla repo. Besides that
> you have the `./mach bootstrap` environment that downloads everything that
> you need like Python, Node, LLVM and the rest of packages that we need to
> build Firefox.
> Once the bootstrap environment is setup is very easy to have VSCode
> configured, just use:
>
> `./mach ide vscode`
>
> The solution will be generated under `.vscode` directory and a temporary
> compilation database will be created in the obj directory. The IDE will be
> automatically open and you will be prompted with a list of extension that
> are going to be installed, this list is situated in
> `.vscode/extensions.json`.
>
> This should be it, after the last step you should have a fully working
> IDE, no matter the platform, Win64, MaxOS64 or Linux64.
>
> Hope this sheds some light,
> ANdi
>
> > On 10 Sep 2020, at 19:48, mhoye  wrote:
> >
> >
> > This is amazing work.
> >
> > For the sake of new user documentation, I have a question: From scratch,
> for me to get from zero to VS Code Community edition to "I have everything
> I need to work on Firefox", what is the consensus around the components or
> options I need to pick to get myself close to an ideal VS setup? I haven't
> revisited this on a clean machine in a long time, and this seems like as
> good a time as any to update that information.
> >
> > I think the answer is, Python, Node, C/C++ desktop and mobile... have I
> missed any, and are there other workload options that would help on first
> setup?
> >
> > - mhoye
> >
> > -- Original Message --
> > From: "Andrew Halberstadt" 
> > To: "Andi-Bogdan Postelnicu" 
> > Cc: "dev-platform" 
> > Sent: 2020-09-10 12:29:48 PM
> > Subject: Re: Visual Studio Code integration with `clangd` for C/C++
> development
> >
> >> This is great, thanks Andi!
> >>
> >> Are there any plans to introduce a `mach lint` integration as well? Or
> is
> >> that what is already being used for "inline parsing errors with limited
> >> auto-fix hints"?
> >>
> >>
> >> On Thu, Sep 10, 2020 at 12:20 PM Andi-Bogdan Postelnicu <
> a...@mozilla.com>
> >> wrote:
> >>
> >>> TLDR: VSCode users can type `./mach ide vscode` in order to get code
> >>> completion, reference navigation, refactoring, reformatting, etc.
> >>>
> >>> Hello all,
> >>>
> >>> VSCode  is a multi-platform
> >>> open-source programming editor developed by Microsoft and volunteers.
> It is
> >>> partly built using source-code components but also uses proprietary
> >>> Microsoft code. It has support for many programming languages using
> >>> extensions.
> >>> In the past we had a minimal
> >>>  configuration
> setup
> >>> in the tree that reflected the basic extensions
> >>> 
> >>> that
> >>> should be used and also some tasks
> >>> 
> that can
> >>> be triggered from the editor.
> >>>
> >>> Now, we significantly improved that!
> >>>
> >>> Starting with Bug 1656740
> >>> , we’ve added
> >>> comprehensive support for C/C++with the help of the `clangd` extension
> for
> >>> Firefox development. Leveraging the `clang` toolchain compiler we now
> have
> >>> support in the IDE for:
> >>>
> >>>1.
> >>>
> >>>Syntax highlighting;
> >>>2.
> >>>
> >>>IntelliSense with comprehensive code completion and suggestion;
> >>>
> >>>
> >>>
> >>>1.
> >>>
> >>>Go-to definition and Go-to declaration;
> >>>2.
> >>>
> >>>Find all references
> >>>3.
> >>>
> >>>Open type hierarchy;
> >>>4.
> >>>
> >>>Rename symbol, all usages of the symbol will be renamed, including
> >>>declaration, definition and references;
> >>>5.
> >>>
> >>>Code formatting, based on `clang-format` that respects our coding
> >>>standard using the `.clang-format` and `.clang-format-ignore` files.
> >>> Format
> >>>can be performed on an entire file or on a code selection;
> >>>6.
> >>>
> >>>Inline parsing errors with limited auto-fix hints;
> >>>
> >>>
> >>>1.
> >>>
> >>>Basic static-code analysis using `clang-tidy` and our list of
> enabled
> >>>checkers. (This is still in progress not all checkers are supported
> by
> >>>`clangd`);
> >>>
> >>>
> >>> This new eco-system for code development is supported on all 

Re: A new testing policy for code landing in mozilla-central

[Brian Grinstead]

> On Sep 15, 2020, at 11:44 AM, Andrew Halberstadt  wrote:
> 
> On Tue, Sep 15, 2020 at 12:28 PM Andrew McCreight 
> wrote:
>> I don't know that tests being an official requirement relieves the burden
> of guilt of asking for tests, as everybody already knows that tests are
> good and that you should always write tests
> 
> I think this is largely true at Mozilla, but we all have our lapses from
> time to time. Thankfully since we all know this, a tiny nudge is all that
> we need.
> 
>> Separately, one category of fixes I often deal with is fixing leaks or
> crashes in areas of the code I am unfamiliar with. I can often figure out
> the localized condition that causes the problem and correct that, but I
> don't really know anything about, say, service workers or networking to
> write a test. Do I need to hold off on landing until I can find somebody
> who is willing and able to write a test for me, or until I'm willing to
> invest the effort to become knowledgeable enough in an area of code I'm
> unlikely to ever look at again? Neither of those feel great to me.
> 
> This sounds like an argument in favour of putting the burden of exceptions
> on the reviewer. Authors can submit without a test (ideally calling out
> it's because they don't know how) and then the reviewer can either:
> A) Explain how to add tests for said patch, or
> B) Knowing that writing a test would be difficult, grant them an exception

Or (C) the reviewer could write the test themselves and land it in a separate 
commit alongside the patch, marking the original patch as 
`testing-exception-elsewhere`.

We definitely don't want to discourage writing drive-by / “good samaritan” 
patches where you are fixing a bug in an area outside of your normal work. This 
came up a few times in discussions and is one of the reasons I prefer 
reviewer-specified instead of author-specified exceptions.

> If a reviewer finds themselves explaining / granting exceptions often, it
> could be a signal that the test infrastructure in that area needs
> improvement.
> Btw I think your points are all valid, I guess we'll have to see how it
> turns out in practice. Hopefully we can adjust the process as needed based
> on what we learn from it.

For sure. I’m happy to discuss any specific pain points you run into with the 
policy.

> - Andrew
> 
> On Tue, Sep 15, 2020 at 12:28 PM Andrew McCreight 
> wrote:
> 
>> On Tue, Sep 15, 2020 at 8:03 AM Brian Grinstead 
>> wrote:
>> 
>>> (This is a crosspost from firefox-dev)
>>> 
>>> Hi all,
>>> 
>>> We’re rolling out a change to the review process to put more focus on
>>> automated testing. This will affect you if you review code that lands in
>>> mozilla-central.
>>> 
>>> ## TLDR
>>> 
>>> Reviewers will now need to add a testing Project Tag in Phabricator when
>>> Accepting a revision. This can be done with the “Add Action” → “Change
>>> Project Tags” UI in Phabricator. There's a screenshot of this UI at
>>> https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ.
>>> 
>> 
>> I of course think having more tests is a good thing, but I don't like how
>> this specific process places all of the burden of understanding and
>> documenting some taxonomy of exceptions on the reviewer. Reviewing is
>> already a fairly thankless and time consuming task. It seems like the way
>> this is set up is due to the specifics of our how reviewing process is
>> implemented, so maybe there's no way around it.
>> 
>> Also, contrary to what ahal said, I don't know that tests being an official
>> requirement relieves the burden of guilt of asking for tests, as everybody
>> already knows that tests are good and that you should always write tests.
>> The situation is different from the linters, as the linters will fix almost
>> all issues automatically, so asking somebody to fix linter issues isn't
>> much of a burden at all. I guess it is impossible to make testing better
>> without somebody directly pressuring somebody, though.
>> 
>> My perspective might be a little distorted as I think a lot of the patches
>> I write would fall under the exceptions, either because they are
>> refactoring, or I am fixing a crash or security issue based on just a stack
>> trace.
>> 
>> Separately, one category of fixes I often deal with is fixing leaks or
>> crashes in areas of the code I am unfamiliar with. I can often figure out
>> the localized condition that causes the problem and correct that, but I
>> don't really know anything about, say, service workers or networking to
>> write a test. Do I need to hold off on landing until I can find somebody
>> who is willing and able to write a test for me, or until I'm willing to
>> invest the effort to become knowledgeable enough in an area of code I'm
>> unlikely to ever look at again? Neither of those feel great to me.
>> 
>> Another testing difficulty I've hit a few times this year (bug 1659013 and
>> bug 1607703) are crashes that happen when starting Firefox with unusual
>> command line or 

Re: A new testing policy for code landing in mozilla-central

[Andrew Halberstadt]

On Tue, Sep 15, 2020 at 12:28 PM Andrew McCreight 
wrote:
> I don't know that tests being an official requirement relieves the burden
of guilt of asking for tests, as everybody already knows that tests are
good and that you should always write tests

I think this is largely true at Mozilla, but we all have our lapses from
time to time. Thankfully since we all know this, a tiny nudge is all that
we need.

> Separately, one category of fixes I often deal with is fixing leaks or
crashes in areas of the code I am unfamiliar with. I can often figure out
the localized condition that causes the problem and correct that, but I
don't really know anything about, say, service workers or networking to
write a test. Do I need to hold off on landing until I can find somebody
who is willing and able to write a test for me, or until I'm willing to
invest the effort to become knowledgeable enough in an area of code I'm
unlikely to ever look at again? Neither of those feel great to me.

This sounds like an argument in favour of putting the burden of exceptions
on the reviewer. Authors can submit without a test (ideally calling out
it's because they don't know how) and then the reviewer can either:
A) Explain how to add tests for said patch, or
B) Knowing that writing a test would be difficult, grant them an exception

If a reviewer finds themselves explaining / granting exceptions often, it
could be a signal that the test infrastructure in that area needs
improvement.

Btw I think your points are all valid, I guess we'll have to see how it
turns out in practice. Hopefully we can adjust the process as needed based
on what we learn from it.
- Andrew

On Tue, Sep 15, 2020 at 12:28 PM Andrew McCreight 
wrote:

> On Tue, Sep 15, 2020 at 8:03 AM Brian Grinstead 
> wrote:
>
> > (This is a crosspost from firefox-dev)
> >
> > Hi all,
> >
> > We’re rolling out a change to the review process to put more focus on
> > automated testing. This will affect you if you review code that lands in
> > mozilla-central.
> >
> > ## TLDR
> >
> > Reviewers will now need to add a testing Project Tag in Phabricator when
> > Accepting a revision. This can be done with the “Add Action” → “Change
> > Project Tags” UI in Phabricator. There's a screenshot of this UI at
> > https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ.
> >
>
> I of course think having more tests is a good thing, but I don't like how
> this specific process places all of the burden of understanding and
> documenting some taxonomy of exceptions on the reviewer. Reviewing is
> already a fairly thankless and time consuming task. It seems like the way
> this is set up is due to the specifics of our how reviewing process is
> implemented, so maybe there's no way around it.
>
> Also, contrary to what ahal said, I don't know that tests being an official
> requirement relieves the burden of guilt of asking for tests, as everybody
> already knows that tests are good and that you should always write tests.
> The situation is different from the linters, as the linters will fix almost
> all issues automatically, so asking somebody to fix linter issues isn't
> much of a burden at all. I guess it is impossible to make testing better
> without somebody directly pressuring somebody, though.
>
> My perspective might be a little distorted as I think a lot of the patches
> I write would fall under the exceptions, either because they are
> refactoring, or I am fixing a crash or security issue based on just a stack
> trace.
>
> Separately, one category of fixes I often deal with is fixing leaks or
> crashes in areas of the code I am unfamiliar with. I can often figure out
> the localized condition that causes the problem and correct that, but I
> don't really know anything about, say, service workers or networking to
> write a test. Do I need to hold off on landing until I can find somebody
> who is willing and able to write a test for me, or until I'm willing to
> invest the effort to become knowledgeable enough in an area of code I'm
> unlikely to ever look at again? Neither of those feel great to me.
>
> Another testing difficulty I've hit a few times this year (bug 1659013 and
> bug 1607703) are crashes that happen when starting Firefox with unusual
> command line or environment options. I think we only have one testing
> framework that can test those kinds of conditions, and it seemed like it
> was in the process of being deprecated. I had trouble finding somebody who
> understood it, and I didn't want to spend more than a few hours trying to
> figure it out for myself, so I landed it without testing. To be clear, I
> could reproduce at least one of those crashes locally, but I wasn't sure
> how to write a test to create those conditions. Under this new policy, how
> do we handle fixing issues where there is not always a good testing
> framework already? Writing a test is hopefully not too much of a burden,
> but obviously having to develop and maintain a new testing framework could

Re: A new testing policy for code landing in mozilla-central

[Brian Grinstead]

> On Sep 15, 2020, at 8:48 AM, Jonathan Kew  wrote:
> 
> On 15/09/2020 16:03, Brian Grinstead wrote:
> 
> > We’re rolling out a change to the review process to put more focus on 
> > automated testing. [...]
> 
> As others have said, it's great that we're setting a clearer policy here - 
> thanks!
> 
> One thing did strike me as a little surprising, and could perhaps be 
> clarified:
> 
> > * testing-exception-other: Commits where none of the defined exceptions 
> > above apply but it should still be landed. This should be scrutinized by 
> > the reviewer before using it - consider whether an exception is actually 
> > required or if a test could be reasonably added before using it. This 
> > requires a comment from the reviewer explaining why it’s appropriate to 
> > land without tests.
> 
> The point that caught my eye here was that it explicitly requires the 
> explanatory comment to be *from the reviewer*. I would have thought that in 
> many cases it would make sense for the *patch author* to provide such an 
> explanatory comment (which the reviewer would of course be expected to 
> scrutinize before granting r+). Is that scenario going to be considered 
> unacceptable?
> 

I think that’s OK and something we can clarify in the text if needed. It’s 
helpful for authors to explain the reasoning if they think an exception should 
be applied (essentially, making a request for a particular exception). We'd 
need to decide if either (a) the reviewer grants the request by adding the 
exception with a link pointing to the authors comment, (b) the reviewer grants 
the request by adding the exception without a comment,  or (c) the author adds 
the exception at the same time they make the request, and the reviewer 
implictly grants the request by not removing it. As the policy is written, (a) 
is already fine.

If based on usage feedback this is inconvenient and not providing value I’m 
open to adjusting it accordingly. Though did want to note that I’d be more 
interested in optimizing the workflow for `testing-exception-elsewhere` which 
also requires a comment (as it is expected / commonly used, like in Stacks) 
than `testing-exception-other` which should be used less often and require more 
scrutiny.

Brian

> Thanks,
> 
> JK
> 
> ___
> 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: Cookie SameSite=lax by default and SameSite=none only if secure

[Daniel Veditz]

On Tue, Sep 15, 2020 at 10:13 AM Michael Reeps  wrote:

> Thank you for the prompt response to my email. I guess I interpreted the
> standard to mean only when the cookie was intended for cross-site delivery,
> which these are not:
>

If the bug carries the SameSite=None attribute how could the browser
possibly know the cookie is only used samesite? In fact it would appear the
cookie has gone out of its way to announce it is NOT only used on the same
site. The "reject" language in the spec seems pretty clear cut.

> I see this message with nearly all of my Adobe Analytics cookies, Google
> Analytics, and a number of others, and am going to be reliant on those
> vendors to address this issue. The folks at Adobe Client Care were
> completely unaware of Mozilla's interpretation when I reported it, which
> differs from Chrome's. Can you give any insight as to when "soon" is in
> "will be soon rejected"?
>

That we differ from Chrome is concerning. The main reason we're following
the spec so carefully is in order to be compatible with the web's 800lb
gorilla. As it happens I'll be in a meeting with the spec author later
today; I'll ask him about Chrome's implementation of that part, and whether
the spec needs an update.

I don't know how soon -- better question for Andrea (original poster) who
implemented this. I suspect it's "when Chrome does it first". We like the
security improvement, but there are already enough "works in Chrome" sites
through no fault of our own. We can't afford adding to that number
unnecessarily through a self-inflicted wound.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A new testing policy for code landing in mozilla-central

[Andrew McCreight]

On Tue, Sep 15, 2020 at 8:03 AM Brian Grinstead 
wrote:

> (This is a crosspost from firefox-dev)
>
> Hi all,
>
> We’re rolling out a change to the review process to put more focus on
> automated testing. This will affect you if you review code that lands in
> mozilla-central.
>
> ## TLDR
>
> Reviewers will now need to add a testing Project Tag in Phabricator when
> Accepting a revision. This can be done with the “Add Action” → “Change
> Project Tags” UI in Phabricator. There's a screenshot of this UI at
> https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ.
>

I of course think having more tests is a good thing, but I don't like how
this specific process places all of the burden of understanding and
documenting some taxonomy of exceptions on the reviewer. Reviewing is
already a fairly thankless and time consuming task. It seems like the way
this is set up is due to the specifics of our how reviewing process is
implemented, so maybe there's no way around it.

Also, contrary to what ahal said, I don't know that tests being an official
requirement relieves the burden of guilt of asking for tests, as everybody
already knows that tests are good and that you should always write tests.
The situation is different from the linters, as the linters will fix almost
all issues automatically, so asking somebody to fix linter issues isn't
much of a burden at all. I guess it is impossible to make testing better
without somebody directly pressuring somebody, though.

My perspective might be a little distorted as I think a lot of the patches
I write would fall under the exceptions, either because they are
refactoring, or I am fixing a crash or security issue based on just a stack
trace.

Separately, one category of fixes I often deal with is fixing leaks or
crashes in areas of the code I am unfamiliar with. I can often figure out
the localized condition that causes the problem and correct that, but I
don't really know anything about, say, service workers or networking to
write a test. Do I need to hold off on landing until I can find somebody
who is willing and able to write a test for me, or until I'm willing to
invest the effort to become knowledgeable enough in an area of code I'm
unlikely to ever look at again? Neither of those feel great to me.

Another testing difficulty I've hit a few times this year (bug 1659013 and
bug 1607703) are crashes that happen when starting Firefox with unusual
command line or environment options. I think we only have one testing
framework that can test those kinds of conditions, and it seemed like it
was in the process of being deprecated. I had trouble finding somebody who
understood it, and I didn't want to spend more than a few hours trying to
figure it out for myself, so I landed it without testing. To be clear, I
could reproduce at least one of those crashes locally, but I wasn't sure
how to write a test to create those conditions. Under this new policy, how
do we handle fixing issues where there is not always a good testing
framework already? Writing a test is hopefully not too much of a burden,
but obviously having to develop and maintain a new testing framework could
be a good deal of work.

Anyways those are some disjointed thoughts based on my experience
developing Firefox that probably don't have good answers.

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

Re: A new testing policy for code landing in mozilla-central

[Jonathan Kew]

On 15/09/2020 16:03, Brian Grinstead wrote:

> We’re rolling out a change to the review process to put more focus on 
automated testing. [...]


As others have said, it's great that we're setting a clearer policy here 
- thanks!


One thing did strike me as a little surprising, and could perhaps be 
clarified:


> * testing-exception-other: Commits where none of the defined 
exceptions above apply but it should still be landed. This should be 
scrutinized by the reviewer before using it - consider whether an 
exception is actually required or if a test could be reasonably added 
before using it. This requires a comment from the reviewer explaining 
why it’s appropriate to land without tests.


The point that caught my eye here was that it explicitly requires the 
explanatory comment to be *from the reviewer*. I would have thought that 
in many cases it would make sense for the *patch author* to provide such 
an explanatory comment (which the reviewer would of course be expected 
to scrutinize before granting r+). Is that scenario going to be 
considered unacceptable?


Thanks,

JK

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

Re: A new testing policy for code landing in mozilla-central

[Andrew Halberstadt]

This change is fantastic, thanks for driving it!

Reviewers should not need to feel bad about asking authors to go back and
add tests, making this official policy removes that burden of guilt (just
like how reviewbot removes the burden of pointing out linting nits).
Authors can now grumble at the process rather than the reviewer. A better
situation all around.

Cheers!

On Tue, Sep 15, 2020 at 11:03 AM Brian Grinstead 
wrote:

> (This is a crosspost from firefox-dev)
>
> Hi all,
>
> We’re rolling out a change to the review process to put more focus on
> automated testing. This will affect you if you review code that lands in
> mozilla-central.
>
> ## TLDR
>
> Reviewers will now need to add a testing Project Tag in Phabricator when
> Accepting a revision. This can be done with the “Add Action” → “Change
> Project Tags” UI in Phabricator. There's a screenshot of this UI at
> https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ.
>
> We’ve been piloting the policy for a few weeks with positive feedback from
> reviewers. Still, there may be some rough edges as we roll this out more
> widely. I’d like to hear about those, so please contact me directly or in
> the #testing-policy channel in Slack / Matrix if you have feedback or
> questions about how the policy should apply to a particular review.
>
> We’re working on ways to make this more convenient in the UI and to
> prevent landing code without a tag in Lando. In the meantime if you'd like
> a reminder to add the tag while reviewing code, Nicolas Chevobbe has built
> a WebExtension that automatically opens up the Project Tags UI when
> appropriate at https://github.com/nchevobbe/phab-test-policy.
>
> ## Why?
>
> Automated tests in Firefox and Gecko reduce risk and allow us to quickly
> and confidently improve our products.
>
> While there’s a general understanding that changes should have tests, this
> hasn’t been tracked or enforced consistently. We think defining an explicit
> policy for including automated tests with code changes will help to achieve
> those goals.
>
> Also, we’ll be able to better understand exactly why and when tests aren’t
> being added. This can help to highlight components that need new testing
> capabilities or technical refactoring, and features that require increased
> manual testing.
>
> There are of course trade-offs to enforcing a testing policy. Tests take
> time to write, maintain, and run which can slow down day-to-day
> development. And given the complexity of a modern web browser, some
> components are impractical to test today (for example, components that
> interact with external hardware and software).
>
> The policy below hopes to mitigate those by using a lightweight
> enforcement mechanism and the ability to exempt changes at the discretion
> of the code reviewer.
>
> ## Policy (This text is also located at
> https://firefox-source-docs.mozilla.org/testing/testing-policy/)
>
> Everything that lands in mozilla-central includes automated tests by
> default. Every commit has tests that cover every major piece of
> functionality and expected input conditions.
>
> One of the following Project Tags must be applied in Phabricator before
> landing, at the discretion of the reviewer:
>
> * `testing-approved` if it has sufficient automated test coverage.
> * One of `testing-exception-*` if not. After speaking with many teams
> across the project we’ve identified the most common exceptions, which are
> detailed below.
>
> ### Exceptions
>
> * testing-exception-unchanged: Commits that don’t change behavior for end
> users. For example:
>   * Refactors, mechanical changes, and deleting dead code as long as they
> aren’t meaningfully changing or removing any existing tests. Authors should
> consider checking for and adding missing test coverage in a separate commit
> before a refactor.
>   * Code that doesn’t ship to users (for example: documentation, build
> scripts and manifest files, mach commands). Effort should be made to test
> these when regressions are likely to cause bustage or confusion for
> developers, but it’s left to the discretion of the reviewer.
> * testing-exception-ui: Commits that change UI styling, images, or
> localized strings. While we have end-to-end automated tests that ensure the
> frontend isn’t totally broken, and screenshot-based tracking of changes
> over time, we currently rely only on manual testing and bug reports to
> surface style regressions.
> * testing-exception-elsewhere: Commits where tests exist but are somewhere
> else. This requires a comment from the reviewer explaining where the tests
> are. For example:
>   * In another commit in the Stack.
>   * In a followup bug.
>   * In an external repository for third party code.
>   * When following the [Security Bug Approval Process](
> https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html)
> tests are usually landed later, but should be written and reviewed at the
> same time as the commit.
> * 

A new testing policy for code landing in mozilla-central

[Brian Grinstead]

(This is a crosspost from firefox-dev)

Hi all,

We’re rolling out a change to the review process to put more focus on automated 
testing. This will affect you if you review code that lands in mozilla-central.

## TLDR

Reviewers will now need to add a testing Project Tag in Phabricator when 
Accepting a revision. This can be done with the “Add Action” → “Change Project 
Tags” UI in Phabricator. There's a screenshot of this UI at 
https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ.

We’ve been piloting the policy for a few weeks with positive feedback from 
reviewers. Still, there may be some rough edges as we roll this out more 
widely. I’d like to hear about those, so please contact me directly or in the 
#testing-policy channel in Slack / Matrix if you have feedback or questions 
about how the policy should apply to a particular review.

We’re working on ways to make this more convenient in the UI and to prevent 
landing code without a tag in Lando. In the meantime if you'd like a reminder 
to add the tag while reviewing code, Nicolas Chevobbe has built a WebExtension 
that automatically opens up the Project Tags UI when appropriate at 
https://github.com/nchevobbe/phab-test-policy.

## Why?

Automated tests in Firefox and Gecko reduce risk and allow us to quickly and 
confidently improve our products.

While there’s a general understanding that changes should have tests, this 
hasn’t been tracked or enforced consistently. We think defining an explicit 
policy for including automated tests with code changes will help to achieve 
those goals.

Also, we’ll be able to better understand exactly why and when tests aren’t 
being added. This can help to highlight components that need new testing 
capabilities or technical refactoring, and features that require increased 
manual testing.

There are of course trade-offs to enforcing a testing policy. Tests take time 
to write, maintain, and run which can slow down day-to-day development. And 
given the complexity of a modern web browser, some components are impractical 
to test today (for example, components that interact with external hardware and 
software).

The policy below hopes to mitigate those by using a lightweight enforcement 
mechanism and the ability to exempt changes at the discretion of the code 
reviewer.

## Policy (This text is also located at 
https://firefox-source-docs.mozilla.org/testing/testing-policy/)

Everything that lands in mozilla-central includes automated tests by default. 
Every commit has tests that cover every major piece of functionality and 
expected input conditions.

One of the following Project Tags must be applied in Phabricator before 
landing, at the discretion of the reviewer:

* `testing-approved` if it has sufficient automated test coverage.
* One of `testing-exception-*` if not. After speaking with many teams across 
the project we’ve identified the most common exceptions, which are detailed 
below.

### Exceptions

* testing-exception-unchanged: Commits that don’t change behavior for end 
users. For example:
  * Refactors, mechanical changes, and deleting dead code as long as they 
aren’t meaningfully changing or removing any existing tests. Authors should 
consider checking for and adding missing test coverage in a separate commit 
before a refactor.
  * Code that doesn’t ship to users (for example: documentation, build scripts 
and manifest files, mach commands). Effort should be made to test these when 
regressions are likely to cause bustage or confusion for developers, but it’s 
left to the discretion of the reviewer.
* testing-exception-ui: Commits that change UI styling, images, or localized 
strings. While we have end-to-end automated tests that ensure the frontend 
isn’t totally broken, and screenshot-based tracking of changes over time, we 
currently rely only on manual testing and bug reports to surface style 
regressions.
* testing-exception-elsewhere: Commits where tests exist but are somewhere 
else. This requires a comment from the reviewer explaining where the tests are. 
For example:
  * In another commit in the Stack.
  * In a followup bug.
  * In an external repository for third party code.
  * When following the [Security Bug Approval 
Process](https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html)
 tests are usually landed later, but should be written and reviewed at the same 
time as the commit.
* testing-exception-other: Commits where none of the defined exceptions above 
apply but it should still be landed. This should be scrutinized by the reviewer 
before using it - consider whether an exception is actually required or if a 
test could be reasonably added before using it. This requires a comment from 
the reviewer explaining why it’s appropriate to land without tests. Some 
examples that have been identified include:
  * Interacting with external hardware or software and our code is missing 
abstractions to mock the interaction out.
  * 

Re: Dogfooding Warp

[Jared Wein (Mozilla)]

Hi Jan,

Thanks for the update and congratulations on reaching this milestone.

Would you like to add Warp to the list of experimental features in
about:preferences? When a feature is listed there, it will appear in crash
stats and about:support, and also give users an easy way to enable/disable.
Feel free to contact me off-list if you are interested. I'm happy to help.

Thanks,
Jared

On Tue, Sep 15, 2020 at 8:58 AM Jan de Mooij  wrote:

> Hi all,
>
> The SpiderMonkey (JS) team has been working on a significant update to
> our JITs called WarpBuilder (or just Warp) [0,1]. Before we enable
> Warp by default in Nightly (hopefully next cycle in 83) we need your
> help dogfooding it.
>
> Warp improves performance by reducing the amount of internal type
> information that is tracked, optimizing for a broader spectrum of
> cases, and by leveraging the same CacheIR optimizations used by last
> year’s BaselineInterpreter work [2]. As a result, Warp has a much
> simpler design and improves responsiveness and page load performance
> significantly (we're seeing 5-15% improvements on many visual metrics
> tests). Speedometer is about 10% faster with Warp. The JS engine also
> uses less memory when Warp is enabled.
>
> To enable Warp in Nightly:
>
> 1. Update to a recent Nightly
> 2. Go to about:config and set the "javascript.options.warp" pref to true
> 3. Restart the browser
>
> We're especially interested in stability issues and real-world
> performance problems. Warp is currently slower on various synthetic JS
> benchmarks such as Octane (which we will continue investigating in the
> coming months) but should perform well on web content.
>
> If you find any issues, please file bugs blocking:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1613592
>
> If you notice any improvements, we'd love to hear about those too.
>
> Finally, we want to thank our amazing contributors André Bargull and
> Tom Schuster for their help implementing and porting many
> optimizations.
>
> Turning Warp on is only our first step, and we expect to see a lot of
> new optimization work over the next year as we build on this. We are
> excited for what the future holds here.
>
> Thanks!
> The Warp team
>
> [0] WarpBuilder still utilizes the backend of IonMonkey so we don't
> feel it has earned the WarpMonkey name just yet.
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613592
> [2]
> https://hacks.mozilla.org/2019/08/the-baseline-interpreter-a-faster-js-interpreter-in-firefox-70/
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>


-- 
Jared Wein
Staff Software Engineer, Firefox
Mozilla Corporation
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Dogfooding Warp

[Jan de Mooij]

Hi all,

The SpiderMonkey (JS) team has been working on a significant update to
our JITs called WarpBuilder (or just Warp) [0,1]. Before we enable
Warp by default in Nightly (hopefully next cycle in 83) we need your
help dogfooding it.

Warp improves performance by reducing the amount of internal type
information that is tracked, optimizing for a broader spectrum of
cases, and by leveraging the same CacheIR optimizations used by last
year’s BaselineInterpreter work [2]. As a result, Warp has a much
simpler design and improves responsiveness and page load performance
significantly (we're seeing 5-15% improvements on many visual metrics
tests). Speedometer is about 10% faster with Warp. The JS engine also
uses less memory when Warp is enabled.

To enable Warp in Nightly:

1. Update to a recent Nightly
2. Go to about:config and set the "javascript.options.warp" pref to true
3. Restart the browser

We're especially interested in stability issues and real-world
performance problems. Warp is currently slower on various synthetic JS
benchmarks such as Octane (which we will continue investigating in the
coming months) but should perform well on web content.

If you find any issues, please file bugs blocking:

https://bugzilla.mozilla.org/show_bug.cgi?id=1613592

If you notice any improvements, we'd love to hear about those too.

Finally, we want to thank our amazing contributors André Bargull and
Tom Schuster for their help implementing and porting many
optimizations.

Turning Warp on is only our first step, and we expect to see a lot of
new optimization work over the next year as we build on this. We are
excited for what the future holds here.

Thanks!
The Warp team

[0] WarpBuilder still utilizes the backend of IonMonkey so we don't
feel it has earned the WarpMonkey name just yet.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613592
[2] 
https://hacks.mozilla.org/2019/08/the-baseline-interpreter-a-faster-js-interpreter-in-firefox-70/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform