Skip to content

Make names of Sender<> and Receiver<> variables more coherent#29710

Merged
bors-servo merged 1 commit intomasterfrom
rename_sender_receiver_in_constellation
May 8, 2023
Merged

Make names of Sender<> and Receiver<> variables more coherent#29710
bors-servo merged 1 commit intomasterfrom
rename_sender_receiver_in_constellation

Conversation

@atbrakhi
Copy link
Copy Markdown
Member

@atbrakhi atbrakhi commented May 4, 2023 •
edited
Loading

Sometimes these variables are called port, sometimes ipc, sometimes they have the name of the thread on the other end and sometimes not. This PR tries to standardize on an obvious name for all of them:

Common pattern to note in this PR:

  • If it is a IpcSender, then naming is followed by ipc_sender, such as: bluetooth_ipc_sender, canvas_ipc_sender etc..
  • If variable is crossbeam_channel sender/receiver then naming is followed by sender/receiver such as: devtools_sender, canvas_sender etc...
  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #29679
  • There are tests for these changes OR
  • These changes do not require tests

@atbrakhi atbrakhi marked this pull request as ready for review May 4, 2023 16:40
@atbrakhi atbrakhi self-assigned this May 4, 2023
mrobinson
mrobinson reviewed May 4, 2023
View reviewed changes
mrobinson
mrobinson reviewed May 4, 2023
View reviewed changes
mrobinson
mrobinson reviewed May 4, 2023
View reviewed changes
mrobinson
mrobinson reviewed May 4, 2023
View reviewed changes
mrobinson
mrobinson reviewed May 4, 2023
View reviewed changes
mrobinson
mrobinson requested changes May 4, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

This is a great start. I think in cases where a sender is merely used to send a response, we shouldn't use the name of the data type that is sent over it, but describe its purpose ie response_sender. I understand that this is an annoying change to make so I'm happy to do that if it's too tedious.

@atbrakhi
Copy link
Copy Markdown
Member Author

atbrakhi commented May 5, 2023 •
edited
Loading

This is a great start. I think in cases where a sender is merely used to send a response, we shouldn't use the name of the data type that is sent over it, but describe its purpose ie response_sender. I understand that this is an annoying change to make so I'm happy to do that if it's too tedious.

Thank you for the review :) I have tried to accumulate these changes in recent commit.

@mrobinson mrobinson force-pushed the rename_sender_receiver_in_constellation branch from c4ba6eb to 9f4fd0d Compare May 8, 2023 07:20
@mrobinson mrobinson force-pushed the rename_sender_receiver_in_constellation branch from 9f4fd0d to 55de8e3 Compare May 8, 2023 07:28
@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented May 8, 2023

@bors-servo r+

Thank you. I've made a couple small change and squashed the commits.

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented May 8, 2023

📌 Commit 55de8e3 has been approved by mrobinson

@atbrakhi
Copy link
Copy Markdown
Member Author

atbrakhi commented May 8, 2023

@bors-servo retry

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented May 8, 2023

@atbrakhi: 🔑 Insufficient privileges: not in try users

@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented May 8, 2023

bors is working the way through the merge queue, so it might take a little while before it gets to this change.

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented May 8, 2023

⌛ Testing commit 55de8e3 with merge 61f872e...

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 8, 2023

Test results for linux-wpt-layout-2013 from try job (#4913697960):

Flaky unexpected result (19)
  • TIMEOUT [expected OK] /FileAPI/url/url-charset.window.html (#26997)
    • TIMEOUT [expected PASS] subtest: Blob charset should override any auto-detected charset. Test timed out
    • TIMEOUT [expected PASS] subtest: Blob charset should override <meta charset>. Test timed out
  • TIMEOUT /FileAPI/url/url-in-tags-revoke.window.html (#19978)
    • TIMEOUT [expected FAIL] subtest: Opening a blob URL in a new window immediately before revoking it works. Test timed out
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-uniform-packing-restrictions.html (#28103)
    • NOTRUN [expected PASS] subtest: Overall test
  • TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/shader-with-non-reserved-words.html (#16216)
    • NOTRUN [expected PASS] subtest: Overall test
  • OK [expected TIMEOUT] /fetch/api/redirect/redirect-keepalive.any.html (#29536)
  • ERROR [expected TIMEOUT] /html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html (#28541)
  • TIMEOUT [expected OK] /html/browsers/browsing-the-web/history-traversal/srcdoc/consecutive-srcdoc.html (#29084)
    • TIMEOUT [expected FAIL] subtest: changing srcdoc to about:srcdoc#yo then another srcdoc does two push navigations and we can navigate back Test timed out
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin.window.html (#29049)
    • PASS [expected FAIL] subtest: Same-origin navigation started from unload handler must be ignored
  • OK /html/browsers/browsing-the-web/navigating-across-documents/replace-before-load/a-click.html (#28697)
    • PASS [expected FAIL] subtest: aElement.click() before the load event must NOT replace
  • OK /html/browsers/history/the-history-interface/traverse_the_history_1.html (#21383)
    • FAIL [expected PASS] subtest: Multiple history traversals from the same task assert_array_equals: Pages opened during history navigation expected property 1 to be 2 but got 1 (expected array [4, 2] got [4, 1])
  • TIMEOUT [expected OK] /html/browsers/sandboxing/sandbox-initial-empty-document-toward-same-origin.html
  • TIMEOUT [expected OK] /html/interaction/focus/the-autofocus-attribute/document-with-fragment-top.html (#28259)
    • TIMEOUT [expected FAIL] subtest: Autofocus elements in top-level browsing context's documents with "top" fragments should work. Test timed out
  • OK /html/semantics/forms/form-submission-0/urlencoded2.window.html (#28687)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: Basic File test (formdata event)
  • OK /html/semantics/forms/historical.html (#28568)
    • PASS [expected FAIL] subtest: <input name=isindex> should not be supported
  • OK /html/webappapis/dynamic-markup-insertion/document-write/module-static-import-delayed.html (#26243)
    • FAIL [expected PASS] subtest: document.write in an imported module assert_true: onload must be called expected true got false
  • TIMEOUT [expected OK] /html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.html (#21616)
    • TIMEOUT [expected PASS] subtest: Reloading a document.open()'d page should reload the URL of the entry realm's responsible document Test timed out
  • TIMEOUT [expected OK] /html/webappapis/scripting/processing-model-2/integration-with-the-javascript-job-queue/promise-job-entry.html (#25805)
    • TIMEOUT [expected FAIL] subtest: Fulfillment handler on pending-then-fulfilled promise Test timed out
    • TIMEOUT [expected FAIL] subtest: Rejection handler on pending-then-rejected promise Test timed out
  • OK [expected TIMEOUT] /webaudio/the-audio-api/the-audiocontext-interface/audiocontext-not-fully-active.html (#27664)
  • TIMEOUT [expected OK] /webmessaging/with-ports/018.html (#24485)
    • TIMEOUT [expected PASS] subtest: origin of the script that invoked the method, javascript: Test timed out
Stable unexpected results that are known to be intermittent (15)

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented May 8, 2023

☀️ Test successful - checks-github
Approved by: mrobinson
Pushing 61f872e to master...

@bors-servo bors-servo merged commit 61f872e into master May 8, 2023
@bors-servo bors-servo deleted the rename_sender_receiver_in_constellation branch May 8, 2023 11:56
@bors-servo bors-servo mentioned this pull request May 8, 2023
Closed
5 tasks
@atbrakhi atbrakhi mentioned this pull request May 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrobinson mrobinson mrobinson requested changes

Assignees

@atbrakhi atbrakhi

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@atbrakhi @mrobinson @bors-servo