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

Add "WebKit" as a product #3213

Merged
merged 2 commits into from
Apr 11, 2023
Merged

Add "WebKit" as a product #3213

merged 2 commits into from
Apr 11, 2023

Conversation

gsnedders
Copy link
Member

This largely mimics what was done in #3161; let me know if I've missed anything.

@gsnedders gsnedders requested a review from KyleJu March 18, 2023 22:21
@gsnedders
Copy link
Member Author

Also: does it still require any work on the admin side to add things to the backend?

@gsnedders
Copy link
Member Author

gsnedders commented Mar 19, 2023

That said, we might want to call this "macOS WebKit" (or similar) given how naming is going for Chrome (see also #3214).

@past
Copy link
Member

past commented Mar 21, 2023

The changes look good to me and I agree that "macOS WebKit" would be better. If you have sorted out the authentication stuff for uploading results, I don't think you need anything else on the backend side.

Copy link
Collaborator

@KyleJu KyleJu left a comment

Choose a reason for hiding this comment

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

LGTM after addressing the naming!

@DanielRyanSmith
Copy link
Contributor

It looks like there is a test that needs to be updated in components/test/test-search.html

@gsnedders
Copy link
Member Author

Seemingly:

    test('browser test status eq', () => {
      // All known browsers.
      for (const browser of AllBrowserNames) {
        assertQueryParse(browser + ':ok', {exists: [{product: browser, status: 'OK'}]});
      }

My guess is that webapp/components/test-search.js isn't coping with having one browser name be a prefix of another.

Does anyone still around here understand Ohm well enough to quickly know the solution here?

Co-authored-by: Daniel Smith <56164590+DanielRyanSmith@users.noreply.github.com>
@KyleJu
Copy link
Collaborator

KyleJu commented Apr 11, 2023

CI passed. Merging!

@KyleJu KyleJu merged commit f995d55 into web-platform-tests:main Apr 11, 2023
@gsnedders gsnedders deleted the webkit branch April 11, 2023 18:41
@gsnedders
Copy link
Member Author

LGTM after addressing the naming!

We never did address this! But it sounded like you were hoping to make progress on #3214 soonish?

@KyleJu
Copy link
Collaborator

KyleJu commented Apr 11, 2023

macOS WebKit

We never did address this! But it sounded like you were hoping to make progress on #3214 soonish?

That is my bad! We do intend to address it in Q2, but the renaming should be done in this PR. Would you mind sending a follow-up PR?

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.

4 participants