[UnifiedPDF] Clean up the coordinate system code #24698

smfr · 2024-02-18T04:27:43Z

`625cdc0`

[UnifiedPDF] Clean up the coordinate system code
https://bugs.webkit.org/show_bug.cgi?id=269657
rdar://123173348

Reviewed by Tim Horton.

Replace the panoply of coordinate conversion functions with two template functions, capable
of converting FloatPoints and FloatRects. These are `convertUp()` and `convertDown()`, where
the former converts to enclosing coordinate systems, and the latter to inner coordinate
systems. An enum describes the available coordinate systems.

It was a purposeful choice to have convertUp() and convertDown() only do conversions within
the Plugin; outside the Plugin boundary (e.g. to/from RootView), functions on PDFPluginBase
can be used, albeit with the minor annoyance that conversions between integer and float
types are necessary.

Generally, staying in FloatPoint/FloatRect are preferred to converting using integer types

Minor other cleanups.

The only thing I can find that doesn't seem to use the correct conversions is painting the
yellow Find highlight, which continues to give incorrect results.

* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDocumentLayout.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDocumentLayout.mm:
(WebKit::PDFDocumentLayout::nearestPageIndexForDocumentPoint const):
(WebKit::PDFDocumentLayout::layoutBoundsForPageAtIndex const):
(WebKit::PDFDocumentLayout::scaledContentsSize const):
(WebKit::PDFDocumentLayout::documentToPDFPage const):
(WebKit::PDFDocumentLayout::pdfPageToDocument const):
(WebKit::PDFDocumentLayout::documentPointToPDFPagePoint const): Deleted.
(WebKit::PDFDocumentLayout::pdfPagePointToDocumentPoint const): Deleted.
(WebKit::PDFDocumentLayout::pdfPageRectToDocumentRect const): Deleted.
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm:
(WebKit::UnifiedPDFPlugin::setNeedsRepaintInDocumentRect):
(WebKit::UnifiedPDFPlugin::paintHoveredAnnotationOnPage):
(WebKit::UnifiedPDFPlugin::updateScrollingExtents):
(WebKit::UnifiedPDFPlugin::pageIndexForDocumentPoint const):
(WebKit::UnifiedPDFPlugin::indexForCurrentPageInView const):
(WebKit::UnifiedPDFPlugin::annotationForRootViewPoint const):
(WebKit::UnifiedPDFPlugin::convertUp const):
(WebKit::UnifiedPDFPlugin::convertDown const):
(WebKit::UnifiedPDFPlugin::pdfElementTypesForPluginPoint const):
(WebKit::UnifiedPDFPlugin::handleMouseEvent):
(WebKit::UnifiedPDFPlugin::documentRectForAnnotation const):
(WebKit::UnifiedPDFPlugin::scrollToPDFDestination):
(WebKit::UnifiedPDFPlugin::scrollToPointInPage):
(WebKit::UnifiedPDFPlugin::scrollToPage):
(WebKit::UnifiedPDFPlugin::beginTrackingSelection):
(WebKit::UnifiedPDFPlugin::updateCurrentSelectionForContextMenuEventIfNeeded):
(WebKit::computeMarqueeSelectionRect):
(WebKit::UnifiedPDFPlugin::continueTrackingSelection):
(WebKit::UnifiedPDFPlugin::existingSelectionContainsPoint const):
(WebKit::UnifiedPDFPlugin::rectForSelectionInRootView const):
(WebKit::UnifiedPDFPlugin::countFindMatches):
(WebKit::UnifiedPDFPlugin::findString):
(WebKit::UnifiedPDFPlugin::collectFindMatchRects):
(WebKit::UnifiedPDFPlugin::rectsForTextMatchesInRect const):
(WebKit::UnifiedPDFPlugin::textIndicatorForSelection):
(WebKit::UnifiedPDFPlugin::performDictionaryLookupAtLocation):
(WebKit::UnifiedPDFPlugin::selectionBoundsForFirstPageInDocumentSpace const):
(WebKit::UnifiedPDFPlugin::showDefinitionForAttributedString):
(WebKit::UnifiedPDFPlugin::lookupTextAtLocation):
(WebKit::UnifiedPDFPlugin::pluginBoundsForAnnotation const):
(WebKit::UnifiedPDFPlugin::convertFromRootViewToDocument const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromPluginToDocument const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromDocumentToPlugin const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromPageToRootView const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromDocumentToPage const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromPageToDocument const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromPageToContents const): Deleted.
(WebKit::UnifiedPDFPlugin::convertFromDocumentToContents const): Deleted.
(WebKit::UnifiedPDFPlugin::offsetContentsSpacePointByPageMargins const): Deleted.

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

da75b60

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

webkit-early-warning-system · 2024-02-18T04:27:55Z

EWS run on previous version of this PR (hash f5e676d)

Details

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

hortont424

Yayyyy

hortont424 · 2024-02-18T04:55:01Z

Source/WebGPU/WebGPU.xcodeproj/xcshareddata/xcschemes/wgslfuzz.xcscheme

aprotyas

This is great! Some minor comments

(LGTM)

aprotyas · 2024-02-18T04:36:52Z

Source/WebGPU/WebGPU.xcodeproj/xcshareddata/xcschemes/wgslfuzz.xcscheme

Was this intentional?

@smfr guessing this was not intentional

No, but there is something funky about the project settings for wgslfuzz.xcscheme, which always gets touched on my filesystem.

Oh thanks, I will file a bug Monday to investigate that issue

https://bugs.webkit.org/show_bug.cgi?id=269734

aprotyas · 2024-02-18T04:43:12Z

Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h

We could pack a little tighter.

Suggested change

enum class CoordinateSpace : uint16_t {

enum class CoordinateSpace : uint8_t {

aprotyas · 2024-02-18T04:48:02Z

Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h

Suggested change

T convertDown(CoordinateSpace sourceSpace, CoordinateSpace destinationSpace, T, std::optional<PDFDocumentLayout::PageIndex> = { }) const;

T convertDown(CoordinateSpace sourceSpace, CoordinateSpace destinationSpace, T sourceValue, std::optional<PDFDocumentLayout::PageIndex> = { }) const;

aprotyas · 2024-02-18T04:56:04Z

Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm

I think we can also add another assertion here, to ensure the conversion direction makes sense?

static_assert(enumToUnderlyingType(sourceSpace) < enumToUnderlyingType(destinationSpace), <some sensible message about moving up>); static_assert(std::is_same<T, FloatPoint>::value || std::is_same<T, FloatRect>::value, "Coordinate conversion should use float types");

I tried this but got static assertion expression is not an integral constant expression.

webkit-early-warning-system · 2024-02-18T17:13:40Z

EWS run on previous version of this PR (hash 51d7f3e)

Details

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

webkit-early-warning-system · 2024-02-18T17:15:03Z

EWS run on current version of this PR (hash da75b60)

Details

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

https://bugs.webkit.org/show_bug.cgi?id=269657 rdar://123173348 Reviewed by Tim Horton. Replace the panoply of coordinate conversion functions with two template functions, capable of converting FloatPoints and FloatRects. These are `convertUp()` and `convertDown()`, where the former converts to enclosing coordinate systems, and the latter to inner coordinate systems. An enum describes the available coordinate systems. It was a purposeful choice to have convertUp() and convertDown() only do conversions within the Plugin; outside the Plugin boundary (e.g. to/from RootView), functions on PDFPluginBase can be used, albeit with the minor annoyance that conversions between integer and float types are necessary. Generally, staying in FloatPoint/FloatRect are preferred to converting using integer types Minor other cleanups. The only thing I can find that doesn't seem to use the correct conversions is painting the yellow Find highlight, which continues to give incorrect results. * Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDocumentLayout.h: * Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDocumentLayout.mm: (WebKit::PDFDocumentLayout::nearestPageIndexForDocumentPoint const): (WebKit::PDFDocumentLayout::layoutBoundsForPageAtIndex const): (WebKit::PDFDocumentLayout::scaledContentsSize const): (WebKit::PDFDocumentLayout::documentToPDFPage const): (WebKit::PDFDocumentLayout::pdfPageToDocument const): (WebKit::PDFDocumentLayout::documentPointToPDFPagePoint const): Deleted. (WebKit::PDFDocumentLayout::pdfPagePointToDocumentPoint const): Deleted. (WebKit::PDFDocumentLayout::pdfPageRectToDocumentRect const): Deleted. * Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h: * Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm: (WebKit::UnifiedPDFPlugin::setNeedsRepaintInDocumentRect): (WebKit::UnifiedPDFPlugin::paintHoveredAnnotationOnPage): (WebKit::UnifiedPDFPlugin::updateScrollingExtents): (WebKit::UnifiedPDFPlugin::pageIndexForDocumentPoint const): (WebKit::UnifiedPDFPlugin::indexForCurrentPageInView const): (WebKit::UnifiedPDFPlugin::annotationForRootViewPoint const): (WebKit::UnifiedPDFPlugin::convertUp const): (WebKit::UnifiedPDFPlugin::convertDown const): (WebKit::UnifiedPDFPlugin::pdfElementTypesForPluginPoint const): (WebKit::UnifiedPDFPlugin::handleMouseEvent): (WebKit::UnifiedPDFPlugin::documentRectForAnnotation const): (WebKit::UnifiedPDFPlugin::scrollToPDFDestination): (WebKit::UnifiedPDFPlugin::scrollToPointInPage): (WebKit::UnifiedPDFPlugin::scrollToPage): (WebKit::UnifiedPDFPlugin::beginTrackingSelection): (WebKit::UnifiedPDFPlugin::updateCurrentSelectionForContextMenuEventIfNeeded): (WebKit::computeMarqueeSelectionRect): (WebKit::UnifiedPDFPlugin::continueTrackingSelection): (WebKit::UnifiedPDFPlugin::existingSelectionContainsPoint const): (WebKit::UnifiedPDFPlugin::rectForSelectionInRootView const): (WebKit::UnifiedPDFPlugin::countFindMatches): (WebKit::UnifiedPDFPlugin::findString): (WebKit::UnifiedPDFPlugin::collectFindMatchRects): (WebKit::UnifiedPDFPlugin::rectsForTextMatchesInRect const): (WebKit::UnifiedPDFPlugin::textIndicatorForSelection): (WebKit::UnifiedPDFPlugin::performDictionaryLookupAtLocation): (WebKit::UnifiedPDFPlugin::selectionBoundsForFirstPageInDocumentSpace const): (WebKit::UnifiedPDFPlugin::showDefinitionForAttributedString): (WebKit::UnifiedPDFPlugin::lookupTextAtLocation): (WebKit::UnifiedPDFPlugin::pluginBoundsForAnnotation const): (WebKit::UnifiedPDFPlugin::convertFromRootViewToDocument const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromPluginToDocument const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromDocumentToPlugin const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromPageToRootView const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromDocumentToPage const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromPageToDocument const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromPageToContents const): Deleted. (WebKit::UnifiedPDFPlugin::convertFromDocumentToContents const): Deleted. (WebKit::UnifiedPDFPlugin::offsetContentsSpacePointByPageMargins const): Deleted. Canonical link: https://commits.webkit.org/274950@main

webkit-commit-queue · 2024-02-18T18:42:16Z

Committed 274950@main (625cdc0): https://commits.webkit.org/274950@main

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

smfr requested review from cdumez, mwyrzykowski and tadeuzagallo as code owners February 18, 2024 04:27

smfr self-assigned this Feb 18, 2024

smfr added the PDF For bugs in WebKit's built-in PDF support. label Feb 18, 2024

hortont424 approved these changes Feb 18, 2024

View reviewed changes

Source/WebGPU/WebGPU.xcodeproj/xcshareddata/xcschemes/wgslfuzz.xcscheme Outdated

Copy link

Contributor

hortont424 Feb 18, 2024

Choose a reason for hiding this comment

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

?

aprotyas reviewed Feb 18, 2024

View reviewed changes

smfr force-pushed the eng/UnifiedPDF-Clean-up-the-coordinate-system-code branch from f5e676d to 51d7f3e Compare February 18, 2024 17:13

smfr force-pushed the eng/UnifiedPDF-Clean-up-the-coordinate-system-code branch from 51d7f3e to da75b60 Compare February 18, 2024 17:14

smfr added the merge-queue Applied to send a pull request to merge-queue label Feb 18, 2024

webkit-commit-queue force-pushed the eng/UnifiedPDF-Clean-up-the-coordinate-system-code branch from da75b60 to 625cdc0 Compare February 18, 2024 18:42

webkit-commit-queue merged commit 625cdc0 into WebKit:main Feb 18, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 18, 2024

smfr deleted the eng/UnifiedPDF-Clean-up-the-coordinate-system-code branch March 20, 2024 03:26

	enum class CoordinateSpace : uint16_t {
	enum class CoordinateSpace : uint8_t {

	T convertDown(CoordinateSpace sourceSpace, CoordinateSpace destinationSpace, T, std::optional<PDFDocumentLayout::PageIndex> = { }) const;
	T convertDown(CoordinateSpace sourceSpace, CoordinateSpace destinationSpace, T sourceValue, std::optional<PDFDocumentLayout::PageIndex> = { }) const;

[UnifiedPDF] Clean up the coordinate system code #24698

[UnifiedPDF] Clean up the coordinate system code #24698

Uh oh!

Conversation

smfr commented Feb 18, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hortont424 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aprotyas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Feb 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

smfr commented Feb 18, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Feb 18, 2024 •

edited

Loading

aprotyas left a comment •

edited

Loading

webkit-early-warning-system commented Feb 18, 2024 •

edited

Loading

webkit-early-warning-system commented Feb 18, 2024 •

edited

Loading