RFR: 8351339: WebSocket::sendBinary assume that user supplied buffers are BIG_ENDIAN [v3]

[Mark Sheppard]

On Mon, 17 Mar 2025 08:05:54 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> persisting with method nomenclature (or painting the bikeshed again). What’s in a name?
>> 
>>     reset is really init() or setMask()
>> 
>>     Currently you have a static applyMask() method and a non static applyMask() — is that ideal ?
>> 
>>    The Galloping mask methods, there has to be a more semantically meaningful name  e.g. applyLongMaskRepeating or applyVectorMaskRepeating
>>     (on your horse rather than on yer bike ;-)
>> 
>> 
>>   I still think the following looking meaningful
>> 
>>         public void applyMask(ByteBuffer src, ByteBuffer dst, theMask) {
>>             if (src.order() == dst.order()) {
>>                 initLongVectorMask(src, dst, theMask);
>>                 applyLongVectorMask(src, dst, theMask);
>>             }
>>             endApplyMask(src, dst, theMask);
>>         }
>
> @msheppar, thanks so much for taking time to re-review the changes. In bb81642acf10159b81f45e3260448a509f4d64c9, I've incorporated your feedback with following `Masker` method name changes:
> 
> * `reset` -> `setMask`
> * `initGallopingMask` -> `initVectorMask`
> * `applyGallopingMask` -> `applyVectorMask`
> 
> I did not use the term `long` in the method name, since it is an internal implementation detail. Vectorization can be implemented in several different ways. The important bit here is the fact that we have two masking routine variants: _plain_ and _vectorized_, and this now we capture in method names.
> 
> I also didn't want to use the term `end` in the method name, because it doesn't _end_ anything – it is simply _plain_ masking without any catches. I could have used `applyByteMask`, but again, this one-`byte`-at-a-time operation is an implementation detail, which I did not want to disclose in the method name.
> 
>> Currently you have a static `applyMask()` method and a non static `applyMask()` — is that ideal ?
> 
> No, it is not. I liked your earlier idea of `Masker` creating a state object that the call-site can pass to masking methods. There are even further optimizations we can carry out – e.g., `MessageDecoder` is not re-using `Masker` instances, whereas `MessageEncoder` does. But as discussed earlier, we want to keep the scope limited with the fix to the problem at hand without disrupting the code base much.

@vy  thanks for taking feedback into consideration

the updated method names
initVectorMask, applyVectorMask, applyPlainMask

look like they "are on the money".  👍

-------------

PR Comment: https://git.openjdk.org/jdk/pull/24033#issuecomment-2733679310