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

Emit console-message in OSR mode #11921

Merged
merged 2 commits into from Feb 14, 2018

Conversation

Projects
None yet
5 participants
@codebytere
Copy link
Member

codebytere commented Feb 14, 2018

Resolves #11919.

This removes the check such that console-message can be fired even when rendering in offscreen.

@codebytere codebytere requested a review from electron/reviewers as a code owner Feb 14, 2018

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Feb 14, 2018

Basically the same as #11056, if these become handled they won't get logged by --enable-logging anymore. Breaking changes for 2.x are 🆗with me

Emit("console-message", level, message, line_no, source_id);
return true;
}
Emit("console-message", level, message, line_no, source_id);

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 14, 2018

Member

Actually, can we return Emit

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 14, 2018

Member

That would semantically be correct as it returns false if the event isn't handled in JS land 👍

@brenca brenca self-requested a review Feb 14, 2018

@brenca

brenca approved these changes Feb 14, 2018

Copy link
Member

brenca left a comment

👍

@ckerr

ckerr approved these changes Feb 14, 2018

Copy link
Member

ckerr left a comment

The description of DidAddMessageToConsole in libcc reads:

  // A message was added to the console of a frame of the page. Returning true
  // indicates that the delegate handled the message. If false is returned the
  // default logging mechanism will be used for the message.
  virtual bool DidAddMessageToConsole(WebContents* source,

IMO return Emit(...) is closer in spirit to the existing code so I am 👍 on return Emit for 2.0

@codebytere codebytere merged commit 7e2f760 into master Feb 14, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the allow-osr-console-message branch Feb 14, 2018

@brian-mann

This comment has been minimized.

Copy link

brian-mann commented Feb 14, 2018

So this will land in 2.x.x ?

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