<AWT Dev> [9] Review request: 8040007 GtkFileDialog strips user inputted filepath

[Sergey Bylokhov]

Hi, Alexander.
The fix looks good.

On 6/9/14 8:58 PM, Anthony Petrov wrote:
> Hi Alexander,
>
> Thanks for the update. The fix looks good to me.
>
> Note that myself, I wouldn't change the existing error handling code 
> (e.g. old LN 224-225 and 231-232 in sun_awt_X11_GtkFileDialogPeer.c), 
> but I'm still OK with that. Leaving this up to you and other reviewers.
>
> -- 
> best regards,
> Anthony
>
> On 6/9/2014 7:07 PM, Alexander Zvegintsev wrote:
>> Hi Anthony,
>>
>> thanks for the review, please see the updated webrev:
>> http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/01/
>>
>>> I assume you've tested this case, and it still works fine, right?
>> Sure, it works fine. That is why gtk_file_chooser_get_current_folder()
>> is not used and
>> isFromSameDirectory() was introduced.
>>
>>
>> -- 
>> Thanks,
>> Alexander.
>>
>> On 06/09/2014 06:07 PM, Anthony Petrov wrote:
>>> Hi Alexander,
>>>
>>> src/solaris/native/sun/awt/sun_awt_X11_GtkFileDialogPeer.c
>>>>  176         free(prevDir);
>>>>  177         prevDir = strdup(dir);
>>>
>>> It's unnecessary to re-duplicate the prevDir on each loop iteration
>>> here. I suggest to initialize it once instead. The less pointer
>>> operations, the less room for bugs.
>>>
>>>
>>>>  229 (*env)->ExceptionClear(env);
>>>>  230         JNU_ThrowInternalError(env, "Could not instantiate
>>>> current folder");
>>>
>>> This error message sounds misleading because the code above doesn't
>>> instantiate a "folder", it tries to instantiate a string. Also, if
>>> exceptions did occur, I believe it's fine to report them as they are.
>>> Otherwise, if the pointer is NULL, then this looks more like an OOM
>>> than an internal error to me.
>>>
>>>
>>>>  278         //This is a hack for use with "Recent Folders" in gtk
>>>> where each
>>>>  279         //file could have its own directory.
>>>
>>> I assume you've tested this case, and it still works fine, right?
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>> On 6/6/2014 8:00 PM, Alexander Zvegintsev wrote:
>>>> Hello,
>>>>
>>>> please review the fix
>>>> http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/00/
>>>> for the bug
>>>> https://bugs.openjdk.java.net/browse/JDK-8040007
>>>>
>>>> With this fix we don't use current folder from native GtkFileChooser
>>>> anymore, now we build it by ourselves.
>>>>
>>>> [1]
>>>> https://developer.gnome.org/gtk2/stable/GtkFileChooser.html#gtk-file-chooser-get-filenames 
>>>>
>>>>
>>>>
>>>> [2]
>>>> https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-path-get-dirname 
>>>>
>>>>
>>>>
>>>>
>>


-- 
Best regards, Sergey.