Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update review of monitored specs #1477

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Update review of monitored specs #1477

merged 3 commits into from
Sep 3, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 1, 2024

The following specs have been updated since the last review:

- Digital Credentials: active development, may transition to a WG soon
- Observable API: active development (though Chromium only)
- HTML Ruby Extensions: development resumes in HTML WG
- DBSC: in development in Chrome but no entry in Chromestatus
- Capture all screens: essentially not for general browsers

Also dropped entries now added to specs.json
@tidoust
Copy link
Member

tidoust commented Sep 2, 2024

I added 3 specs to the list and updated a couple of review comments, @dontcallmedom PTAL.

Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in addition to the specific comments, do we lose any useful testing by not going through the "add spec through issue" workflow?

@@ -1276,6 +1278,12 @@
"https://www.w3.org/TR/html-aam-1.0/",
"https://www.w3.org/TR/html-aria/",
"https://www.w3.org/TR/html-media-capture/",
{
"url": "https://www.w3.org/TR/html-ruby-extensions/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... This is in practice a delta spec of the HTML spec (although not intended to serve as a next iteration of it); should this be represented somehow?

In particular, it adds 2 HTML elements that I suspect we want to detect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in practice a delta spec of the HTML spec (although not intended to serve as a next iteration of it); should this be represented somehow?

I was thinking we would handle that as we handle TC39 proposals: at some point in the future, we flag the entry as discontinued and obsoleted by HTML. It's not easy to flag the spec as being a "delta" of "HTML" (code would get confused by a seriesPrevious property in specs.json in particular)

In particular, it adds 2 HTML elements that I suspect we want to detect.

They should end up being detected. But the ones that already exist in HTML will be detected as well. We're going to end up with duplicates in extracts, including duplicates of exported definitions such as the ruby element, which may be more problematic for Bikeshed. Nothing that should be blocking though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not blocking for sure; the reason I'm mentioning it is in case we had an annotation here that would help with that conflict down the line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #1488. In the meantime, I'll warm up for a little Webref patching exercise.

@@ -725,6 +726,7 @@
"shortname": "speculation-rules"
},
"https://wicg.github.io/netinfo/",
"https://wicg.github.io/observable/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not shipping anywhere yet though? (although somehow WPT shows support in Cr 130?)

not opposed to adding it, but this changes a bit our current expectations…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then "shipping in a browser" is not in our list of criteria. For this and a few other entries under incubation, I don't really know how to evaluate readiness to enter the list. In this case, "arguments" in favor of adding it to the list:

Arguments against adding it to the list:

  • No involvement from other browser engines for now
  • Tests in WPT are flagged as "tentative"
  • No documentation on MDN

Essentially, I've been flipping a coin. I'm happy to flip it again and keep it in the monitor list.

I would find it useful to review and clarify the spec selection criteria in light of accumulated experience. I'll track that in a separate issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the pro arguments convincing, so let's add it and continue the criteria discussion separately

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Criteria discussion in #1481

@tidoust
Copy link
Member

tidoust commented Sep 2, 2024

in addition to the specific comments, do we lose any useful testing by not going through the "add spec through issue" workflow?

Yes. I tested the changes locally but that's not a good workflow. I updated the PR directly because going through the issue route would have created merge conflicts (adding the spec would drop the entry from monitor.json that this PR also updates). I don't like merge conflicts.

A better workflow could perhaps be:

  1. Report all specs to monitor in an issue with checkboxes.
  2. Review specs to monitor, check those that look correct as is
  3. Handle manual updates in separate issues/PRs
  4. Tell bot to create a PR that bumps the review date for all checked specs

@tidoust tidoust merged commit 915f3ad into main Sep 3, 2024
1 check passed
@tidoust tidoust deleted the monitor-update-1725149698 branch September 3, 2024 14:58
tidoust added a commit that referenced this pull request Sep 10, 2024
The digital-credentials repo was added to the monitor list back in January
(#1174). The digital-identities spec was added shortly afterwards (#1204),
but was using a different repository at the time.

Then digital-identities became digital-credentials, but we missed that.
Instead we recently reviewed the list of monitored specs and decided to
add digital-credentials (#1477).

That means we have twice the same spec in the list. This update drops
the digital-identities entry and adds the name as a former name of
digital-credentials, since that's the name under which the spec is now
progressing.
tidoust added a commit that referenced this pull request Sep 10, 2024
The digital-credentials repo was added to the monitor list back in January
(#1174). The digital-identities spec was added shortly afterwards (#1204),
but was using a different repository at the time.

Then digital-identities became digital-credentials, but we missed that.
Instead we recently reviewed the list of monitored specs and decided to
add digital-credentials (#1477).

That means we have twice the same spec in the list. This update drops
the digital-identities entry and adds the name as a former name of
digital-credentials, since that's the name under which the spec is now
progressing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants