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

Fix some clippy warnings in components/script/webdriver_handlers.rs #31784

Merged
merged 5 commits into from Mar 22, 2024

Conversation

jahielkomu
Copy link
Contributor

@jahielkomu jahielkomu commented Mar 20, 2024

Fixed some clippy warnings in components/script/webdriver_handlers.rs


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes are part of Fix as many clippy problems as possible #31500
  • These changes do not require tests because they do not touch functionality, they only fix lint errors.

@jahielkomu jahielkomu changed the title Fix some clippy warnings in components/script/textinput.rs Fix some clippy warnings in components/script/webdriver_handlers.rs Mar 20, 2024
@jahielkomu
Copy link
Contributor Author

Hello @mrobinson . I hope you are well.
Please kindly review.
Thank you

@@ -382,10 +382,7 @@ fn get_element_in_view_center_point(element: &Element) -> Option<Point2D<i64>> {
.map(DomRoot::upcast::<Element>)
.and_then(|body| {
element
.GetClientRects()
.iter()
// Step 1
Copy link
Member

Choose a reason for hiding this comment

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

This comment refers to first step in spec:

Let rectangle be the first element of the DOMRect sequence returned by calling getClientRects() on element.

so it should be kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @sagudev .
Your response helps a lot.

I'm working on it now.

@jahielkomu
Copy link
Contributor Author

Hello @mrobinson and @sagudev
Please kindly review.
Thanks.

Comment on lines 384 to 386
let rectangle_element = element.GetClientRects();
let rectangle_option = rectangle_element.iter().next();
rectangle_option.map(|rectangle| {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that @sagudev meant that you should keep the comment. I recommend something like you had before, just restoring the original comment. We could even improve it a bit:

        // Step 1: Let rectangle be the first element of the DOMRect sequence 
        // returned by calling getClientRects() on element.
        element
            .GetClientRects().first()
            .map(|rectangle| {

@jahielkomu
Copy link
Contributor Author

jahielkomu commented Mar 22, 2024

I have updated the PR @mrobinson, please review.

@mrobinson in the comment;
// Step 1: Let rectangle be the first element of the DOMRect sequence returned by calling getClientRects() on element, should element, DOMRect and getClientRects() be links as earlier portrayed by @sagudev ?

@mrobinson
Copy link
Member

I have updated the PR @mrobinson, please review.

@mrobinson in the comment; // Step 1: Let rectangle be the first element of the DOMRect sequence returned by calling getClientRects() on element, should element, DOMRect and getClientRects() be links as earlier portrayed by @sagudev ?

This is not necessary. Normal comments aren't processed into rustdoc, so it isn't always useful to make links. In addition, the link exist in the specification that is linked to. I think these links are in @sagudev's comment because they were preserved while copying and pasting from the specification.

@mrobinson mrobinson changed the title Fix some clippy warnings in components/script/webdriver_handlers.rs Fix some clippy warnings in components/script/webdriver_handlers.rs Mar 22, 2024
@mrobinson mrobinson added this pull request to the merge queue Mar 22, 2024
Merged via the queue into servo:main with commit 3e9b808 Mar 22, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants