Closed Bug 1443317 Opened 3 years ago Closed 3 years ago

Update WebIDL hook to allow ChromeOnly changes without DOM peer review

Categories

(Developer Services :: Mercurial: hg.mozilla.org, enhancement)

Product:
Developer Services
Component:
Mercurial: hg.mozilla.org
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(2 files)

Bug 1443317: Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review.
59 bytes, text/x-review-board-request
gps
: review+
nika
: review+
Details
Bug 1443317: Check IDL reviewers per-commit rather than per-push.
59 bytes, text/x-review-board-request
gps
: review+
Details 
+++ This bug was initially created as a clone of Bug #1442931 +++

Bug 1442931 changes the WebIDL codegen to only allow web-visible WebIDL files to exist in specific roots. This is a follow-up allow changes to non-web-visible files outside that root without DOM peer review.
Comment on attachment 8956243 [details]
Bug 1443317: Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review.

https://reviewboard.mozilla.org/r/225152/#review231134

::: hghooks/mozhghooks/prevent_webidl_changes.py:100
(Diff revision 1)
> +    web_roots = (
> +        'dom/bindings/',
> +        'dom/webidl/',
> +    )

We're only allowing the ChromeOnly webidl files in dom/chrome-webidl right now. I think I'd rather whitelist that directory to allow edits to webidl there.

This doesn't match what we do in the webidl, but I think that's OK. If we do it this way then we're being conservative in both cases, only allowing web-exposed code inside dom/webidl, and only allowing unreviewed changes to dom/chrome-webidl.

This will also help ensure that people don't start checking in webidl code in other directories, which I am pretty sure we don't want.

::: hghooks/mozhghooks/prevent_webidl_changes.py:107
(Diff revision 1)
> -    webidlReviewed = False
> -    syncIPCReviewed = False
> +    webidlReviewed = None
> +    syncIPCReviewed = None

I don't think I like this change/optimization. Having it be a trinary value of None/True/False is gross...

::: hghooks/mozhghooks/prevent_webidl_changes.py:150
(Diff revision 1)
> +                if webidlReviewed is None:
> -                webidlReviewed = search(DOM_authors, DOM_peers)
> +                    webidlReviewed = search(DOM_authors, DOM_peers)
> -                if not webidlReviewed:
> +                if not any(file.startswith(root) for root in web_roots):

This means that we'll report the "thanks for getting the proper review" message when a dom peer reviews a change which is not in a web root, which I don't think is what we wanted.

I'd prefer checking this after doing the web_roots check, adding a `continue` in that if statement.

::: hghooks/mozhghooks/prevent_webidl_changes.py:163
(Diff revision 1)
> +                elif not webidlReviewed:
>                      error += "WebIDL file %s altered in changeset %s without DOM peer review\n" % (file, short(c.node()))
>                      note = "\nChanges to WebIDL files in this repo require review from a DOM peer in the form of r=...\nThis is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..\n"
>              # Only check the IPDL sync-messages.ini here.
>              elif file.endswith('ipc/ipdl/sync-messages.ini'):
> +                if syncIPCReviewed is None:

Please don't change the syncIPC stuff while your commit message says you're just changing the webidl stuff.

::: hghooks/tests/test-prevent-webidl.t:306
(Diff revision 1)
>    added 1 changesets with 1 changes to 1 files
> -  (79921ab00c00 modifies servo/interface.webidl from Servo; not enforcing peer review)
> +  (4ad2747fa9e2 modifies servo/interface.webidl from Servo; not enforcing peer review)
> +
> +Editing a .webidl file that isn't in a web root should pass
> +
> +  $ mkdir -p dom/chrome-webidl

Please create this directory at the top where the other directory is created.
Attachment #8956243 - Flags: review?(nika) → review-
Comment on attachment 8956243 [details]
Bug 1443317: Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review.

https://reviewboard.mozilla.org/r/225152/#review231144

::: hghooks/mozhghooks/prevent_webidl_changes.py:100
(Diff revision 1)
> +    web_roots = (
> +        'dom/bindings/',
> +        'dom/webidl/',
> +    )

Fine with me.

::: hghooks/mozhghooks/prevent_webidl_changes.py:107
(Diff revision 1)
> -    webidlReviewed = False
> -    syncIPCReviewed = False
> +    webidlReviewed = None
> +    syncIPCReviewed = None

It's a pretty common pattern in python code. `None` is the not initialized state; a boolean is the initialized value. It would be a `Maybe` in C++ or Rust, but python just uses type polymorphism.

I don't have a really strong opinion on it, but re-calculating this value every time we see a WebIDL file makes me itch...

::: hghooks/mozhghooks/prevent_webidl_changes.py:150
(Diff revision 1)
> +                if webidlReviewed is None:
> -                webidlReviewed = search(DOM_authors, DOM_peers)
> +                    webidlReviewed = search(DOM_authors, DOM_peers)
> -                if not webidlReviewed:
> +                if not any(file.startswith(root) for root in web_roots):

I thought about that, but in the end decided that DOM peer review for WebIDL changes is still desirable even if it's not necessary, so the message may as well stay.
Comment on attachment 8956243 [details]
Bug 1443317: Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review.

https://reviewboard.mozilla.org/r/225152/#review231460

The approach seems reasonable from a hooks perspective, modulo the logic error I identified.

It would be nice to avoid printing the warning on every changed file.

Keep in mind that as we move to Autoland, people won't see these warnings (unless we change Autoland to print them in bugs - which may be a legitimate feature request). So non-fatal warnings may not add much value and we should consider removing them.

::: hghooks/mozhghooks/prevent_webidl_changes.py:151
(Diff revision 2)
> +                    if webidlReviewed is None:
> -                webidlReviewed = search(DOM_authors, DOM_peers)
> +                        webidlReviewed = search(DOM_authors, DOM_peers)

This doesn't seem correct to me and seems to point at a lack of test coverage.

The new behavior will only examine the commit message on the first commit touching a .webidl outside of `dom/chrome-webidl/`. So, a subsequent commit in the push touching a .webidl that doesn't have DOM peer review would likely slip through.

I think you want the "if chrome webidl" branch to print the warning and `continue` the file iteration loop.

Also, the fact that the `webidlReviewed` variable is defined outside the loop and is effectively used as a local in the loop is kinda wonky and difficult to reason about. If you wanted to refactor the code a bit for readability, it would be appreciated. But this isn't necessary to gain an r+.
Attachment #8956243 - Flags: review?(gps) → review-
Comment on attachment 8956656 [details]
Bug 1443317: Check IDL reviewers per-commit rather than per-push.

https://reviewboard.mozilla.org/r/225624/#review231482

This is much easier to reason about! Thanks for the refactor.
Attachment #8956656 - Flags: review?(gps) → review+
Comment on attachment 8956243 [details]
Bug 1443317: Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review.

https://reviewboard.mozilla.org/r/225152/#review231484

I'm OK with this from a hooks side.

I'd appreciate a (at least) rubber stamp r+ from a DOM peer that this is wanted. Although there may be enough people CC'd on the bug to constitute consent. I defer to someone else to press the autoland button :)
Attachment #8956243 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #9)
> I'd appreciate a (at least) rubber stamp r+ from a DOM peer that this is
> wanted. Although there may be enough people CC'd on the bug to constitute
> consent. I defer to someone else to press the autoland button :)

Agreed. This is why I also requested review from mystor :)
Comment on attachment 8956243 [details]
Bug 1443317: Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review.

https://reviewboard.mozilla.org/r/225152/#review231810

::: hghooks/mozhghooks/prevent_webidl_changes.py:147
(Diff revision 3)
>  
>              # Only check WebIDL files here.
>              if file.endswith('.webidl'):
> +                if file.startswith(chrome_webidl_root):
> +                    print ("Not enforcing DOM peer review for WebIDL file %s "
> +                           "in changeset %s since it is not in a web root. "

s/since it is not in a web root/as it is in the chrome webidl root/
Attachment #8956243 - Flags: review?(nika) → review+
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5fd802067309
Check IDL reviewers per-commit rather than per-push. r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7d8dc89dee25
Update WebIDL hook to allow ChromeOnly WebIDL changes without DOM peer review. r=gps,mystor
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.