[rfc][icedtea-web] do not dye if jnlp's icons points to nothing (eg 404)

[Jie Kang]

----- Original Message -----
> On 02/02/2015 03:26 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 02/02/2015 01:57 PM, Jiri Vanek wrote:
> >>> This patch is fixing state, when icon is expected to be downlaoded, but
> >>> its
> >>> location do not exists.
> >>> Then when this code expect it to be found, NPE is thrown.
> >>>
> >>> Hmm.. Looking to this after break,
> >>>
> >>> - location = CacheUtil.getCachedResource(uiconLocation, null,
> >>> UpdatePolicy.SESSION)
> >>> -                     .toString();
> >>> + locationURL = CacheUtil.getCachedResource(uiconLocation, null,
> >>> UpdatePolicy.SESSION);
> >>> + if (locationURL == null) {return null;}
> >>> + location = locationURL.toString();
> >>>
> >>> Is probably much more suitable (need to test it)
> >>>
> >>>
> >>> J.
> >> Or maybe this third attempt?
> >>
> >> Note - this is not refactoring, it changes behaviou, but I have not found
> >> more usages of
> >> getCachedResource ...
> >
> > Hello,
> >
> > Ignore my previous e-mail;
> >
> > This looks much better!
> >
> >
> >
> > +    public static File  getCachedResourceFile(URL location, Version
> > version, UpdatePolicy policy) {
> >
> > Can we add comments to this function?
> 
> did
> >
> >
> >       public static URL getCachedResource(URL location, Version version,
> >       UpdatePolicy policy) {
> >
> > Since we have getCachedResourceFile, maybe rename this to
> > getCachedResourceURL?
> >
> >
> > +            //this throws npe, if url (specified in jnl) points to 404
> >
> > s/jnl/jnlp
> 
> fixed
> >
> >
> > +                    cantCache();
> >                   }
> >               } catch (IOException ex) {
> >                   //favicon 404 or similar
> >
> > With these changes, is this IOException catch still relevant? I am
> > wondering, we choose to throw the NonFileProtocolException up to the
> > caller, and the function says:
> 
> I believe yes.
> >
> >      private void cacheIcon() throws IOException, NonFileProtocolException
> >      {
> >
> > How come we don't throw this IOException up too?
> >
> > So in:
> >      public void
> >      createDesktopShortcuts(AccessWarningPaneComplexReturn.ShortcutResult
> >      menu, AccessWarningPaneComplexReturn.ShortcutResult desktop, boolean
> >      isSigned) {
> >                  try {
> >                      cacheIcon();
> >                  } catch (NonFileProtocolException ex) {
> >                      OutputController.getLogger().log(ex);
> >                      //default icon will be used later
> >                  }
> >
> > Maybe we should catch the IOException here as well? I am not too familiar
> > with this code so any explanations would be very helpful!
> >
> 
> It seems to me like two different cases. Failure in first is leading to
> seccond attempt. The last
> throw if ioexception is from copy. Thats very rare exception and our reaction
> should be different.
> >
> 
>   Regards,
>    And tY for check!

Hello,

One nit:

diff -r b4ac69f5297e netx/net/sourceforge/jnlp/util/XDesktopEntry.java
--- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java	Fri Feb 06 15:14:32 2015 +0100
+++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java	Mon Feb 09 15:10:10 2015 +0100
@@ -431,10 +431,14 @@
 
         String location = null;
         if (uiconLocation != null) {
-            location = CacheUtil.getCachedResource(uiconLocation, null, UpdatePolicy.SESSION)
-                    .toString();
+            //this throws npe, if url (specified in jnl) points to 404

s/jnl/jnlp


Good to go with fix and without re-review.


Regards,


> 
>   J.
> 
> 
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern