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

Jump to bottom

refactor: eliminate brightray #15240

Merged
merged 1 commit into from Oct 24, 2018
Merged

refactor: eliminate brightray #15240

merged 1 commit into from Oct 24, 2018

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Oct 18, 2018 •
edited

Description of Change

This PR definitely removes all traces of brightray.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines

Release Notes

Notes: Merged code in brightray into atom.

@miniak miniak requested review from a team October 18, 2018 05:57
@miniak miniak changed the title Miniak/brightray refactor: remove brightray Oct 18, 2018
@miniak miniak changed the title refactor: remove brightray refactor: eliminate brightray Oct 18, 2018
@miniak miniak force-pushed the miniak/brightray branch 2 times, most recently from 2a367f4 to 104ecdc Compare October 18, 2018 07:36
deepak1556
deepak1556 requested changes Oct 18, 2018
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.

The devtools components are already available as separate PR, can you also split media , net and rest into separate PR's, we don't have tests for any of the startup components, would be easier to review. Thanks!

@miniak
Copy link
Contributor Author

miniak commented Oct 18, 2018

@deepak1556 will split the PR, need to merge the devtools one first though.

@miniak
Copy link
Contributor Author

miniak commented Oct 19, 2018 •
edited

@deepak1556 separated part of this into #15288

@miniak miniak force-pushed the miniak/brightray branch 2 times, most recently from e7f2f57 to 2b62603 Compare October 20, 2018 06:24
@miniak miniak self-assigned this Oct 20, 2018
@miniak miniak force-pushed the miniak/brightray branch 4 times, most recently from ba956f4 to ef084df Compare October 20, 2018 08:59
@miniak miniak changed the title refactor: eliminate brightray [wip] refactor: eliminate brightray Oct 20, 2018
@miniak miniak force-pushed the miniak/brightray branch 5 times, most recently from 9c188cb to c855466 Compare October 23, 2018 20:33
@miniak miniak changed the title [wip] refactor: eliminate brightray refactor: eliminate brightray Oct 23, 2018
ckerr
ckerr approved these changes Oct 23, 2018
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

🎉

@felixrieseberg
Copy link
Member

👏 Solid effort, A+. Thank you so much.

@miniak
Copy link
Contributor Author

miniak commented Oct 23, 2018

@deepak1556 this is the last PR, should be just leftovers

deepak1556
deepak1556 approved these changes Oct 24, 2018
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.

Awesome, thanks for the cleanup @miniak

atom/common/mac/main_application_bundle.mm

#include "base/files/file_path.h"
#include "base/mac/bundle_locations.h"
#include "base/mac/foundation_util.h"
#include "base/path_service.h"
#include "base/strings/string_util.h"

namespace brightray {
namespace atom {

namespace {

Copy link
Member

Choose a reason for hiding this comment

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

A possible TODO for future, could rewrite the check for Helper.app path with base::mac::IsBackgroundOnlyProcess

atom/browser/ui/inspectable_web_contents_impl.cc
const char kDevToolsBoundsPref[] = "brightray.devtools.bounds";
const char kDevToolsZoomPref[] = "brightray.devtools.zoom";
const char kDevToolsPreferences[] = "brightray.devtools.preferences";
const char kDevToolsBoundsPref[] = "electron.devtools.bounds";
Copy link
Member

Choose a reason for hiding this comment

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

The key name changes will break existing preferences, but since it will be in next major release, guess its fine.

Copy link
Member

Choose a reason for hiding this comment

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

That and these are purely used for devtools

Copy link
Member

Choose a reason for hiding this comment

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

The first change is for generation of media device id, but since its generated from a random value. We can freely change the name.

And the rest are devtools related. But even if its devtools related api, I don't think we can break them unless its in a major/minor release ? They are not a blocker for this PR, just wanted to highlight the result of some changes.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah was just agreeing we can break these in the next major easily because they're barely app facing just being keys for devtools prefs

codebytere
codebytere approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

🎉

@miniak
Copy link
Contributor Author

miniak commented Oct 24, 2018

@zcbenz can you please approve for BrowserView reviewers? Thanks!

@miniak
Copy link
Contributor Author

miniak commented Oct 24, 2018

@alexeykuzmin can you please merge?

@alexeykuzmin alexeykuzmin changed the title refactor: eliminate brightray wip? refactor: eliminate brightray Oct 24, 2018
@alexeykuzmin alexeykuzmin changed the title wip? refactor: eliminate brightray refactor: eliminate brightray Oct 24, 2018
@alexeykuzmin alexeykuzmin merged commit 8ba271e into master Oct 24, 2018
@release-clerk
Copy link

release-clerk bot commented Oct 24, 2018

Release Notes Persisted

Merged code in brightray into atom.

@alexeykuzmin alexeykuzmin deleted the miniak/brightray branch October 24, 2018 10:49
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

@MarshallOfSound MarshallOfSound MarshallOfSound left review comments

@ckerr ckerr ckerr approved these changes

@alexeykuzmin alexeykuzmin alexeykuzmin approved these changes

@zcbenz zcbenz zcbenz approved these changes

@codebytere codebytere codebytere approved these changes

Assignees

@miniak miniak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@miniak @felixrieseberg @ckerr @alexeykuzmin @zcbenz @deepak1556 @codebytere @MarshallOfSound