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

Abstracting selecting an icon. #710

Closed
wants to merge 1 commit into from

Conversation

jakearchibald
Copy link

@jakearchibald jakearchibald commented Aug 21, 2018

I'm using a similar "pick the best icon from this set" pattern in background fetch, and I'm wanting to use definitions from this spec.

This PR abstracts the prose for selecting an ImageResource from a list.


Preview | Diff

@kenchris
Copy link
Collaborator

LGTM. @mgiuca @dominickng want to take a look to see how this fits with your changes to the adaptive icon PR?

@jakearchibald
Copy link
Author

cc @dbaron - this is an effort to merge together icon usage in specs.

@marcoscaceres
Copy link
Member

I'm not thrilled that we are using the document format for APIs. I think we should spin off an new ImageResource spec that we can use for APIs independently on the manifest spec (we can then build on the stuff in the Notifications API and in the Credential Management API, and in the Payment Handler spec... with the aim of them hopefully using ImageResource if possible, as well as the appropriate security model for these ImageResources as defined in Credential Management spec). The ImageResource as currently specified is not really suitable for reuse in other specs, because it has purpose and platform members.

I think we should seriously consider reverting all the WebIDL changes in the spec, as I think we made a mistake in using WebIDL to define a document format :(

@mgiuca
Copy link
Collaborator

mgiuca commented Aug 22, 2018

The "adaptive icon PR" @kenchris mentions is #657. I don't think this clashes with that. ImageResource isn't being extended there, we're just adding a new "purpose".

@jakearchibald Will Service Worker be making use of the ImageResource dictionary defined here? Including "purpose" and "platform"? Otherwise it might not make sense to do this, as Marcos says.

@marcoscaceres Why is WebIDL not appropriate for defining a document format?

It defines a JSON dictionary structure much more succinctly and readably than the parser algorithms we had previously specified. I don't think there's a problem with using the same definition for both interfaces and JSON. Even if it doesn't make sense in service workers to re-use ImageResource, imagine a hypothetical API where you can dynamically register new icons for the manifest:

void RegisterIcon(ImageResource icon);

It would make sense to re-use ImageResource dictionary for that method, no?

@jakearchibald
Copy link
Author

@mgiuca

Will Service Worker be making use of the ImageResource dictionary defined here? Including "purpose" and "platform"?

Hmm no, they don't really make sense. I need src & size, and the hand-wavey selection mechanism (the fetching is different in the background fetch case).

Yeah, maybe this needs to be somewhere else. Ugh I was trying to avoid something as huge as spinning up a new spec 😀.

Anyone know the process for this? I imagine it's different to proposing a new feature.

@marcoscaceres
Copy link
Member

@marcoscaceres Why is WebIDL not appropriate for defining a document format?

IDL defines type coercion rules, so when one does:

{
   orientation: "bananas",
}

That should "throw" a TypeError... but throwing doesn't mean anything in our format.

Similarly, per IDL:

{
   name: ["this gets coerced to a string"],
}

And so on...

@jakearchibald wrote:

Anyone know the process for this? I imagine it's different to proposing a new feature.

We can just quickly spin something up at the WICG. All we need is an index.html file and a collaborative spirit :) When we are happy, we move it back into Web Platform and just explain that we sprouted it from this spec.

@marcoscaceres
Copy link
Member

How does https://discourse.wicg.io/t/new-spec-request-select-most-appropriate-image/2964 sound?

Amazing! Spun up https://github.com/WICG/imageresource

Has a ReSpec doc - but if you prefer BikeShed, that's also 👌

@mgiuca
Copy link
Collaborator

mgiuca commented Aug 23, 2018

@marcoscaceres:

That should "throw" a TypeError... but throwing doesn't mean anything in our format.

Hmm, yeah that's the one thing that bugs me. We can write a top-level rule that says "if the ECMAScript to IDL conversion throws a TypeError, the manifest is invalid", so it isn't meaningless that the conversion throws a TypeError outside of an actual JavaScript execution context. BUT the problem is that we don't want to trash the whole manifest if one member is invalid. If one member throws a TypeError, we generally want to ignore that member, not the whole thing.

It seems that this isn't an issue with using IDL in a document format, but rather that IDL's conversion algorithm doesn't have a "non-strict mode" (that I know of). If there was a way to say "convert an ECMAScript object to IDL, with any TypeErrors in a dictionary member causing that member to be set as undefined". Or thereabouts. That would be perfectly usable in this spec.

I also wanted to say I'm not sure it's worth breaking the current icon selection language out into its own spec. At the moment, it's just too vague to be useful. The only normative text is:

If there are multiple equally appropriate icons in icons, a user agent MUST use the last one declared in order at the time that the user agent collected the list of icons. If the user agent tries to use an icon but that icon is determined, upon closer examination, to in fact be inappropriate (e.g. because its content type is unsupported), then the user agent MUST try the next-most-appropriate icon as determined by examining the ImageResource's members.

"appropriate" is never defined. The only real normative requirement here is that ties (of an unspecified evaluation function) are broken by which comes last. Doesn't seem worth trying to make this weak requirement available across specs. Though perhaps this could be an opportunity to better define "appropriate" icon selection.

@jakearchibald
Copy link
Author

The dictionary and fetching rules would also go into the spec. But I agree the spec would be otherwise small and vague.

@marcoscaceres
Copy link
Member

marcoscaceres commented Aug 23, 2018

BUT the problem is that we don't want to trash the whole manifest if one member is invalid. If one member throws a TypeError, we generally want to ignore that member, not the whole thing.

Yes, exactly. So to do that, we would need to implement a custom IDL binding layer. That's not feasible.

Our (Gecko's) implementation just runs the old processing rules:
https://github.com/mozilla/gecko-dev/blob/master/dom/manifest/ManifestProcessor.jsm#L65

We just do simple processing, it doesn't go through the IDL binding layer. Does Chrome's implementation go through the IDL layer?

It seems that this isn't an issue with using IDL in a document format, but rather that IDL's conversion algorithm doesn't have a "non-strict mode" (that I know of).

That's really not feasible. We don't need to fork our IDL biding layer for this format.

I think we need to go and figure out whatwg/infra#159

@marcoscaceres
Copy link
Member

The dictionary and fetching rules would also go into the spec. But I agree the spec would be otherwise small and vague.

Small and vague is good - but with precision on the fetching aspects. Just having the dictionary would be great.

@marcoscaceres
Copy link
Member

Cc'ing @rsolomakhin for Payment Handlers. I'd like to help with the icon effort when I finish Payment Request. I'm hoping that will be by TPAC. Maybe we can have a meeting at the TPAC Tech Plenary on Wednesday about this with Annevk and Mike for Credential Management?

@marcoscaceres
Copy link
Member

marcoscaceres commented Aug 27, 2018

(I didn't cc Mike and Anne on purpose... we can formally arrange things once we have a clear understanding of what we want to do)

@rsolomakhin
Copy link

@marcoscaceres : Sounds good to work at TPAC, but keep in mind that I'm there only for Monday and Tuesday. Can we arrange something on one of these days instead of Wednesday?

@marcoscaceres
Copy link
Member

@marcoscaceres : Sounds good to work at TPAC, but keep in mind that I'm there only for Monday and Tuesday.

That should be ok, I think.

@aarongustafson
Copy link
Collaborator

Yesterday (at TPAC) we agreed to move ImageResource out to its own spec. @rayankans and I will be tackling that. Once that happens we can look to incorporate some of these suggestions into that.

@marcoscaceres
Copy link
Member

Closing, as ImageResource moved to its own repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants