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

Allow BrowserWindows to emit `console-message` events. #11056

Merged
merged 5 commits into from Nov 11, 2017

Conversation

Projects
None yet
4 participants
@ajmacd
Copy link
Contributor

ajmacd commented Nov 7, 2017

The Slack app has cases of BrowserWindows without an accompanying WebView/BrowserView that we'd like to capture logs from. This change permits that.

@ajmacd ajmacd requested a review from electron/reviewers as a code owner Nov 7, 2017

@ajmacd

This comment has been minimized.

Copy link
Contributor

ajmacd commented Nov 7, 2017

Is there a reason we should be preventing BrowserWindows from emitting console-message events?

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 8, 2017

The behavior was changed in fb5fe7a , I don't see any reason to not allow it. Chromium will print the log messages to the console when it is not handled, you can verify by using https://github.com/electron/electron/blob/master/docs/api/chrome-command-line-switches.md#--enable-logging

@deepak1556
Copy link
Member

deepak1556 left a comment

Can you add a spec in api-web-contents-spec.js . Thanks!

@deepak1556 deepak1556 requested a review from zcbenz Nov 8, 2017

@ajmacd

This comment has been minimized.

Copy link
Contributor

ajmacd commented Nov 8, 2017

Good idea, added a test 👍


describe('console-message event', () => {
it('is triggered with correct log message', (done) => {
w.webContents.on('console-message', (e, level, message) => {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Nov 8, 2017

Member

s/on/once

It's possible that Chromium spits out deprecation warnings to console, or other console messages that we don't explicitly trigger. We want to be sure that we only call done once 👍

This comment has been minimized.

@ajmacd

ajmacd Nov 8, 2017

Contributor

Ah, right. I suppose those unexpected logs might precede our 'a', in which case once would fail? The latest commit removes the assert which should work for those cases.

@MarshallOfSound
Copy link
Member

MarshallOfSound left a comment

This probably needs to be documented inside browser-window.md 👍

EDIT: last thing I swear 😆 sorry for the disjointed review

@ajmacd ajmacd requested a review from electron/docs as a code owner Nov 8, 2017

@ajmacd

This comment has been minimized.

Copy link
Contributor

ajmacd commented Nov 8, 2017

Heh, no worries. Added documentation, but I think it rightly belongs in web-contents.md, since it applies to the webContents of e.g. a BrowserView as well. The text is derived from the event in webview-tag.md. I think it's correct to leave that as-is, but let me know what you think.

@ajmacd

This comment has been minimized.

Copy link
Contributor

ajmacd commented Nov 10, 2017

Are any of the existing reviewers able to approve this? 🙇

@poiru

poiru approved these changes Nov 10, 2017

Copy link
Member

poiru left a comment

👍

@MarshallOfSound MarshallOfSound merged commit a06a8a6 into electron:master Nov 11, 2017

5 of 6 checks passed

ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ajmacd ajmacd deleted the ajmacd:allow-browser-window-logs branch Nov 11, 2017

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