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

Pass around event types as Atoms instead of Strings #8930

Merged
merged 1 commit into from Dec 11, 2015

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Dec 11, 2015

Event internally stores the type as an Atom, and we're Strings
everywhere, which can cause unnecessary allocations to occur since
they'll end up as Atoms anyways.

Review on Reviewable

`Event` internally stores the `type` as an `Atom`, and we're `String`s
everywhere, which can cause unnecessary allocations to occur since
they'll end up as `Atom`s anyways.
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 11, 2015

Should the new initialization methods take EventBubbles?

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 11, 2015

Probably, but I'd rather make a separate issue for that

@nox nox self-assigned this Dec 11, 2015
@nox
Copy link
Member

nox commented Dec 11, 2015

@bors-servo r+

Looks ok to me, please file a follow-up issue for what @Ms2ger said.


Reviewed 25 of 25 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Dec 11, 2015

📌 Commit 4accaf5 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Dec 11, 2015

Testing commit 4accaf5 with merge 99fd946...

bors-servo added a commit that referenced this pull request Dec 11, 2015
Pass around event types as Atoms instead of Strings

`Event` internally stores the `type` as an `Atom`, and we're `String`s
everywhere, which can cause unnecessary allocations to occur since
they'll end up as `Atom`s anyways.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8930)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 11, 2015

💔 Test failed - mac-rel-wpt

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 11, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Dec 11, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Dec 11, 2015

@bors-servo bors-servo merged commit 4accaf5 into servo:master Dec 11, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:event-type-atom branch Dec 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.