Clean up HTMLImageElement::handle_event #17632

omakk · 2017-07-07T12:08:21Z

Reflects desired changes in #15832

Cleans up HTMLImageElement::handle_event located in components/script/dom/htmlimageelement.rs

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Clean up un-idiomatic code in HTMLImageElement::handle_event #15832
These changes do not require tests because this change maintains the same logic

This change is

highfive · 2017-07-07T12:08:28Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

highfive · 2017-07-07T12:08:29Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmlareaelement.rs, components/script/dom/htmlimageelement.rs
@KiChjang: components/script/dom/htmlareaelement.rs, components/script/dom/htmlimageelement.rs

highfive · 2017-07-07T12:08:30Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2017-07-07T13:32:36Z

components/script/dom/htmlimageelement.rs

+           let shape = element.get_shape_from_coords();
+           let shp = match shape {
+               Some(x) => x.absolute_coords(bcr_p),
+               None    => return,


nit: no need to align the => on both lines; the preferred style is None => return, for all of the similar lines.

Will make the change now

jdm · 2017-07-07T13:33:32Z

Also, it appears that ./mach test-unit -p script now fails:

   Compiling script_tests v0.0.1 (file:///C:/projects/servo/tests/unit/script)
error[E0308]: mismatched types
   --> C:\projects\servo\tests\unit\script\htmlareaelement.rs:103:28
    |
103 |    assert!(!circ1.hit_test(Point2D::new(10.0, 20.0)));
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `euclid::TypedPoint2D`
    |
    = note: expected type `&euclid::TypedPoint2D<f32, euclid::UnknownUnit>`
               found type `euclid::TypedPoint2D<{float}, euclid::UnknownUnit>`
    = help: try with `&Point2D::new(10.0, 20.0)`
error[E0308]: mismatched types
   --> C:\projects\servo\tests\unit\script\htmlareaelement.rs:105:27
    |
105 |    assert!(circ2.hit_test(Point2D::new(10.0, 12.0)));
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `euclid::TypedPoint2D`
    |
    = note: expected type `&euclid::TypedPoint2D<f32, euclid::UnknownUnit>`
               found type `euclid::TypedPoint2D<{float}, euclid::UnknownUnit>`
    = help: try with `&Point2D::new(10.0, 12.0)`
error[E0308]: mismatched types
   --> C:\projects\servo\tests\unit\script\htmlareaelement.rs:111:28
    |
111 |    assert!(!rect1.hit_test(Point2D::new(10.0, 5.0)));
    |                            ^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `euclid::TypedPoint2D`
    |
    = note: expected type `&euclid::TypedPoint2D<f32, euclid::UnknownUnit>`
               found type `euclid::TypedPoint2D<{float}, euclid::UnknownUnit>`
    = help: try with `&Point2D::new(10.0, 5.0)`
error[E0308]: mismatched types
   --> C:\projects\servo\tests\unit\script\htmlareaelement.rs:113:27
    |
113 |    assert!(rect2.hit_test(Point2D::new(10.0, 12.0)));
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `euclid::TypedPoint2D`
    |
    = note: expected type `&euclid::TypedPoint2D<f32, euclid::UnknownUnit>`
               found type `euclid::TypedPoint2D<{float}, euclid::UnknownUnit>`
    = help: try with `&Point2D::new(10.0, 12.0)`
error[E0308]: mismatched types
   --> C:\projects\servo\tests\unit\script\htmlareaelement.rs:119:28
    |
119 |    assert!(!poly1.hit_test(Point2D::new(10.0, 5.0)));
    |                            ^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `euclid::TypedPoint2D`
    |
    = note: expected type `&euclid::TypedPoint2D<f32, euclid::UnknownUnit>`
               found type `euclid::TypedPoint2D<{float}, euclid::UnknownUnit>`
    = help: try with `&Point2D::new(10.0, 5.0)`
error[E0308]: mismatched types
   --> C:\projects\servo\tests\unit\script\htmlareaelement.rs:121:28
    |
121 |    assert!(!poly2.hit_test(Point2D::new(10.0, 5.0)));
    |                            ^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `euclid::TypedPoint2D`
    |
    = note: expected type `&euclid::TypedPoint2D<f32, euclid::UnknownUnit>`
               found type `euclid::TypedPoint2D<{float}, euclid::UnknownUnit>`
    = help: try with `&Point2D::new(10.0, 5.0)`
error: aborting due to previous error(s)
error: Could not compile `script_tests`.

omakk · 2017-07-07T13:40:20Z

Yea, Area::hit_test now takes a reference making the test fail. Should I get rid of that change?

jdm · 2017-07-07T14:16:30Z

No, you can update the tests to accommodate it instead.

omakk · 2017-07-07T15:42:27Z

Hopefully that should do it. Thanks for the help @jdm !

jdm · 2017-07-07T16:44:43Z

This looks good! Could you squash the commits into one before we merge this?

Clean up HTMLImageElement::handle_event Area::hit_test now uses a reference instead of taking ownership Fix trailing whitespace Unalign => in match Fix Area::hit_test tests to reflect updated function signature

omakk · 2017-07-07T17:09:09Z

Done!

jdm · 2017-07-07T18:21:12Z

@bors-servo: r+
Thanks!

bors-servo · 2017-07-07T18:21:13Z

📌 Commit 420a787 has been approved by jdm

bors-servo · 2017-07-07T18:21:17Z

⌛ Testing commit 420a787 with merge 6e2e715...

Clean up HTMLImageElement::handle_event Reflects desired changes in #15832  Cleans up `HTMLImageElement::handle_event` located in `components/script/dom/htmlimageelement.rs` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15832 - [X] These changes do not require tests because this change maintains the same logic  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17632)

bors-servo · 2017-07-07T19:17:10Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 6e2e715 to master...

highfive assigned jdm Jul 7, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 7, 2017

jdm requested changes Jul 7, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 7, 2017

jdm added the S-fails-travis label Jul 7, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 7, 2017

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. S-fails-travis labels Jul 7, 2017

Refactor HTMLImageElement::handle_event to be idiomatic

420a787

Clean up HTMLImageElement::handle_event Area::hit_test now uses a reference instead of taking ownership Fix trailing whitespace Unalign => in match Fix Area::hit_test tests to reflect updated function signature

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 7, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Jul 7, 2017

bors-servo merged commit 420a787 into servo:master Jul 7, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up HTMLImageElement::handle_event #17632

Clean up HTMLImageElement::handle_event #17632

omakk commented Jul 7, 2017 •

edited by larsbergstrom

Loading

highfive commented Jul 7, 2017

highfive commented Jul 7, 2017

highfive commented Jul 7, 2017

jdm Jul 7, 2017

omakk Jul 7, 2017

jdm commented Jul 7, 2017

omakk commented Jul 7, 2017

jdm commented Jul 7, 2017

omakk commented Jul 7, 2017

jdm commented Jul 7, 2017

omakk commented Jul 7, 2017

jdm commented Jul 7, 2017

bors-servo commented Jul 7, 2017

bors-servo commented Jul 7, 2017

bors-servo commented Jul 7, 2017

Clean up HTMLImageElement::handle_event #17632

Clean up HTMLImageElement::handle_event #17632

Conversation

omakk commented Jul 7, 2017 • edited by larsbergstrom Loading

highfive commented Jul 7, 2017

highfive commented Jul 7, 2017

highfive commented Jul 7, 2017

jdm Jul 7, 2017

Choose a reason for hiding this comment

omakk Jul 7, 2017

Choose a reason for hiding this comment

jdm commented Jul 7, 2017

omakk commented Jul 7, 2017

jdm commented Jul 7, 2017

omakk commented Jul 7, 2017

jdm commented Jul 7, 2017

omakk commented Jul 7, 2017

jdm commented Jul 7, 2017

bors-servo commented Jul 7, 2017

bors-servo commented Jul 7, 2017

bors-servo commented Jul 7, 2017

omakk commented Jul 7, 2017 •

edited by larsbergstrom

Loading