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

Provide "suggestedFilename" in "WebDriver BiDi download started" hook #11139

Merged
merged 9 commits into from
Mar 28, 2025

Conversation

sadym-chromium
Copy link
Contributor

@sadym-chromium sadym-chromium commented Mar 17, 2025

Provide "suggestedFilename" in "WebDriver BiDi download started" hook. Addressing w3c/webdriver-bidi#840.

(See WHATWG Working Mode: Changes for more details.)


/browsers.html ( diff )
/browsing-the-web.html ( diff )
/infrastructure.html ( diff )
/links.html ( diff )

Sorry, something went wrong.

@sadym-chromium sadym-chromium changed the title [WIP] Provide "suggestedFilename" in "WebDriver BiDi download started" hook Provide "suggestedFilename" in "WebDriver BiDi download started" hook Mar 25, 2025
@sadym-chromium sadym-chromium marked this pull request as ready for review March 25, 2025 10:42
@sadym-chromium
Copy link
Contributor Author

@domenic could you please take a look?

Copy link
Contributor

@Lightning00Blade Lightning00Blade left a comment

Choose a reason for hiding this comment

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

Also took a look at it and looks good to me.

@domenic
Copy link
Member

domenic commented Mar 26, 2025

Can you ensure the build is passing first?

@sadym-chromium
Copy link
Contributor Author

Can you ensure the build is passing first?

Oh, sure! Sorry for that. Fixed.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Generally looks good, nice refactoring. Just some stylistic updates.

@@ -25778,7 +25786,7 @@ document.body.appendChild(wbr);</code></pre>
<li><p>Let <var>filename</var> be the undefined value.</p></li>

<!-- Content-Disposition: attachment; filename="" is always honoured, even cross-origin -->
<li><p>If the resource has a `<code data-x="http-content-disposition">Content-Disposition</code>`
<li><p>If the <var>resource</var> has a `<code data-x="http-content-disposition">Content-Disposition</code>`
Copy link
Member

Choose a reason for hiding this comment

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

Nice job adding the explicit resource variable. However, here and below, it's better to use just "resource" instead of "the resource". (Or I guess response, once we make the update I suggest above.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sadym-chromium
Copy link
Contributor Author

Generally looks good, nice refactoring. Just some stylistic updates.

Thanks for the feedback. I believe it is addressed. Please consider merging if there are no other concerns

domenic added 2 commits March 28, 2025 14:49
@domenic domenic merged commit 2ab779b into whatwg:main Mar 28, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants