Start adding log module

[jgraham]

This adds a domain with a single log.entryAdded event. It currently handles
both console logs and javascript execptions, but further additions are
going to be required for things like network errors.

---

Preview | Diff

[jgraham]

More to do here but this is for early feedback on the general form.

[christian-bromann]

some cddl related comments

[whimboo]

In the last days I implemented the Runtime.consoleAPICalled from CDP for Firefox. And as noticed this kind of event is used for calls into the Console API (eg. console.log(), console.error(), ...), and for console messages. Any Javascript error is instead routed through Runtime.exceptionThrown.

As seen here we want all to be reported through the same kind event, and allow pre-filtering for types? How would we differentiate between a Javascript error and a call to console.error()? For now those would be identical, right? And I wonder if we have to differentiate between those.

[foolip]

This is a lot of glue code. As we keep going, is there a path to abstracting away most of this, so that events could just return maps and something else turns those into the right productions? Something a bit handwavy like https://dom.spec.whatwg.org/#concept-event-create which nonetheless everyone knows how to interpret.

[jgraham]

So there's no difference between a map and a map that matches a production other that the latter is guaranteed to match a production. I was thinking we could have some pseudo-varargs function like:

When required to make a map matching CDDL production |production| with a variable number of arguments |args|:

1. Let |result| be an [=map=] matching the production |production|, specifying the fields according to the values of |args| as follows:

• If |arg| is not a [=tuple=], set the field with the name of |arg| to the value of arg.
• If |arg| is a tuple, set the field named by the first item of |arg| to the second item of |arg|.

1. Assert: |result| matches |production|

2. Return |result|

And then you would use it like:

1. [=Make a map matching the CDDL production=] LogEntry with ("type", "console"), |level|, |text|, |timestamp| and |extra|/

[jgraham]

OK, I fied a few issues, but need to come back and improve the way we hook into other specs.

[sadym-chromium]

Don't we need a context in the LogEntry? ConsoleLogEntry has a realm, but we don't have a mechanism to match the Realm and the BrowsingContext yet.

Suggested change

	type: text,
	?context: BrowsingContext,
	type: text,

[jgraham]

I think this makes sense, although of course the proposed change isn't enough to actually populate the context id. I was looking at what CDP has and it doesn't seem to have this for Log.LogEntry. How do existing clients/implementations work here?

[sadym-chromium]

It looks like the Log.LogEntry event is global per connection, while Runtime.consoleAPICalled has an executionContextId.

[jgraham]

I remembered the reason we don't provide a context id but just a realm id is that something like a service worker has a realm but not a context. We could of course provide both, but I expect that we'll end up with a generic way of converting between them, and lots of places will take either a realm id or context id and work with both.

[css-meeting-bot]

The Browser Testing and Tools Working Group just discussed Start adding Logging module.

The full IRC log of that discussion

<AutomatedTester> Topic: Start adding Logging module
<AutomatedTester> github https://github.com//pull/73
<AutomatedTester> github: https://github.com//pull/73
<AutomatedTester> jgraham: the update from my side is I havent been able to do much
<AutomatedTester> sadym: the main question, should we focus on scalability or the end user usability
<jgraham> q+
<foolip> q+
<AutomatedTester> ... as we focus on scalable we can create a situation that becomes unusable from a UX point of view
<AutomatedTester> q?
<AutomatedTester> ack jgraham
<AutomatedTester> jgraham: My understandin here is on the question is [describes how log events might work]. In the spec we js errors or log messages. These can have different types of arguments that come up.
<AutomatedTester> ... There are a few ways we could structure the data and go for the Union of all the fields that could come through
<AutomatedTester> .. that feels like a bad idea and you're encoding which fields could be null
<AutomatedTester> ... one way we can solve this it to have a base type that has the basic fields and then have items that build off that
<AutomatedTester> ... and the suggestion that was made lets have a flat structure and then have an "inheritence" type model
<AutomatedTester> ... I don't have a strong oppinion
<simonstewart> q+
<foolip> q-
<AutomatedTester> q?
<AutomatedTester> ... and it feels like what we decide here will set the precedent for the rest of the spec
<AutomatedTester> q?
<AutomatedTester> ack simonstewart
<AutomatedTester> simonstewart: The inheritence model makes sense because it make gives you how that would go
<foolip> q+
<AutomatedTester> ...but lets rather think of them as mixins
<AutomatedTester> jgraham: that makes sense
<AutomatedTester> q?
<AutomatedTester> ack foolip
<AutomatedTester> foolip: At the protocol level we can have this in CDDL
<AutomatedTester> ... and it seems we all agree
<AutomatedTester> q?

[sadym-chromium]

LGTM with few comments

[sadym-chromium]

Meaning with implicit maxDepth = null. Should serialisationMaxDepth be kind of a global param?

[jgraham]

So, I think we're going to need to improve the control of serialization for the whole spec at some point. I think I'd prefer to work on that when we specify script execution. So maybe file an issue for now?

[sadym-chromium]

Agree it's not in the scope of this PR.

Filed Allow maxDepth serialisation setting #86