Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Sep 15, 2024

Description of Change

Change the EmitWarning() helper functions:

+ EmitWarning(std::string_view msg, std::string_view type);
+ EmitWarning(v8::Isolate*, std::string_view msg, std::string_view type);
- EmitWarning(node::Environment*, std::string_view msg, std::string_view type);

All the old calls to EmitWarning() looked like this:

v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
node::Environment* env = node::Environment::GetCurrent(isolate);
EmitWarning(env, msg, type);

This refactor does two things:

  1. It eliminates the get-isolate-from-JavascriptEnvironment-and-then-get-node-environment code repetition by moving it into EmitWarning()
  2. It eliminates some incidental coupling with Node.js, e.g. there were several files that were only including Node headers in order to be able to get a node::Environment for EmitWarning().

Checklist

Release Notes

Notes: none

@ckerr ckerr added semver/patch backwards-compatible bug fixes target/33-x-y PR should also be added to the "33-x-y" branch. labels Sep 15, 2024
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Sep 15, 2024
@ckerr ckerr marked this pull request as draft September 15, 2024 19:51
@ckerr ckerr marked this pull request as ready for review September 15, 2024 22:01
@ckerr ckerr merged commit 05dfd14 into main Sep 16, 2024
60 checks passed
@ckerr ckerr deleted the refactor/add-EmitWarning-isolate-helper branch September 16, 2024 20:53
@release-clerk
Copy link

release-clerk bot commented Sep 16, 2024

No Release Notes

@trop
Copy link
Contributor

trop bot commented Sep 16, 2024

I was unable to backport this PR to "33-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/33-x-y and removed target/33-x-y PR should also be added to the "33-x-y" branch. labels Sep 16, 2024
ckerr added a commit that referenced this pull request Sep 16, 2024
* refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove EmitWarning(node::Environment*, ...)

* chore: add code comments

* fixup! refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove unused node #includes
@trop
Copy link
Contributor

trop bot commented Sep 16, 2024

@ckerr has manually backported this PR to "33-x-y", please check out #43736

codebytere pushed a commit that referenced this pull request Sep 17, 2024
refactor: add `EmitWarning(v8::Isolate*)` helper (#43722)

* refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove EmitWarning(node::Environment*, ...)

* chore: add code comments

* fixup! refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove unused node #includes
@trop trop bot added merged/33-x-y PR was merged to the "33-x-y" branch. and removed in-flight/33-x-y labels Sep 17, 2024
samuelmaddock added a commit to samuelmaddock/electron that referenced this pull request Sep 19, 2024
samuelmaddock added a commit to samuelmaddock/electron that referenced this pull request Sep 26, 2024
samuelmaddock added a commit to samuelmaddock/electron that referenced this pull request Sep 30, 2024
samuelmaddock added a commit to samuelmaddock/electron that referenced this pull request Oct 1, 2024
jkleinsc pushed a commit that referenced this pull request Oct 10, 2024
fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

fix: content::FrameTreeNodeId lookup and converter

wip

lint

docs: clarify why frame properties may be null

docs: add notice for frame change of behavior

fix: use updated util #43722

maybe fix 🤷
@jkleinsc jkleinsc mentioned this pull request Oct 10, 2024
5 tasks
jkleinsc added a commit that referenced this pull request Oct 11, 2024
* feat: add WebFrameMain detached property

fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

* maybe fix 🤷

* fix: use updated util #43722

* docs: add notice for frame change of behavior

* docs: clarify why frame properties may be null

* lint

* wip

* fix: content::FrameTreeNodeId lookup and converter

* refactor: avoid holey array deoptimization

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
trop bot added a commit that referenced this pull request Oct 11, 2024
* feat: add WebFrameMain detached property

fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

* maybe fix 🤷

* fix: use updated util #43722

* docs: add notice for frame change of behavior

* docs: clarify why frame properties may be null

* lint

* wip

* fix: content::FrameTreeNodeId lookup and converter

* refactor: avoid holey array deoptimization

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

Co-authored-by: Sam Maddock <smaddock@slack-corp.com>
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
* refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove EmitWarning(node::Environment*, ...)

* chore: add code comments

* fixup! refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove unused node #includes
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
* feat: add WebFrameMain detached property

fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

* maybe fix 🤷

* fix: use updated util electron#43722

* docs: add notice for frame change of behavior

* docs: clarify why frame properties may be null

* lint

* wip

* fix: content::FrameTreeNodeId lookup and converter

* refactor: avoid holey array deoptimization

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
* refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove EmitWarning(node::Environment*, ...)

* chore: add code comments

* fixup! refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove unused node #includes
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
* feat: add WebFrameMain detached property

fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

* maybe fix 🤷

* fix: use updated util electron#43722

* docs: add notice for frame change of behavior

* docs: clarify why frame properties may be null

* lint

* wip

* fix: content::FrameTreeNodeId lookup and converter

* refactor: avoid holey array deoptimization

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
* refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove EmitWarning(node::Environment*, ...)

* chore: add code comments

* fixup! refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove unused node #includes
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
* feat: add WebFrameMain detached property

fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

* maybe fix 🤷

* fix: use updated util electron#43722

* docs: add notice for frame change of behavior

* docs: clarify why frame properties may be null

* lint

* wip

* fix: content::FrameTreeNodeId lookup and converter

* refactor: avoid holey array deoptimization

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
codebytere pushed a commit that referenced this pull request Oct 31, 2024
* feat: add WebFrameMain detached property

fix: throw instead of returning null senderFrame

test: detached frames

fix: ensure IPCs of pending deletion RFHs are dispatched

fix: lookup WFM by FTN ID to dispatch IPCs

feat: add frame.isDestroyed()

return null

fix: return undefined

docs: add null to all frame properties

refactor: option c, return null and emit warning

refactor: add routingId & processId to navigation events

test: null frame property

docs: clarify warning message

better wording

clarify null frame

fix: browserwindow spec

* maybe fix 🤷

* fix: use updated util #43722

* docs: add notice for frame change of behavior

* docs: clarify why frame properties may be null

* lint

* wip

* fix: content::FrameTreeNodeId lookup and converter

* refactor: avoid holey array deoptimization

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Sam Maddock <smaddock@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged/33-x-y PR was merged to the "33-x-y" branch. new-pr 🌱 PR opened recently semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.