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

Add File Handling manifest member, parsing, LaunchQueue/LaunchParams #51

Merged
merged 7 commits into from
Apr 14, 2022

Conversation

evanstade
Copy link
Contributor

@evanstade evanstade commented Mar 30, 2022

Add description of manifest member file_handlers as well as a rough sketch of LaunchQueue and LaunchParams.


Preview | Diff

Copy link
Contributor

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some drive-by review comments.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Execute a file handler launch
</h2>
<p>
The steps to <dfn>execute a file handler launch</dfn> are given by

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inexorabletash should this wording also be modernized, like in w3c/compute-pressure#96

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not? As this isn't actually a web-facing method, i.e. there's no webidl definition for it or its parameters.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
<ol>
<li>[=list/for each=] |extension:string| of |extensions|
<ol>
<li>If |filename| does not end in |extension|,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do trimming of filenames? We used to do trimming in web app manifest, so might be good to check what we do there.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk I see that infra has https://infra.spec.whatwg.org/#string-starts-with so we need something for suffix/ends with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean whitespace trimming? The file names are provided by the OS as command line arguments. AFAIK there's no rule saying a file name can't start with a space.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
<li>Else, set |launches|[|file_handler|] to a list
with the single element |filename|.
</li>
<li>Continue to next element of |files|.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[=iteration/continue=]

Not sure what is the correct wording here @annevk?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@evanstade
Copy link
Contributor Author

PTAL @dmurph

index.html Outdated Show resolved Hide resolved
</section>
<section>
<h3>
Handling Web App Launches
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to acknowledge the step of 'creating a new document' or browsing context.

"user agent SHOULD navigate to url and the appropriate browsing context is set to a new top level browsing context." (same line as in protocol handling) somewhere in the algorithm?

Should we be mentioning edge cases like redirection? Or are we not mentioning that on purpose?

Copy link
Contributor Author

@evanstade evanstade Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a line about the creation of the new browser context. I didn't use the exact same language since it's a step in an algorithm rather than part of a paragraph.

Redirection (and reload) should be mentioned but I think that's sort of out of the scope of file handling. It should definitely be mentioned when the LaunchQueue/Params spec is fleshed out.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@evanstade
Copy link
Contributor Author

@tomayac @kenchris thanks for the feedback so far! Any further thoughts or comments?

index.html Outdated Show resolved Hide resolved
</section>
<section>
<h3>
Handling Web App Launches
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@evanstade
Copy link
Contributor Author

thanks!

@dmurph dmurph merged commit 3893c8e into WICG:gh-pages Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants