1442931 - Create a separate directory for ChromeOnly bindings that don't require DOM peer review

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement)

Description

[Kris Maglione [:kmag]]

Boris suggested this the other week.

People ask me fairly often whether they should use XPC or WebIDL bindings for something they're about to work on, and what the various advantages and disadvantages are. Every time, my response is that WebIDL is better in almost every way, but has the drawback of requiring DOM peer review. So if they're working on something non-perf-critical, they tend to choose XPC, even though WebIDL would really be the better choice.

I do like having DOM peer review for my WebIDL changes, but the pool of reviewers is small, and often don't work on areas related to our ChromeOnly bindings, so the extra review adds friction to every change. And the alternative of using XPIDL bindings instead doesn't help our code quality at all—exactly the opposite, really.

Having a separate directory which only allows ChromeOnly bindings, but doesn't require DOM peer review, would preserve the benefits of requiring review for web-visible changes, but allow us to migrate more quickly away from XPC bindings for internal interfaces.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We'd want to change the hook at https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/prevent_webidl_changes.py to ignore this directory.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1442931: Part 1 - Forbid web-visible interfaces outside of WebIDL root.

Web-visible WebIDL interfaces require DOM peer review with every change, which
is enforced by a commit hook. ChromeOnly interfaces are not exposed to the
web, and therefore don't require the same strictures.

The current commit hook enforces the review requirement for changes to any
(non-Servo) WebIDL file, and is not smart enough to determine if the change is
web-visible. In order to loosen that restriction, we need the build system to
enforce the requirement that only WebIDL files in certain locations may
contain web-visible interfaces, so that the commit hook can restrict itself to
checking only those directories.

This change restricts the location of web-visible WebIDL interfaces to the
dom/webidl/ and dom/bindings/ roots (along with the corresponding objdir root
for generated interfaces). A follow-up will change the commit hook to only
enforce review requirements on these directories.

Review commit: https://reviewboard.mozilla.org/r/224894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224894/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory.

This change moves several chrome-only WebIDL files to a separate chrome-only
directory.

There are several other chrome-only interfaces which are heavily tied to DOM
code. Since those should probably still require DOM peer review for changes, I
left them where they were.

Review commit: https://reviewboard.mozilla.org/r/224896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224896/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8955955 [details]
Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224896/diff/1-2/

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> We'd want to change the hook at
> https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/
> mozhghooks/prevent_webidl_changes.py to ignore this directory.

Please file a bug to make this change to the hg hook!

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8955954 [details]
Bug 1442931: Part 1 - Forbid web-visible interfaces outside of WebIDL root.

https://reviewboard.mozilla.org/r/224894/#review231074

::: dom/bindings/Configuration.py:107
(Diff revision 1)
>                              "%s" %
>                              (partialIface.location, iface.location))
> +                if not (iface.getExtendedAttribute("ChromeOnly") or
> +                        isInWebIDLRoot(iface.filename())):
> +                    raise TypeError(
> +                        "Interfaces which are exposed to the web may only be "

Please add a note here mentioning that you might want to mark the interface [ChromeOnly].

::: dom/bindings/mozwebidlcodegen/__init__.py:153
(Diff revision 1)
>          'ResolveSystemBinding.cpp',
>          'UnionTypes.cpp',
>          'PrototypeList.cpp',
>      }
>  
> -    def __init__(self, config_path, inputs, exported_header_dir,
> +    def __init__(self, config_path, webidl_root, inputs, exported_header_dir,

You call webidl_root web_dir elsewhere. It'd be nice if we could be consistent with the naming.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8955955 [details]
Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory.

https://reviewboard.mozilla.org/r/224896/#review231080

::: dom/moz.build:44
(Diff revision 2)
>      'bindings',
>      'battery',
>      'browser-element',
>      'cache',
>      'canvas',
> +    'chrome-webidl',

Don't love this directory name, but it's fine.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8955955 [details]
Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory.

https://reviewboard.mozilla.org/r/224896/#review231080

> Don't love this directory name, but it's fine.

I'm open to other names, if you have any suggestions.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I have no better ideas. I'd say just go with that name, and if someone else hates it enough to want to rename it, then they can go ham.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb20ada1a0aa1f6d621ba3c85ce9946a6f9841f
Bug 1442931: Part 1 - Forbid web-visible interfaces outside of WebIDL root. r=mystor

https://hg.mozilla.org/integration/mozilla-inbound/rev/195cbf3d34334978e5a9d101d4b79f899598159c
Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory. r=mystor DONTBUILD

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44b331a2a53d9d12106c1711814ab80a2cd8829e
Backed out two changesets (bug 1442931) for Windows build bustage

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/608e21fcd1674dae3f8b685cedab85c43ffdb485
Bug 1442931: Part 1 - Forbid web-visible interfaces outside of WebIDL root. r=mystor

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f46e7d52b9b2e30bf0ccf64bb5805168dd79c29
Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory. r=mystor

Comment 13

[Andreea Pavel [:apavel]]

Backed out for build bustages at ..\dom\bindings\mozwebidlcodegen\test\test_mozwebidlcodegen.py::TestWebIDLCodegenManager::test_copy_input

Push that caused the bustages: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9f46e7d52b9b2e30bf0ccf64bb5805168dd79c29

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166408598&repo=mozilla-inbound&lineNumber=40627

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7eb716937a529d734c440d6fb41adac58bcc22

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/403b15374e2f245728d7646f60aeb27b578d4a34
Bug 1442931: Part 1 - Forbid web-visible interfaces outside of WebIDL root. r=mystor

https://hg.mozilla.org/integration/mozilla-inbound/rev/7532ccb5c0b39d6abbf7f67caee3f72c7f5addbe
Bug 1442931: Part 2 - Move internal WebIDL interfaces to separate directory. r=mystor

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/403b15374e2f
https://hg.mozilla.org/mozilla-central/rev/7532ccb5c0b3