Skip to content

chore: node_includes header no longer needs to be at the end of the list #17090

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.

Sign up for GitHub

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

Jump to bottom
Merged
MarshallOfSound merged 1 commit into from
Mar 12, 2019
Merged

chore: node_includes header no longer needs to be at the end of the list #17090

MarshallOfSound merged 1 commit into from
Mar 12, 2019

Conversation

@nitsakh
Copy link
Contributor

@nitsakh nitsakh commented Feb 21, 2019 •
edited
Loading

Until one of the latest version of node, the definition of the DISALLOW_COPY_AND_ASSIGN macro in node was different than in chromium. That is no longer the case, so just undefining the macro in node_includes.h works.
Related to #10363.

Description of Change

Checklist

Release Notes

Notes: no-notes

@nitsakh nitsakh requested review from a team February 21, 2019 22:45
@nitsakh nitsakh changed the title chore: node_includes header no longer needs to be at the end of the list WIP:chore: node_includes header no longer needs to be at the end of the list Feb 22, 2019
@MarshallOfSound MarshallOfSound added the new-pr 🌱 PR opened recently label Feb 22, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Feb 22, 2019
@codebytere codebytere changed the title WIP:chore: node_includes header no longer needs to be at the end of the list [wip] chore: node_includes header no longer needs to be at the end of the list Feb 25, 2019
@deepak1556 deepak1556 mentioned this pull request Feb 28, 2019
Merged
6 tasks
@deepak1556
Copy link
Member

deepak1556 commented Mar 4, 2019

@nitsakh can you rebase on latest master to fix the compilation errors. Thanks!

@nitsakh nitsakh force-pushed the node-incl-header branch from 7040ab2 to 1dcdce9 Compare March 8, 2019 01:41
@nitsakh nitsakh changed the title [wip] chore: node_includes header no longer needs to be at the end of the list chore: node_includes header no longer needs to be at the end of the list Mar 8, 2019
@nitsakh
chore: node_includes header no longer needs to be at the end of the list
51bde89 
Until one of the latest version of node, the definition of the DISALLOW_COPY_AND_ASSIGN macro in node was different than in chromium. That is no longer the case, so just undefining the macro in node_includes.h works.
@nitsakh nitsakh force-pushed the node-incl-header branch from 1dcdce9 to 51bde89 Compare March 8, 2019 22:43
deepak1556
deepak1556 approved these changes Mar 9, 2019
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM, Failing CI are known issues.

@MarshallOfSound MarshallOfSound merged commit e77d065 into master Mar 12, 2019
@release-clerk
Copy link

release-clerk bot commented Mar 12, 2019

No Release Notes

@MarshallOfSound MarshallOfSound deleted the node-incl-header branch March 12, 2019 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

+1 more reviewer

@nornagon nornagon nornagon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nitsakh @deepak1556 @nornagon @MarshallOfSound