WebDriver: Add headers for Input State and Input Source #1333
+280
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noahmbright
force-pushed
the
webdriver_actions
branch
2 times, most recently
from
dad5765
to
391609c
Compare
AtkinsSJ
requested changes
Sep 9, 2024
There was a problem hiding this comment.
Hi! It's great to have someone tackling this. :^)
I have left rather a lot of comments. 😅 Some are just the same thing in multiple places.
A few more general issues:
- This is all currently "dead code" - nothing uses it. We like all code to be used by something so that we know it works, even if it's just a stub that prints a debug message. I guess this was waiting on WebDriver: Add boilerplate for endpoint 15.7 Perform Actions #1261, but now that's merged you should be able to rebase this branch and start building on that.
- Please read the Coding Style document, and run
clang-formaton your changes before submitting them. There are a few naming convention and formatting issues here. - When adding functions/methods/constructors etc, generally the code for them should go in a .cpp file with the same name. (You'll then need to add that .cpp file to the cmake files so it can compile it. Look for a file named
CMakeLists.txtin a parent directory, or you can search for a different .cpp file's name and see which one mentions it.) I can see several places here where you've declared a function, and then not implemented it. Please make sure there's an implementation, even if it's just a stub function that just logs a debug message. - I think both these headers look like they'd fit better in
Libraries/LibWeb/WebDriver, but I'm not an expert on WebDriver so I could be wrong. Maybe wait for someone else's opinion on that. (cc @trflynn89?)
Comment on lines
71
to
73
| // FIXME: return types, arguments | ||
| void keyDown(); | ||
| void keyUp(); |
There was a problem hiding this comment.
Might be good to just not include these for now, until you know how they need to work. Same for the other "action" methods.
Comment on lines
16
to
19
| // FIXME: Action object is an object, constructed with fields | ||
| // property id, type, and subtype | ||
| // this type might not express that | ||
| using ActionObject = AK::JsonObject; |
There was a problem hiding this comment.
Ideally we'd use a struct or class for this, but the need to set arbitrary keys and values on it makes that tricky, so this might be fine.
| private: | ||
| // A map where keys are input ids, and the values are input sources. | ||
| // Input ids are UUIDs, which are strings | ||
| AK::HashMap<AK::String, NonnullOwnPtr<InputSource>> m_input_state_map; |
There was a problem hiding this comment.
Because I mentioned OwnPtrs before: Using them here is correct. 👍
Endpoints 15.7 and 15.8 require input state and some associated state around that. This starts to implement that and get some scaffolding around those endpoints Make factory functions for the input sources, partially implement get a pointer id, stub in get the input state
noahmbright
force-pushed
the
webdriver_actions
branch
from
391609c
to
c6f3ea1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For issue #1040
Endpoints 15.7 and 15.8 require input state and some associated state around that. This starts to implement that and get some scaffolding around those endpoints