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

refactor: eliminate brightray #15240

Merged
merged 1 commit into from Oct 24, 2018

Conversation

@miniak
Contributor

miniak commented Oct 18, 2018

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 electron/browserview as code owners Oct 18, 2018

@miniak miniak changed the title from Miniak/brightray to refactor: remove brightray Oct 18, 2018

@miniak miniak changed the title from refactor: remove brightray to refactor: eliminate brightray Oct 18, 2018

@deepak1556

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

This comment has been minimized.

Contributor

miniak commented Oct 18, 2018

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

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 19, 2018

@deepak1556 separated part of this into #15288

@miniak miniak self-assigned this Oct 20, 2018

@miniak miniak changed the title from refactor: eliminate brightray to [wip] refactor: eliminate brightray Oct 20, 2018

@miniak miniak changed the title from [wip] refactor: eliminate brightray to refactor: eliminate brightray Oct 23, 2018

@ckerr

ckerr approved these changes Oct 23, 2018

🎉

@felixrieseberg

This comment has been minimized.

Member

felixrieseberg commented Oct 23, 2018

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

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 23, 2018

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

@deepak1556

Awesome, thanks for the cleanup @miniak

#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 {

This comment has been minimized.

@deepak1556

deepak1556 Oct 24, 2018

Member

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

const char kDevToolsBoundsPref[] = "brightray.devtools.bounds";
const char kDevToolsZoomPref[] = "brightray.devtools.zoom";
const char kDevToolsPreferences[] = "brightray.devtools.preferences";
const char kDevToolsBoundsPref[] = "electron.devtools.bounds";

This comment has been minimized.

@deepak1556

deepak1556 Oct 24, 2018

Member

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

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 24, 2018

Member

That and these are purely used for devtools

This comment has been minimized.

@deepak1556

deepak1556 Oct 24, 2018

Member

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.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 24, 2018

Member

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

🎉

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 24, 2018

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

@zcbenz

zcbenz approved these changes Oct 24, 2018

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 24, 2018

@alexeykuzmin can you please merge?

@alexeykuzmin alexeykuzmin added wip and removed wip labels Oct 24, 2018

@alexeykuzmin alexeykuzmin added the wip label Oct 24, 2018

@alexeykuzmin alexeykuzmin changed the title from refactor: eliminate brightray to wip? refactor: eliminate brightray Oct 24, 2018

@alexeykuzmin alexeykuzmin changed the title from wip? refactor: eliminate brightray to refactor: eliminate brightray Oct 24, 2018

@alexeykuzmin alexeykuzmin removed the wip label Oct 24, 2018

@alexeykuzmin alexeykuzmin merged commit 8ba271e into master Oct 24, 2018

20 of 21 checks passed

WIP work in progress
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

release-clerk bot commented Oct 24, 2018

Release Notes Persisted

Merged code in brightray into atom.

@alexeykuzmin alexeykuzmin deleted the miniak/brightray branch Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment