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

feat: add activate option to webContents.openDevTools #13852

Merged
merged 1 commit into from Nov 27, 2018
Merged

feat: add activate option to webContents.openDevTools #13852

merged 1 commit into from Nov 27, 2018

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Jul 29, 2018 •
edited by zcbenz

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 a team July 29, 2018 18:36
@miniak miniak force-pushed the miniak/devtools-activate-option branch from 48c3609 to b69298c Compare July 29, 2018 18:40
@miniak miniak requested a review from zcbenz July 29, 2018 18:40
@miniak miniak force-pushed the miniak/devtools-activate-option branch from b69298c to 1c89271 Compare July 29, 2018 18:52
@miniak miniak mentioned this pull request Jul 29, 2018
Closed
@miniak
Copy link
Contributor Author

miniak commented Jul 29, 2018

@zcbenz can you please review?

@MarshallOfSound MarshallOfSound added semver/minor backwards-compatible functionality and removed target/3-0-x labels Jul 30, 2018
MarshallOfSound
MarshallOfSound requested changes Jul 30, 2018
View reviewed changes
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Just a style query

brightray/browser/inspectable_web_contents_impl.cc Outdated
@@ -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_) {
Copy link
Member

Choose a reason for hiding this comment

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

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

cc @ckerr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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 Compare July 30, 2018 16:10
alexeykuzmin
alexeykuzmin reviewed Aug 6, 2018
View reviewed changes
spec/api-web-contents-spec.js Outdated
w.show()
assert.equal(w.isFocused(), true)
w.webContents.openDevTools({ mode: 'detach', activate: false })
w.webContents.once('devtools-opened', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 Compare August 25, 2018 07:38
@miniak miniak changed the title feat: add activate option to webContents.openDevTools [wip] feat: add activate option to webContents.openDevTools Aug 26, 2018
alexeykuzmin
alexeykuzmin suggested changes Aug 29, 2018
View reviewed changes
Copy link
Contributor

@alexeykuzmin alexeykuzmin left a comment

Choose a reason for hiding this comment

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

See my comments in code.

docs/api/web-contents.md Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

alexeykuzmin
alexeykuzmin suggested changes Aug 29, 2018
View reviewed changes
Copy link
Contributor

@alexeykuzmin alexeykuzmin left a comment

Choose a reason for hiding this comment

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

^

@miniak miniak self-assigned this Sep 3, 2018
@zcbenz zcbenz force-pushed the miniak/devtools-activate-option branch from 65e918f to 6f7385d Compare November 27, 2018 07:06
@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 Compare November 27, 2018 07:17
@zcbenz zcbenz force-pushed the miniak/devtools-activate-option branch from e3a20a4 to 1ae8b28 Compare November 27, 2018 08:03
@zcbenz
Copy link
Member

zcbenz commented Nov 27, 2018

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

@zcbenz zcbenz merged commit d63a848 into master Nov 27, 2018
@release-clerk
Copy link

release-clerk bot commented Nov 27, 2018

Release Notes Persisted

Add activate option to webContents.openDevTools

@zcbenz zcbenz deleted the miniak/devtools-activate-option branch November 27, 2018 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr left review comments

@MarshallOfSound MarshallOfSound MarshallOfSound requested changes

@alexeykuzmin alexeykuzmin alexeykuzmin requested changes

@zcbenz zcbenz zcbenz approved these changes

Assignees

@miniak miniak

Labels
semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@miniak @zcbenz @ckerr @alexeykuzmin @MarshallOfSound