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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

60 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

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.
Attachment #8955954 - Flags: review?(nika)
Attachment #8955955 - Flags: review?(nika)
(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 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.
Attachment #8955954 - Flags: review?(nika) → review+
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.
Attachment #8955955 - Flags: review?(nika) → review+
Blocks: 1443317
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.
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.
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
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
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
Flags: needinfo?(kmaglione+bmo)
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
https://hg.mozilla.org/mozilla-central/rev/403b15374e2f
https://hg.mozilla.org/mozilla-central/rev/7532ccb5c0b3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.