Skip to content

Save linked subresources when saving web page resources#22623

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
szewai:eng/Save-linked-subresources-when-saving-web-page-resources
Jan 11, 2024
Merged

Save linked subresources when saving web page resources#22623
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
szewai:eng/Save-linked-subresources-when-saving-web-page-resources

Conversation

@szewai
Copy link
Copy Markdown
Contributor

@szewai szewai commented Jan 10, 2024

f006d3e

Save linked subresources when saving web page resources
https://bugs.webkit.org/show_bug.cgi?id=267372
rdar://120491493

Reviewed by Ryosuke Niwa and BJ Burg.

When collecting subresources URLs, we should include URL of link element if it is a link to external resource
(https://html.spec.whatwg.org/multipage/links.html#linkTypes), and it may fetch or cache the linked resource
(https://html.spec.whatwg.org/multipage/semantics.html#fetching-and-processing-a-resource-from-a-link-element). We
currently only include URL of style sheet.

Update API test: WebArchive.SaveResourcesLink

* Source/WebCore/html/HTMLLinkElement.cpp:
(WebCore::mayFetchResource):
(WebCore::HTMLLinkElement::addSubresourceAttributeURLs const):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/CreateWebArchive.mm:

Canonical link: https://commits.webkit.org/272925@main

3b82ed3

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 🧪 gtk-wk2
✅ 🛠 tv 🧪 mac-AS-debug-wk2 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@szewai szewai requested review from cdumez and rniwa as code owners January 10, 2024 22:53
@szewai szewai self-assigned this Jan 10, 2024
@szewai szewai added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Jan 10, 2024
@szewai szewai requested a review from burg January 10, 2024 22:55
Comment thread Source/WebCore/html/HTMLLinkElement.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Naming suggestion: linksToExternalResource(attr)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Based on @rniwa's comment, It seems we only need to collect URLs for subresources that may be fetched based on https://html.spec.whatwg.org/multipage/semantics.html#fetching-and-processing-a-resource-from-a-link-element, so I updated this to mayFetchResource. Let me know if you think there is better naming for this.

Comment thread Source/WebCore/html/HTMLLinkElement.cpp Outdated
Comment thread Source/WebCore/html/HTMLLinkElement.cpp Outdated
Comment thread Source/WebCore/html/HTMLLinkElement.cpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't need to do prefetch or preconnect either

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems prefetch could request the resource (LinkLoader::prefetchIfNeeded) but preconnect and DNSPrefetch do not, so I removed reconnect and DNSPrefetch from the list.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, prefetch COULD request the resource but the effect isn't script observable so we don't need to do it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed

@szewai szewai force-pushed the eng/Save-linked-subresources-when-saving-web-page-resources branch from 409a0c3 to b34ef4a Compare January 11, 2024 01:02
Comment thread Source/WebCore/html/HTMLLinkElement.cpp Outdated
Copy link
Copy Markdown
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

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

r=me with comments.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 11, 2024
@szewai szewai removed the merging-blocked Applied to prevent a change from being merged label Jan 11, 2024
@szewai szewai force-pushed the eng/Save-linked-subresources-when-saving-web-page-resources branch from b34ef4a to 4e64621 Compare January 11, 2024 04:54
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

Starting EWS tests for 4e64621. Live statuses available at the PR page, #22623

@szewai szewai force-pushed the eng/Save-linked-subresources-when-saving-web-page-resources branch from 4e64621 to 3b82ed3 Compare January 11, 2024 17:26
@szewai szewai added the merge-queue Applied to send a pull request to merge-queue label Jan 11, 2024
https://bugs.webkit.org/show_bug.cgi?id=267372
rdar://120491493

Reviewed by Ryosuke Niwa and BJ Burg.

When collecting subresources URLs, we should include URL of link element if it is a link to external resource
(https://html.spec.whatwg.org/multipage/links.html#linkTypes), and it may fetch or cache the linked resource
(https://html.spec.whatwg.org/multipage/semantics.html#fetching-and-processing-a-resource-from-a-link-element). We
currently only include URL of style sheet.

Update API test: WebArchive.SaveResourcesLink

* Source/WebCore/html/HTMLLinkElement.cpp:
(WebCore::mayFetchResource):
(WebCore::HTMLLinkElement::addSubresourceAttributeURLs const):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/CreateWebArchive.mm:

Canonical link: https://commits.webkit.org/272925@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Save-linked-subresources-when-saving-web-page-resources branch from 3b82ed3 to f006d3e Compare January 11, 2024 18:46
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 272925@main (f006d3e): https://commits.webkit.org/272925@main

Reviewed commits have been landed. Closing PR #22623 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit f006d3e into WebKit:main Jan 11, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants