Add type for message types

[jrandolf]

Fixed: #480

---

Preview | Diff

[jgraham]

I think this is good, but given that it's changing the low-level messages I think we should add it to the WG meeting agenda to ensure there aren't any concerns / people are aware of the change.

[css-meeting-bot]

The Browser Testing and Tools Working Group just discussed add `type` for `Message` types.

The full IRC log of that discussion

<jgraham_> Topic: add `type` for `Message` types
<orkon> jgraham_ (IRC): yeah, it makes sense, so we don't block on url pattern
<jgraham_> github: https://github.com//pull/484
<jrandolf> q+
<jrandolf> https://github.com//pull/484
<jgraham_> ack jrandolf
<orkon> jrandolf: while working on the chromium impl for webdriver bidi, we face problems to discriminate between message types in particular between top level types
<orkon> jrandolf: we wanted to implement a mechanism to discriminate them so we want to add a type attribute to messages
<jgraham_> q+
<orkon> jrandolf: or we could get rid of Extensible in the spec
<jrandolf> q-
<jgraham_> ack jgraham_
<JimEvans> q+
<orkon> jgraham_ (IRC): I am in favour of adding the type and it matches the pattern that we adopted for other places in the spec
<orkon> jgraham_ (IRC): we should be aware that it is a fundamental change so people should be aware but I do not think it will cause problems
<jrandolf> q+
<orkon> jgraham_ (IRC): based on previous discussions cddl has semantic to ignore extra fields so perhaps we dont need extensible but we perhaps need to explain in the spec what are the requirements for vendor extensions
<orkon> jgraham_ (IRC): for example in webdriver we used reserved key name patterns
<jgraham_> ack jrandolf
<orkon> jgraham_ (IRC): having some way to do vendor specific stuff is important. Whether it needs to be on the top level, idk but perhaps there are use cases for that. For example, logging through an intermediarz
<orkon> s/intermediarz/intermediary/
<orkon> Jim Evans: as a client author, I think adding the type property is no problem and seems natural
<orkon> Jim Evans: as far as extensible goes, as a client author, it is important to be able to handle that come across the wire that may not strictly, e.g., if the client receives smth from the remote end where the client has not caught up yet
<jgraham_> ack jrandolf
<orkon> Jim Evans: we need some mechanism to say that there will be other properties that you might or might not need to handle as a client
<jgraham_> ack JimEvans
<jgraham_> q+
<orkon> jrandolf: James you mentioned that Extensible can be used as a means of transporting implementation specific metadata. From looking from the users it was built to allow future keys
<orkon> jrandolf: I find it weird that we do it for the future compatibility
<orkon> jrandolf: to reserve the space to change the spec
<orkon> jrandolf: the way I see it if it is for metadata why not have the metadata field and get rid of extensiblity
<jgraham_> ack jgraham_
<orkon> jgraham_ (IRC): I don't really understand the point about the metadata specifically
<orkon> jgraham_ (IRC): there are a couple of kinds of extensibility: we want to be able to add stuff to the spec to allow existing client to not refuse the message. As long as the client data is there, they should process the message. If we want the behaviour to change, we need to make it explicit so that the clients process it with the new semantic
<jrandolf> q+
<orkon> jgraham_ (IRC): so it looks like cddl supports this explicitly.
<orkon> jgraham_ (IRC): the second kind is when the people want to add vendor-specific data and that is not necessarily metadata
<orkon> jgraham_ (IRC): new session is a clear example where there are browser specific options
<orkon> jgraham_ (IRC): so to be able to dump this stuff into the message seems fine
<orkon> jgraham_ (IRC): example from classic, if you talk to selenium grid or other components, and they might need to add some extra data that might be metadata but is it really? perhaps
<orkon> jgraham_ (IRC): I guess that stuff makes sense perhaps to add to metadata but then we have the same issue with the metadata field to avoid collision
<orkon> jgraham_ (IRC): and we end up with key namespaces
<jgraham_> ack jrandolf
<orkon> jgraham_ (IRC): it is where we ended up for classic
<orkon> jrandolf: I think it should be made clear that cddl does not define that more entries match or does not match the spec and it means that it is ambigious state.
<orkon> jrandolf: for sure having this extensible map is actually good if we want the client authors to understand that there will be extensions in the future. However, I don't see how the extensible trait guarantee any kind ... make anything easier? it is meant for documentation?
<orkon> jrandolf: you mentioned the namespacing but that was the point of metadata and we put this extra vendor information into this dedicated key
<orkon> jrandolf: in other rfcs usually a registry get created so that top level is not polluted
<orkon> jrandolf: metadata could be on anything and based on interpretation
<jrandolf> +1
<orkon> jgraham_ (IRC): I think there is clearly more to discuss. No objects to type key so we can go ahead. More discussion needed about extensible
<orkon> q+

[whimboo]

@jrandolf given that this PR is merged and no tests have been updated I wonder if you already implemented the type field in your BiDi implementation. If that is the case we could simply do it on our side as well and update tests to enforce the type field to be set.

[jrandolf]

Yes, we've added it to our side. I'll add tests soon.

[whimboo]

Yes, we've added it to our side. I'll add tests soon.

@jrandolf, to avoid test bustage on our side we would happily take the work in updating the tests to make the type field mandatory. It's probably that we only have to check the validity when handling a response / event in the WebDriver BiDi client.

[thiagowfx]

Please link the WPT PR to this bug once you send it (whoever sends one first)

[whimboo]

Note that no new tests need to be added given that the webdriver client can actually handle the type to better differentiate between the message types. I'm going to make those changes on https://bugzilla.mozilla.org/show_bug.cgi?id=1844009.

[whimboo]

@jrandolf could it be that the required changes are not yet part of a chromedriver release? When I tried my patch https://phabricator.services.mozilla.com/D185173 for the webdriver client it's hanging. I see the same when I remove the type field in Firefox. So I assume a chromedriver release is needed?

[whimboo]

@nechaev-chromium or @mathiasbynens could one of you please help? Thanks.

[jrandolf]

The mapper was rolled last Monday (https://chromium-review.googlesource.com/c/chromium/src/+/4738162), so it should be in the Chromedriver by now. Could you retry the run?

[mathiasbynens]

Per https://chromiumdash.appspot.com/commit/c6d8a1c01af25b5246ac1c331a95b7634ee419ca the roll commit made it to 117.0.5927.0. Any ChromeDriver of that version or more recent should include the changes.

[whimboo]

It doesn't work with https://chromedriver.storage.googleapis.com/112.0.5615.49/chromedriver_mac_arm64.zip which is downloaded by wpt automatically. But as per the Chromedriver download page the latest release is 114.

[mathiasbynens]

It doesn't work with https://chromedriver.storage.googleapis.com/112.0.5615.49/chromedriver_mac_arm64.zip which is downloaded by wpt automatically.

The latest Stable Chrome version is 115. Why is WPT using 112? If the infra is running that far behind, it’s gonna take a while to catch up to M117 where the change landed :(

But as per the Chromedriver download page the latest release is 114.

Hmm, the ChromeDriver download page says this for me:

[jgraham]

Per https://community.taskcluster-artifacts.net/L4is0pM-Ssm-sgEGnP5LlQ/0/public/logs/live_backing.log a recent master run is downloading Chromedriver 116. Are you sure you're passing in the right --channel to get nightly rather than stable? Otherwise, where are you seeing the older version?

[whimboo]

Oh, I have Chrome installed locally and wpt picks it up automatically. Sadly this is a 112 release. After upgrading Chrome locally it now tries to download the new 115 chromedriver binary but it fails with:

requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://storage.googleapis.com/chromium-browser-snapshots/Mac_Arm/1148911/chromedriver_mac64.zip

  File "/Users/henrik/code/gecko/testing/web-platform/mach_commands.py", line 540, in run_wpt
    return run_web_platform_tests(command_context, **params)
  File "/Users/henrik/code/gecko/testing/web-platform/mach_commands.py", line 528, in run_web_platform_tests
    return wpt_runner.run(logger, **params)
  File "/Users/henrik/code/gecko/testing/web-platform/mach_commands_base.py", line 57, in run
    kwargs = self.setup.kwargs_wptrun(kwargs)
  File "/Users/henrik/code/gecko/testing/web-platform/mach_commands.py", line 190, in kwargs_wptrun
    kwargs = run.setup_wptrunner(venv, **kwargs)
  File "/Users/henrik/code/gecko/testing/web-platform/tests/tools/wpt/run.py", line 862, in setup_wptrunner
    setup_cls.setup(kwargs)
  File "/Users/henrik/code/gecko/testing/web-platform/tests/tools/wpt/run.py", line 189, in setup
    self.setup_kwargs(kwargs)
  File "/Users/henrik/code/gecko/testing/web-platform/tests/tools/wpt/run.py", line 377, in setup_kwargs
    webdriver_binary = self.browser.install_webdriver(
  File "/Users/henrik/code/gecko/testing/web-platform/tests/tools/wpt/browser.py", line 1031, in install_webdriver
    chromedriver_path = self.install_webdriver_by_version(version, dest, revision)
  File "/Users/henrik/code/gecko/testing/web-platform/tests/tools/wpt/browser.py", line 744, in install_webdriver_by_version
    unzip(get(url).raw, dest)
  File "/Users/henrik/code/gecko/testing/web-platform/tests/tools/wpt/utils.py", line 106, in get
    resp.raise_for_status()
  File "/Users/henrik/code/gecko/third_party/python/requests/requests/models.py", line 943, in raise_for_status
    raise HTTPError(http_error_msg, response=self)

@mathiasbynens could you please check why from https://storage.googleapis.com/chromium-browser-snapshots/Mac_Arm/1148911/chromedriver_mac64.zip is not valid?

[whimboo]

It looks like the download URLs have been changed to https://edgedl.me.gvt1.com/edgedl/chrome/chrome-for-testing/115.0.5790.170/mac-arm64/chromedriver-mac-arm64.zip.

@jgraham do we miss an upstream change which hasn't been downstream synced yet?

[whimboo]

The same failure can actually be seen when running chrome tests in the upstream repository itself. Given that this is a wpt issue I filed web-platform-tests/wpt#41359.