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.

Sign up for GitHub

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

Jump to bottom

Rename EventTrackingRegions::Event to EventTrackingRegions::EventType #585

Merged
merged 0 commits into from May 14, 2022
Merged

Rename EventTrackingRegions::Event to EventTrackingRegions::EventType #585

merged 0 commits into from May 14, 2022

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented May 11, 2022 •
edited by webkit-early-warning-system

e59b6fc

Rename EventTrackingRegions::Event to EventTrackingRegions::EventType
https://bugs.webkit.org/show_bug.cgi?id=240295

Reviewed by Darin Adler.

This patch is follow-up after r293967 by Darin's comment. EventTrackingRegions::Event is not event actually,
it is just an EventType. This patch renames it with EventType. We also rename variables "event" to "eventType".

* Source/WebKit/Shared/WebCoreArgumentCoders.cpp:
(IPC::ArgumentCoder<EventTrackingRegions >::decode):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
(WebKit::RemoteScrollingCoordinatorProxy::eventTrackingTypeForPoint const):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::updateTouchEventTracking):
* Source/WebCore/page/DebugPageOverlays.cpp:
(WebCore::NonFastScrollableRegionOverlay::drawRect):
* Source/WebCore/page/Page.cpp:
(WebCore::Page::touchEventRectsForEventForTesting):
* Source/WebCore/page/Page.h:
* Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:
(WebCore::ScrollingCoordinator::absoluteEventTrackingRegionsForFrame const):
* Source/WebCore/page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::computeWheelProcessingSteps):
(WebCore::ScrollingTree::eventTrackingTypeForPoint):
* Source/WebCore/page/scrolling/ScrollingTree.h:
* Source/WebCore/platform/EventTrackingRegions.cpp:
(WebCore::EventTrackingRegions::eventName):
(WebCore::EventTrackingRegions::eventNameAtomString): We add this function to have a way to get AtomString event name. It simplifies Internal code.
(WebCore::EventTrackingRegions::trackingTypeForPoint):
(WebCore::EventTrackingRegions::uniteSynchronousRegion):
* Source/WebCore/platform/EventTrackingRegions.h:
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::touchEventRectsForEvent):

Canonical link: https://commits.webkit.org/250567@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294209 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@Constellation Constellation self-assigned this May 11, 2022
@Constellation Constellation added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 11, 2022
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 11, 2022
@Constellation Constellation removed merging-blocked Applied to prevent a change from being merged New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 11, 2022
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 11, 2022
@Constellation Constellation added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 11, 2022
Constellation
Constellation commented May 11, 2022
View reviewed changes
Source/WebCore/ChangeLog Outdated
Reviewed by NOBODY (OOPS!).

This patch is follow-up after r293967 by Darin's comment. EventTrackingRegions::Event is not event actually,
it is just an EventType. This patch renames it with EventType. We also rename variables "event" to "eventType".
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we have a patch for internal. Please check rdar://93075115 too (it has a link to that).

@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label May 11, 2022
darinadler
darinadler approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@darinadler darinadler left a comment

Choose a reason for hiding this comment

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

Looks good. I reviewed both this change and the Internal part.

Source/WebCore/platform/EventTrackingRegions.cpp Outdated
return "wheel"_s;
}
return ASCIILiteral();
}

TrackingType EventTrackingRegions::trackingTypeForPoint(Event event, const IntPoint& point)
const AtomString& EventTrackingRegions::eventNameAtomString(const EventNames& eventNames, EventType eventType)
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see us taking advantage of the pre-existing atom strings. I’m not sure we also need the ASCII literal version of eventName. Is there a case where the greater efficiency of having a literal is critical?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah :D eventNames is materialized in the main thread and worker threads. But we can avoid materializing it in the scrolling thread, and we can save 260~ AtomString allocations in the scrolling thread only for some logging features :)

Constellation
Constellation commented May 12, 2022
View reviewed changes
Copy link
Member Author

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

Thank you!

Source/WebCore/platform/EventTrackingRegions.cpp Outdated
return "wheel"_s;
}
return ASCIILiteral();
}

TrackingType EventTrackingRegions::trackingTypeForPoint(Event event, const IntPoint& point)
const AtomString& EventTrackingRegions::eventNameAtomString(const EventNames& eventNames, EventType eventType)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah :D eventNames is materialized in the main thread and worker threads. But we can avoid materializing it in the scrolling thread, and we can save 260~ AtomString allocations in the scrolling thread only for some logging features :)

@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 14, 2022
@Constellation
Copy link
Member Author

Landing it via unsafe-merge-queue since it needs to be landed with internal patch at the same time.

@Constellation Constellation added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels May 14, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit e59b6fc into WebKit:main May 14, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r294209 (250567@main): https://commits.webkit.org/250567@main

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

@webkit-early-warning-system webkit-early-warning-system removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 14, 2022
@Constellation Constellation deleted the eng/eventtype branch May 14, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darinadler darinadler darinadler approved these changes

Assignees

@Constellation Constellation

Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
Milestone
No milestone
3 participants
@Constellation @webkit-early-warning-system @darinadler