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

Simplifying the WebIDL #338

Closed
HadrienGardeur opened this issue Oct 8, 2018 · 13 comments
Closed

Simplifying the WebIDL #338

HadrienGardeur opened this issue Oct 8, 2018 · 13 comments

Comments

@HadrienGardeur
Copy link

HadrienGardeur commented Oct 8, 2018

The current WebIDL has dedicated elements for a few infoset items:

I think that having these in our WebIDL is a little bit misleading:

  • they mostly duplicate info that are in readingOrder, resources or links
  • I don't think we have any specific handling in them in our canonicalization or processing of the manifest, which means that currently they would all be empty
  • they're all helpers, which are usually meant to facilitate fetching specific info in a model, which would be fine as methods in an object, but IMO are not meant to show up in IDL
@iherman
Copy link
Member

iherman commented Oct 9, 2018

@HadrienGardeur I do not have a strong opinion on this. I understand the arguments. For me the central point is your third one:

they're all helpers, which are usually meant to facilitate fetching specific info in a model, which would be fine as methods in an object, but IMO are not meant to show up in IDL

yes indeed, they are helpers, and they would have their place as a method in a, say, JS object that embodies the Manifest. We agree on that. However, I tend to view our WebIDL (because we do not define some sort of a Web API) as mostly an abstract, language independent definition of such an object and, hence, I think it is better to keep it there. But, as I say, I do not have a strong opinion, ie, I am fine with whatever direction the WG takes.

@TzviyaSiegman @wareid @GarthConboy

(Note, for completeness, that if the PR #339 is merged, then pagelist should be added to the list.)

@HadrienGardeur
Copy link
Author

@iherman this is my first time working with WebIDL, so I'm clearly no expert at this but from a quick glance at the spec, if we really want to declare such helpers, they should be defined as getters, not the way they're currently defined where we're basically duplicating info.

Do we have anyone with good knowledge of the way WebIDL works in our group?

@iherman
Copy link
Member

iherman commented Oct 9, 2018 via email

@iherman
Copy link
Member

iherman commented Oct 9, 2018

B.t.w., the URL of the HTMLElement can be retrieved, I believe, via:

element.ownerDocument.documentURI

@iherman
Copy link
Member

iherman commented Oct 9, 2018

With the caveat that we should ask an expert... having looked at the WebIDL a bit, what WebIDL calls a getter/setter is different than what we refer to (see https://www.w3.org/TR/WebIDL-1/#idl-special-operations). They seem to be the operations one can define when defining one's own 'dictionary' like structure (but I may misunderstand).

If we want to separate the 'helpers' from the rest, an approach may be:

  1. We define an Interface not a Dictionary, and the bona fide values are defined as attributes. I am actually tempted to define all of them as readonly, something that is possible with an Interface.
  2. The helper functions are defined not as attributes but as operators, something like getCover(). This makes it clear that it is a helper.

But yes, we should ask for help:-)

@iherman
Copy link
Member

iherman commented Oct 11, 2018

Let me modify my previous proposal a bit. It may be cleaner to do something like:

dictionary WebPublicationManifest {
    ...
};

[Constructor(Document primary_entry_page, WebPublicationManifest manifest)]
interface ManifestHelpers {
    sequence<PublicationLink> getCover();
    PublicationLink getPrivacyPolicy();
    PublicationLink getAccessibilityReport();
    Promise<HTMLElement> getTOC();
    Promise<HTMLElement> getPagelist();
};

This may provide a better separation of the manifest and the helper functions, defined as separate set of operations, and we keep the relative simplicity of dictionaries for the rest.

@HadrienGardeur
Copy link
Author

As suggested by @JayPanoz, I think that the two participants that are the most likely capable of helping us on this would be @danielweck or someone from Microsoft (ping @BCWalters).

@TzviyaSiegman TzviyaSiegman added this to discuss - F2F in PWG Task Management Oct 17, 2018
@danielweck
Copy link
Member

danielweck commented Oct 19, 2018

Last time I "played" with WebIDL was when I helped write the definition of navigator.epubReadingSystem ( http://www.idpf.org/epub/31/spec/epub-contentdocs.html#app-epubReadingSystem ) ... so unfortunately I am as qualified as you are here :(

@TzviyaSiegman
Copy link
Contributor

We could consider soliciting help at TPAC from people really familiar with WebIDL.

@iherman
Copy link
Member

iherman commented Nov 5, 2018

FYI, I have asked one of my team colleagues to give his reaction on the approach described in #338 (comment). If I get a positive reply, is it o.k. if I come up with a PR with the changes?

@iherman
Copy link
Member

iherman commented Nov 27, 2018

The latest discussions on #291 shows that (1) the extraction of a TOC from the HTML structure may be relatively complex and (2) it may not really needed by the User Agent which may decide to use the TOC HTML directly. What this means is that the helper functions described in #338 (comment) may be unnecessary.

I think we should

  • go along the original issue proposal and remove those four items from the WebIDL
  • may come back, at a later stage to the issue of whether we needed helper functions in the IDL, but that should be a separate issue
  • close this issue

I will add the changes to the PR related to the resolutions of #291.

@HadrienGardeur
Copy link
Author

I'm OK with removing them from the WebIDL for now, it felt like duplication anyway.

iherman added a commit that referenced this issue Nov 27, 2018
1. Have put a reference to the new appendix from section 4.7.3.
2. Have removed the explicit WebIDL entry for TOC (and its acolytes), in line with the proposal in #338

Cc @mattgarrish
mattgarrish added a commit that referenced this issue Nov 29, 2018
- adds a reference to the new appendix from section 4.7.3.
- removes the explicit WebIDL entry for TOC (and its acolytes), in line with the proposal in #338
mattgarrish added a commit that referenced this issue Nov 29, 2018
…or a machine-readable table of contents

- adds a reference to the new appendix from section 4.7.3.
- removes the explicit WebIDL entry for TOC (and its acolytes), in line with the proposal in #338
@iherman
Copy link
Member

iherman commented Dec 8, 2018

Done in #371; closing

@iherman iherman closed this as completed Dec 8, 2018
PWG Task Management automation moved this from discuss - F2F to Editorial Dec 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PWG Task Management
  
Editorial
Development

No branches or pull requests

4 participants