[rfc][icedtea-web] Security dialogs wait for PolicyEditor to finish parsing

[Andrew Azores]

On 03/26/2014 07:37 AM, Jiri Vanek wrote:
> On 03/25/2014 09:36 PM, Andrew Azores wrote:
>> On 03/24/2014 12:01 PM, Jiri Vanek wrote:
>>> On 03/24/2014 04:46 PM, Andrew Azores wrote:
>>>> On 03/24/2014 11:11 AM, Jiri Vanek wrote:
>>>>> On 03/24/2014 03:44 PM, Andrew Azores wrote:
>>>>>> On 03/24/2014 10:40 AM, Jiri Vanek wrote:
>>>>>>> On 03/24/2014 03:01 PM, Andrew Azores wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> PolicyEditor loads and saves policy files async, obviously. 
>>>>>>>> However, this means that opening
>>>>>>>> a new
>>>>>>>> PolicyEditor instance on an existing policy file and 
>>>>>>>> programmatically adding a new codebase
>>>>>>>> to the
>>>>>>>> editor does not have a well-defined order. This can be 
>>>>>>>> problematic because PolicyEditor
>>>>>>>> highlights/selects the last added codebase, so eg in the 
>>>>>>>> scenario where the editor is being
>>>>>>>> launched
>>>>>>>> from a security dialog, we want to be sure that the current 
>>>>>>>> applet's codebase is the one
>>>>>>>> selected
>>>>>>>> when the editor appears. This patch adds a method to 
>>>>>>>> PolicyEditor that indicates whether the
>>>>>>>> editor
>>>>>>>> is currently reading/writing a file, and adds a loop to the two 
>>>>>>>> security dialogs so that the new
>>>>>>>> codebase is not added and the editor made visible until it 
>>>>>>>> completes its IO.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>
>>>>>>> Hm... I dont think this is correct approach. Why are you so 
>>>>>>> strongly against modal dialogue?
>>>>>>>
>>>>>>>
>>>>>>> J.
>>>>>>
>>>>>> I'm not... making PolicyEditor modal will come later. This patch 
>>>>>> is just about making sure that
>>>>>> when
>>>>>> you launch PolicyEditor from a security dialog, the current 
>>>>>> applet's codebase is guaranteed to be
>>>>>> the selected one when the editor appears. This and modality are 
>>>>>> completely separate.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>> uff... wouldnt be much more easy to make the load/save in sync?  I 
>>>>> do not see any reason why it is
>>>>> in new thread Thread(save/load).start() ....
>>>>>
>>>>>
>>>>> J.
>>>>
>>>> I really don't want to be doing it sync on the UI thread. This 
>>>> makes the application less
>>>> responsive, and is generally bad practice. Not worth the benefit of 
>>>> making it easier to guarantee
>>>> the programmatically added new codebases come after. There are 
>>>> better ways to do it async than
>>>> manually spawning a new Thread, though, yes. eg SwingWorker as you 
>>>> said on IRC, or Executors and
>>>> Runnables as I suggested. Still, these potential enhancements to 
>>>> the multithreading model would
>>>> still have the codebase ordering problem.
>>>
>>> I agree thath do it in gui thread is wrong. But I disagree with 
>>> current aproach and new suggested
>>> locking.
>>>
>>> Correct aporach is:
>>> invoke thread (swingworker)
>>>   disable gui or show dialog with progressbar or soem wait please
>>>    (so gui will be "responsoble")
>>> wait for thread.
>>>
>>>
>>> Would be correct approach.
>>
>> Okay, this is an enhancement in that it uses the nice SwingWorker 
>> abstraction and provides a visual
>> indicator of progress for the IO operation, but I don't see how it 
>> really solves the problem. If by
>> "wait for thread" you mean Thread#join or similar, then we're again 
>> just blocking the EDT, so it
>> won't actually be responsive until the IO is done. In that case we 
>> might as well just have the EDT
>> do the IO directly. If we don't block the EDT and continue to let the 
>> IO complete async, albeit with
>> visual progress indication and nicely abstracted, then there is still 
>> the problem of addNewCodebase
>> being called externally and not having a well-defined order compared 
>> to the addNewCodebase calls
>> made by the thread performing the IO.
>
>
> Agree. Then please do this now:
>
> ** failed to import extension forest from 
> /home/jvanek/hg-forest-ext/forest.py: No module named repo
> diff -r 4c584d98c42d 
> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
> --- 
> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java 
> Wed Mar 26 12:21:48 2014 +0100
> +++ 
> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java 
> Wed Mar 26 12:37:11 2014 +0100
> @@ -1067,7 +1067,7 @@
>                      OutputController.getLogger().log(e);
>                  }
>              }
> -        }.start();
> +        }.run();
>      }
>
>      /**
> @@ -1170,7 +1170,7 @@
>                      showCouldNotSaveDialog();
>                  }
>              }
> -        }.start();
> +        }.run();
>      }
>
>      /**
>
>
>>
>>>
> because :
>>>
>>> However - the polici files *are* small. So the burden of *doing it 
>>> IN*  AWT is imho acceptable.
>>>
>>>
>

Good to go?

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: policyeditor-io-sync.patch
Type: text/x-patch
Size: 1236 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140326/e0ff10e8/policyeditor-io-sync.patch>