Skip to content

/ servo

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

Change while loop in HTMLImageElement::handle_event to for loop #15892

Merged
merged 1 commit into from Mar 10, 2017
Merged

Change while loop in HTMLImageElement::handle_event to for loop #15892

merged 1 commit into from Mar 10, 2017

Conversation

@hgallagher1993
Copy link
Contributor

hgallagher1993 commented Mar 9, 2017 •
edited by larsbergstrom

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15885
  • There are tests for these changes OR
  • These changes do not require tests because @Ms2ger said none were needed

This change is Reviewable
@highfive
Copy link

highfive commented Mar 9, 2017

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

highfive commented Mar 9, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmlimageelement.rs
  • @KiChjang: components/script/dom/htmlimageelement.rs
@highfive
Copy link

highfive commented Mar 9, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@emilio
emilio approved these changes Mar 9, 2017
View changes
Copy link
Member

emilio left a comment

Looks good, thanks!
components/script/dom/htmlimageelement.rs
let p = Point2D::new(self.upcast::<Element>().GetBoundingClientRect().X() as f32,
self.upcast::<Element>().GetBoundingClientRect().Y() as f32);
self.upcast::<Element>().GetBoundingClientRect().Y() as f32);

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Mar 9, 2017

Member

As a followup, someone should probably ensure that we only call GetBoundingClientRect once, since it's somewhat expensive.
@emilio
Copy link
Member

emilio commented Mar 9, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Mar 9, 2017

📌 Commit 3a7ccff has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Mar 9, 2017

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #15825
@bors-servo
Copy link
Contributor

bors-servo commented Mar 9, 2017

📌 Commit 3a7ccff has been approved by emilio
@hgallagher1993
Copy link
Contributor Author

hgallagher1993 commented Mar 9, 2017

Happy to help 😄
@bors-servo
Copy link
Contributor

bors-servo commented Mar 9, 2017

⌛ Testing commit 3a7ccff with merge 1c718eb...
@bors-servo
Copy link
Contributor

bors-servo commented Mar 9, 2017

💔 Test failed - linux-dev
@emilio
emilio reviewed Mar 10, 2017
View changes
components/script/dom/htmlimageelement.rs Outdated
let shp = if let Some(x) = shape {
x.absolute_coords(p)
} else {
return
};

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Mar 10, 2017

Member

You'll need to remove this trailing whitespace before landing, since CI is failing with:

./components/script/dom/htmlimageelement.rs:690: trailing whitespace

This comment has been minimized.

Sign in to view
Copy link
@hgallagher1993

hgallagher1993 Mar 10, 2017

Author Contributor

I actually saw Intellij had a little marker in the line number bar on that line but didn't know what it meant, TIL.
@hgallagher1993
Copy link
Contributor Author

hgallagher1993 commented Mar 10, 2017

@emilio checks passed, squash?
@emilio
Copy link
Member

emilio commented Mar 10, 2017

Yes, please do squash and we'll merge this, thanks for doing this! :)
@hgallagher1993
Change while loop in HTMLImageElement::handle_event to for loop
Loading status checks…
97e7c6b 
Remove trailing whitespace
@hgallagher1993 hgallagher1993 force-pushed the hgallagher1993:local_branch branch from 034f77f to 97e7c6b Mar 10, 2017
@nox
Copy link
Member

nox commented Mar 10, 2017

@bors-servo r=emilio
@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2017

📌 Commit 97e7c6b has been approved by emilio
@hgallagher1993
Copy link
Contributor Author

hgallagher1993 commented Mar 10, 2017

Should be it now. . .and no problem 😄
@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2017

⌛ Testing commit 97e7c6b with merge c584f39...
bors-servo added a commit that referenced this pull request Mar 10, 2017
@bors-servo
Auto merge of #15892 - hgallagher1993:local_branch, r=emilio
Loading status checks…
c584f39 
Change while loop in HTMLImageElement::handle_event to for loop

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #15885

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because @Ms2ger said none were needed

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15892)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 10, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: emilio
Pushing c584f39 to master...
@bors-servo bors-servo merged commit 97e7c6b into servo:master Mar 10, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@hgallagher1993 hgallagher1993 deleted the hgallagher1993:local_branch branch Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

HTMLImageElement::handle_event should use a for loop
5 participants
@hgallagher1993 @highfive @emilio @bors-servo @nox
You can’t perform that action at this time.