-
Notifications
You must be signed in to change notification settings - Fork 162
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
Conversation
LGTM. @mgiuca @dominickng want to take a look to see how this fits with your changes to the adaptive icon PR? |
cc @dbaron - this is an effort to merge together icon usage in specs. |
I'm not thrilled that we are using the document format for APIs. I think we should spin off an new 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 :( |
The "adaptive icon PR" @kenchris mentions is #657. I don't think this clashes with that. @jakearchibald Will Service Worker be making use of the @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 void RegisterIcon(ImageResource icon); It would make sense to re-use |
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. |
IDL defines type coercion rules, so when one does:
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:
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. |
Amazing! Spun up https://github.com/WICG/imageresource Has a ReSpec doc - but if you prefer BikeShed, that's also 👌 |
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:
"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. |
The dictionary and fetching rules would also go into the spec. But I agree the spec would be otherwise small and vague. |
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: We just do simple processing, it doesn't go through the IDL binding layer. Does Chrome's implementation go through the IDL layer?
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 |
Small and vague is good - but with precision on the fetching aspects. Just having the dictionary would be great. |
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? |
(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) |
@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? |
That should be ok, I think. |
Yesterday (at TPAC) we agreed to move |
Closing, as ImageResource moved to its own repo. |
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