Conversation from https://chromium-review.googlesource.com/c/chromium/src/+/1060090:
Kyle: For GetSoftwareFeatureState(), GetDeviceId(), GetTruncatedDeviceIdForLogs(), I'd recommend implementing the actual logic here instead. That way, we can make RemoteDevice a "dumb" struct that contains only values with no functions/logic. Also add tests for the first two functions (currently, there are no tests for these since RemoteDevice has no unit tests).
Ryan: Interesting proposal. A few questions/concerns about that, which I'd appreciate your thoughts on:
(1) RemoteDevice also has static methods like TruncateDeviceIdForLogs(). If we move the methods you recommend, it probably also makes sense to move these static methods. The unfortunate (in my opinion) effect of this is that the whole codebase's usage of these methods becomes "RemoteDeviceRef::TruncateDeviceIdForLogs()" -- but perhaps that's better because it highlights RemoteDevice as a "dumb" data object.
(2) Even if we move all methods I mentioned into RemoteDeviceRef, it's still best to leave AreBeaconSeedsEqual, operator==, and operator< in RemoteDevice. This defeats the purpose of making RemoteDevice a "dumb" data object.
(3) Keep in mind that RemoteDevice will become a class that implements RefCounted<T>. As such, is it the right thing to do to treat RemoteDevice as a "dumb" data object?
All in all, I think it's best to leave as-is, but I'm curious what you think.
Kyle:
(1) IMO, TruncateDeviceIdForLogs() should be a function on RemoteDeviceRef().
(2) Agreed - those functions should stay on RemoteDevice itself. Only the "logic" functions should be moved; I think it's reasonable to keep operator==, etc. on RemoteDevice itself.
(3) I'd say it is the right thing to do.
That being said, I'm also fine with you just filing a bug tracking this and doing this later. I do think it's the right long-term plan, but you're correct that it's not super necessary right now.
Comment 1 by khorimoto@chromium.org
, May 16 2018Components: UI>ProximityAuth
Labels: OS-Chrome