Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup java.io)

[Weijun Wang]

On 12/03/2011 06:12 AM, Stuart Marks wrote:
> I'm adding Weijun (Max) Wang to this thread.
>
> The same "ackbarfaccept" code had come up a third time when I was
> reviewing some of Max's changes. The code in question all has to do with
> permissions, and Max is in the security group, so he might have a better
> insight whether doing a refactoring is appropriate and how to approach
> doing it.

Maybe we can group all permissions with a target and a predefined set of 
actions under a new abstract class.

-Max

>
> Some searching around reveals that "ackbarfaccept" appears in the
> PlayerPermission class of the Java ME Mobile Media API (not in any of
> the OpenJDK repositories) so it looks like the code has been copied even
> more times than are visible here. Perhaps a public permissions parsing
> utility method is called for.
>
> s'marks
>
> On 12/2/11 9:02 AM, Sebastian Sickelmann wrote:
>> Am 02.12.2011 16:27, schrieb Brandon Passanisi:
>>> Hi Sebastian. I'm not sure if you had seen the e-mail from Stuart Marks
>>> regarding this, but Stuart was able to find more instances of the
>>> similar
>>> block of "fallthrough" code. I can volunteer to apply your upcoming
>>> change
>>> to FilePermission to the other files if you wish. Or, you can try
>>> applying
>>> the change to the other files, but if you don't have time I don't
>>> mind doing
>>> it. Here's the section of Stuart's e-mail on this topic:
>>>
>>> -------------------------------------------------------------------------------------
>>>
>>> (Incidentally, this is the third time I've reviewed code today that
>>> looks exactly like this. The other cases are in java.io.FilePermission
>>> and java.util.PropertyPermission. They each have the /*FALLTHROUGH*/
>>> into a set of cases that do nothing but break; and they have similar
>>> ("ackbarfaccept") comments. It would be nice if these chunks of code
>>> could be unified, but they differ in a number of fiddly details.)
>>>
>>> (The string "ackbarfaccept" occurs in the following files:
>>> 1. java/io/FilePermission.java
>>> 2. java/net/SocketPermission.java
>>> 3. java/util/PropertyPermission.java
>>> 4. javax/management/MBeanPermission.java
>>> 5. javax/security/auth/kerberos/ServicePermission.java
>>> Hmmm.)
>>> -------------------------------------------------------------------------------------
>>>
>>>
>>> Thanks.
>>
>> Hi Brandon,
>> Hi Stuart,
>>
>> i had a look at all those classes and it seems to be the same
>> algorithm. In an
>> normal project (not the jdk) i would suggest to completely refactor it
>> and make
>> some code de-duplication cleanup. But if i thinks on projects like
>> jigsaw this
>> can easily get a real problem. What do you think, should we try to
>> cleanup them
>> all. Or should we try to make some de-duplication/code-reuse refactoring.
>>
>>
>> -- Sebastian
>>>
>>> On 12/1/2011 10:18 PM, Sebastian Sickelmann wrote:
>>>> Hi Brandon,
>>>>
>>>> i will try to work out a fix for both and cc the review request to you.
>>>>
>>>> -- Sebastian
>>>>
>>>> Am 01.12.2011 23:54, schrieb Brandon Passanisi:
>>>>> Hi Sebastian. I was speaking with Stuart Marks earlier today and he
>>>>> mentioned that the "fallthrough" code in FilePermission.java also
>>>>> exists in
>>>>> java.util.PropertyPermission.java. Maybe the code author had done some
>>>>> copy/paste when working on these files. Stuart had said that you
>>>>> might be
>>>>> planning on doing some work on this after the warnings cleanup work.
>>>>> If/when you do this work, maybe you can let me know so that I can
>>>>> sync the
>>>>> same changes you apply to FilePermission.java to
>>>>> PropertyPermission.java?
>>>>> Or, maybe you can apply the same changes yourself to
>>>>> PropertyPermission.java?