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

Generate serialization for WCLayerUpdateInfo. #22830

Conversation

nishajain61
Copy link
Contributor

@nishajain61 nishajain61 commented Jan 16, 2024

@nishajain61 nishajain61 self-assigned this Jan 16, 2024
@nishajain61 nishajain61 added the WebKit API For issues and bugs in the Web Kit public embedding APIs label Jan 16, 2024
@nishajain61 nishajain61 force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from ec4b7ce to 81a374e Compare January 16, 2024 20:33
Copy link
Contributor

@donny-dont donny-dont left a comment

Choose a reason for hiding this comment

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

Looks correct assuming Windows build works. Thanks for doing this!

@@ -761,6 +761,7 @@ SERIALIZATION_DESCRIPTION_FILES = \
WebProcess/UserContent/InjectUserScriptImmediately.serialization.in \
WebProcess/WebCoreSupport/WebSpeechSynthesisVoice.serialization.in \
WebProcess/WebPage/RemoteLayerTree/PlatformCAAnimationRemoteProperties.serialization.in \
WebProcess/WebPage/wc/WCUpdateInfo.serialization.in \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to add this here since Apple code is not using this.

Copy link
Contributor

@sheeparegreat sheeparegreat left a comment

Choose a reason for hiding this comment

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

I believe you also need to update Source/WebKit/DerivedSources-input.xcfilelist too?

@@ -588,6 +588,8 @@ set(WebKit_SERIALIZATION_IN_FILES
WebProcess/UserContent/InjectUserScriptImmediately.serialization.in

WebProcess/WebCoreSupport/WebSpeechSynthesisVoice.serialization.in

WebProcess/WebPage/wc/WCUpdateInfo.serialization.in
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops caught this. It should just be in PlatformWin.cmake. It is not used by other ports

Copy link
Contributor

@donny-dont donny-dont Jan 16, 2024

Choose a reason for hiding this comment

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

Actually just delete this since you're appending to the serialization.in file and its already present in the PlatformWin.cmake file.

@donny-dont
Copy link
Contributor

I believe you also need to update Source/WebKit/DerivedSources-input.xcfilelist too?

The WC stuff is only used by Windows at this time so I don't think you need to do this. In fact the CMake changes should be specific to Windows.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 16, 2024
@nishajain61 nishajain61 removed the merging-blocked Applied to prevent a change from being merged label Jan 17, 2024
@nishajain61 nishajain61 force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 81a374e to 9094a4b Compare January 17, 2024 21:46
@nishajain61
Copy link
Contributor Author

I believe you also need to update Source/WebKit/DerivedSources-input.xcfilelist too?

Fixed

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 17, 2024
@nishajain61 nishajain61 removed the merging-blocked Applied to prevent a change from being merged label Jan 17, 2024
@nishajain61 nishajain61 force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 9094a4b to 4b44863 Compare January 17, 2024 23:40
@nishajain61
Copy link
Contributor Author

I believe you also need to update Source/WebKit/DerivedSources-input.xcfilelist too?

Not needed as only for Windows

@nishajain61
Copy link
Contributor Author

I believe you also need to update Source/WebKit/DerivedSources-input.xcfilelist too?

The WC stuff is only used by Windows at this time so I don't think you need to do this. In fact the CMake changes should be specific to Windows.

Fixed

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 18, 2024
@@ -45,7 +45,7 @@ struct WCTileUpdate {
WebCore::IntRect dirtyRect;
};

enum class WCLayerChange : uint32_t {
enum class WCLayerChange : uint8_t {
Children = 1 << 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this change and mark the serialization files definition as uint32_t

This is why compilation is failing

@nishajain61 nishajain61 removed the merging-blocked Applied to prevent a change from being merged label Jan 18, 2024
@nishajain61 nishajain61 force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 4b44863 to 8091a28 Compare January 18, 2024 17:28
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 18, 2024
@nishajain61 nishajain61 removed the merging-blocked Applied to prevent a change from being merged label Jan 22, 2024
@nishajain61 nishajain61 force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 8091a28 to 2e021af Compare January 22, 2024 18:07
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 22, 2024
@nishajain61 nishajain61 removed the merging-blocked Applied to prevent a change from being merged label Jan 22, 2024
@nishajain61 nishajain61 force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 2e021af to 0a7ec7d Compare January 22, 2024 22:15
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 23, 2024
@donny-dont
Copy link
Contributor

The definition of WCLayerUpdateInfo needs to be adjusted to make it work with the expectations of the serialization generator so this should be put on pause until that's resolved. Will notify here when that is resolved as @fujii wanted to make an attempt at refactoring.

Thanks for starting this work @nishajain61 !

@nishajain61
Copy link
Contributor Author

The definition of WCLayerUpdateInfo needs to be adjusted to make it work with the expectations of the serialization generator so this should be put on pause until that's resolved. Will notify here when that is resolved as @fujii wanted to make an attempt at refactoring.

Thanks for starting this work @nishajain61 !

Please let me know what are the next steps needed for this PR to land. Thanks

@fujii
Copy link
Contributor

fujii commented Jan 30, 2024

Please let me know what are the next steps needed for this PR to land. Thanks

I'm going to do more refactoring in https://bugs.webkit.org/show_bug.cgi?id=267968 before adopting a serializer generator.
Give me more time. BTW, does it block anything?

@cdumez
Copy link
Contributor

cdumez commented Jan 30, 2024

Please let me know what are the next steps needed for this PR to land. Thanks

I'm going to do more refactoring in https://bugs.webkit.org/show_bug.cgi?id=267968 before adopting a serializer generator. Give me more time. BTW, does it block anything?

We're on a deadline to get rid of all legacy IPC coders, so yes, it is fairly urgent.

@fujii
Copy link
Contributor

fujii commented Jan 30, 2024

We're on a deadline to get rid of all legacy IPC coders, so yes, it is fairly urgent.

I misunderstood your intension. I didn't know why you want to switch WCLayerUpdateInfo to the serializer generator. WCBackingStore is also using the lagacy decoder. I'm going to rewrite them to the modern decoder.

@cdumez
Copy link
Contributor

cdumez commented Jan 30, 2024

We're on a deadline to get rid of all legacy IPC coders, so yes, it is fairly urgent.

I misunderstood your intension. I didn't know why you want to switch WCLayerUpdateInfo to the serializer generator. WCBackingStore is also using the lagacy decoder. I'm going to rewrite them to the modern decoder.

Yes, WCBackingStore is also on our list of things to be fixed. Thanks for helping.

@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label Jan 31, 2024
@donny-dont donny-dont force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 0a7ec7d to faf5b01 Compare January 31, 2024 18:47
@donny-dont donny-dont self-assigned this Jan 31, 2024
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

LGTM if the bots are happy.

@donny-dont donny-dont force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from faf5b01 to 8484ae5 Compare January 31, 2024 21:00
@donny-dont donny-dont added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 31, 2024
https://bugs.webkit.org/show_bug.cgi?id=267600
rdar://121069952

Reviewed by Chris Dumez.

Generate serialization for WCLayerUpdateInfo.

* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.h:
(WebKit::WCLayerUpdateInfo::encode const): Deleted.
(WebKit::WCLayerUpdateInfo::decode): Deleted.
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.serialization.in:

Canonical link: https://commits.webkit.org/273862@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Generate-serialization-for-WCLayerUpdateInfo- branch from 8484ae5 to 107335f Compare January 31, 2024 22:44
@webkit-commit-queue webkit-commit-queue merged commit 107335f into WebKit:main Jan 31, 2024
@webkit-commit-queue
Copy link
Collaborator

Committed 273862@main (107335f): https://commits.webkit.org/273862@main

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

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit API For issues and bugs in the Web Kit public embedding APIs
Projects
None yet
8 participants