1859759 - network/sctp: Refactor DataChannel

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Victor Boivie]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/118.0.0.0 Safari/537.36

Steps to reproduce:

network/sctp/datachannel would benefit from some refactoring wrt code style.

Comment 1

[Victor Boivie]

So I created this bug to have as a reference for some refactoring that I think would be good to do in the data channel files. If you think this is fine, please assign it to me. I'm also on chat - thanks

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Can you give some examples of the types of changes you're suggesting? In changeset: 448947:6f3709b38781 (Nov 30 2018) we reformatted the existing code according to our style guidelines (based on google style), and we have automatic reformatting to current style guidelines typically done in our workflow/tools.

Comment 3

[Victor Boivie]

Some ideas that come to mind:

• Using type safe enum class instead of enum (https://abseil.io/tips/86)
• Use of const (https://abseil.io/tips/109)
• Replacing typedefs with using (from style guide)

... but honestly, I hope to fix a few bugs in this domain and I generally prefer to refactor the code a bit as I learn it.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Those sound like reasonable changes, though certainly not required. We'd accept/review patches doing that, thanks!

Comment 5

[Victor Boivie]

Attachment: Bug 1859759: Use enum class for data channel states

There are two states; One for the entire data channel connection, which is
an internal property (now called just "state") indicating the state of the
SCTP connection setup.

The other state is the per-data channel state, indicating if it's opening, open
or closing. This is called "readyState" in the RTCDataChannel API, and that
(somewhat odd) name is kept in the underlying C++ representation.

This commit changes from using a C style enum to a C++ enum class which adds
type safety. Some minor code style cleanup was done as well as code was
refactored.

Comment 6

[Victor Boivie]

Attachment: Bug 1859759: Use enum class for events/message types

This commit changes from using a C style enum to a C++ enum class which adds
type safety. This is just a refactoring commit with no changes in behavior.

Depends on D192066

Comment 7

[Victor Boivie]

Attachment: Bug 1859759: Use enum class for pending message type

This commit changes from using a C style enum to a C++ enum class which adds
type safety. This is just a refactoring commit with no changes in behavior.

Depends on D192068

Comment 8

[Victor Boivie]

Attachment: Bug 1859759: Use enum class for partial reliability policy

This commit changes from using a C style enum to a C++ enum class which adds
type safety. This is just a refactoring commit with no changes in behavior.

This avoids an (potentially unsafe) untyped downcast from PeerConnection.idl
types to what's used in DataChannel.h.

Depends on D192069

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=25cf082fa2232b2e3551cb31c6ae2c8611ed2212
https://treeherder.mozilla.org/jobs?repo=try&revision=30b60e04e37be71dae75ecc4f095713695c56b0b
https://treeherder.mozilla.org/jobs?repo=try&revision=aa1b5560c99b8addcca0c4566fda57accf1afc79

Comment 10

[Byron Campen [:bwc]]

I see one DataChannel-related failure in there, but I think this is probably pre-existing. Retriggering and making a baseline push to check:

https://treeherder.mozilla.org/jobs?repo=try&revision=7ffcaa81ce1de277d488952df34ee513e0d0644f

Comment 11

[Victor Boivie]

Attachment: Bug 1859759: Misc c++ style fixes

Including, but not limited to:

• Following the style guide wrt declaration order.
• Use const whenever it makes sense.
• Move members to be private where possible (e.g. mLock was previously public,
  which it didn't have to be).
• Fix most compiler warnings.

These kind of changes are rarely complete - there is always something that was
missed. It's still a step in the right direction.

Comment 12

[Phabricator Automation]

Comment on attachment 9361586 [details]
Bug 1859759: Misc c++ style fixes

Revision D192563 was moved to bug 1862740. Setting attachment 9361586 [details] to obsolete.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77c6af541a02
Use enum class for data channel states r=jesup,bwc
https://hg.mozilla.org/integration/autoland/rev/1516f20cdd30
Use enum class for events/message types r=bwc
https://hg.mozilla.org/integration/autoland/rev/8f33fd78d7ac
Use enum class for pending message type r=bwc
https://hg.mozilla.org/integration/autoland/rev/0d91ef074019
Use enum class for partial reliability policy r=jesup,bwc

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/77c6af541a02
https://hg.mozilla.org/mozilla-central/rev/1516f20cdd30
https://hg.mozilla.org/mozilla-central/rev/8f33fd78d7ac
https://hg.mozilla.org/mozilla-central/rev/0d91ef074019