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

feat: add activate option to webContents.openDevTools #13852

Merged
merged 1 commit into from Nov 27, 2018

Conversation

Projects
None yet
5 participants
@miniak
Copy link
Contributor

commented Jul 29, 2018

Make it possible to show the devtools window without focusing. Fixes #13095

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Notes: Add activate option to webContents.openDevTools

@miniak miniak requested review from as code owners Jul 29, 2018

@miniak miniak force-pushed the miniak/devtools-activate-option branch from 48c3609 to b69298c Jul 29, 2018

@miniak miniak requested a review from zcbenz Jul 29, 2018

@miniak miniak force-pushed the miniak/devtools-activate-option branch from b69298c to 1c89271 Jul 29, 2018

@miniak miniak added the target/3-0-x label Jul 29, 2018

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

@zcbenz can you please review?

@MarshallOfSound
Copy link
Member

left a comment

Just a style query

@@ -412,8 +414,9 @@ void InspectableWebContentsImpl::CloseWindow() {

void InspectableWebContentsImpl::LoadCompleted() {
frontend_loaded_ = true;
if (managed_devtools_web_contents_)
view_->ShowDevTools();
if (managed_devtools_web_contents_) {

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jul 30, 2018

Member

Pretty sure we have a style guide / made a style decision that we wanted single liners to have no braces 🤔

cc @ckerr

This comment has been minimized.

Copy link
@miniak

miniak Jul 30, 2018

Author Contributor

I will change it, but IMHO single liners without braces are really bad idea. It reminds me of the goto fail bug

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Aug 9, 2018

Contributor

Pretty sure we have a style guide / made a style decision that we wanted single liners to have no braces

What was a reasoning behind that decision? This style screws up git blame of a line with a condition when a second line (and braces) has to be added.

This comment has been minimized.

Copy link
@ckerr

ckerr Aug 10, 2018

Member

We generally follow the Chrome / Google C++ guidelines, where braces are generally optional (not forbidden) for single-liners:

In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some projects require that an if must always have an accompanying brace.

So there's wiggle room there if we want to require braces or if we want them to be optional for single-liners. I don't really care what we use as long as we're consistent.

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Aug 10, 2018

Member

I don't really care what we use as long as we're consistent.

👍

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Aug 29, 2018

Contributor

I think in general we should accept anything produced by clang-format.
Curly braces are probably optional here.

@miniak miniak force-pushed the miniak/devtools-activate-option branch 2 times, most recently from d1e0fc3 to 3916d59 Jul 30, 2018

w.show()
assert.equal(w.isFocused(), true)
w.webContents.openDevTools({ mode: 'detach', activate: false })
w.webContents.once('devtools-opened', () => {

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Aug 6, 2018

Contributor
await emittedOnce(w.webContents, 'devtools-opened')
expect(w.isFocused()).to.be.true()

@miniak miniak force-pushed the miniak/devtools-activate-option branch from 3916d59 to 65e918f Aug 25, 2018

@miniak miniak changed the title feat: add activate option to webContents.openDevTools [wip] feat: add activate option to webContents.openDevTools Aug 26, 2018

@alexeykuzmin
Copy link
Contributor

left a comment

See my comments in code.

@@ -1182,6 +1182,8 @@ app.once('ready', () => {
* `options` Object (optional)
* `mode` String - Opens the devtools with specified dock state, can be
`right`, `bottom`, `undocked`, `detach`. Defaults to last used dock state.
* `activate` Boolean (optional) - Whether to bring the opened devtools window to the
foreground. The default is `true`.
In `undocked` mode it's possible to dock back. In `detach` mode it's not.

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin Aug 29, 2018

Contributor

This line should be right next to the mode option description.
Please add a description of the activate option below it.

@alexeykuzmin
Copy link
Contributor

left a comment

^

@miniak miniak self-assigned this Sep 3, 2018

@zcbenz zcbenz force-pushed the miniak/devtools-activate-option branch from 65e918f to 6f7385d Nov 27, 2018

@zcbenz zcbenz changed the title [wip] feat: add activate option to webContents.openDevTools feat: add activate option to webContents.openDevTools Nov 27, 2018

@zcbenz zcbenz force-pushed the miniak/devtools-activate-option branch from 6f7385d to e3a20a4 Nov 27, 2018

@zcbenz zcbenz force-pushed the miniak/devtools-activate-option branch from e3a20a4 to 1ae8b28 Nov 27, 2018

@zcbenz

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

I have rebased the branch and resolved the review comments, this should be ready to go.

@zcbenz

zcbenz approved these changes Nov 27, 2018

@zcbenz zcbenz merged commit d63a848 into master Nov 27, 2018

26 of 27 checks passed

Artifact Comparison Changes Detected
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
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
electron-arm-testing Build #20181127.7 succeeded
Details
electron-arm64-testing Build #20181127.7 succeeded
Details
electron-lint Build #20181127.8 succeeded
Details
electron-mas-testing Build #20181127.18 succeeded
Details
electron-osx-testing Build #20181127.18 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Nov 27, 2018

Release Notes Persisted

Add activate option to webContents.openDevTools

@zcbenz zcbenz deleted the miniak/devtools-activate-option branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.