[rfc][icedtea-web] bringing applets out of browser (part1)
Jiri Vanek
jvanek at redhat.com
Tue Dec 16 15:32:09 UTC 2014
On 12/12/2014 05:12 PM, Jie Kang wrote:
> Hello,
>
>
> Review in code below:
Thank you for review!
>
> diff -r 52160fef5621 netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java Fri Dec 05 18:21:52 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java Fri Dec 05 19:57:14 2014 +0100
> @@ -294,7 +294,7 @@
> [...]
> - private static InputStream openURL(URL location, Version version, UpdatePolicy policy) throws IOException {
> + public static InputStream openURL(URL location, Version version, UpdatePolicy policy) throws IOException {
>
> This part might have a number of un-wanted side-effects.
What side effect you mean?
Actually this is the only *correct* way, how any resource should be downloaded. So the method
should be public, and actually highlighted to prevent others from inviting wheel.
>
> I think you should consider adding a new function: public static String getJNLPFileAsString(...) and use that instead of making this public.
=> kept.
>
> + OutputController.getLogger().log((debugJnlp==null)?"null":debugJnlp);
>
> Can you fix the spacing here to: (debugJnlp==null) ? "null" : debugJnlp
>
> [...]
> + public String toJnlp(boolean needSecurity, boolean useHref, boolean fix) {
> + if (useJNLPHref && debugJnlp!=null && useHref) {
> + OutputController.getLogger().log("Using debugjnlp as return value toJnlp");
>
> Indentation
should be fixed.
>
> [...]
> + public static String strippClkass(String s) {
>
> s/Clkass/Class
fixed
>
> [...]
> + private String fixCommonIsuses(boolean needSecurity, String orig) {
> [...]
> + static String fixCommonIsuses(boolean needSecurity, String orig, String codebase, String title, String vendor) {
>
> Can you reorder the functions to be together?
done
>
> [...]
> + //are tested
>
> Can you expand on the comment here: tested for what?
done
>
> + static final String SANDBOX_REGEX = toBaseRegex("sandbox", false);
> + static final String CLOSE_INFORMATION_REGEX = toBaseRegex("information",true);
> + static final String SECURITY_REGEX = toBaseRegex("security", false);
> [...]
> + OutputController.getLogger().log("no information element Found. Trying to fix");
> + OutputController.getLogger().log("jnlphreff dif not had codebase. Fixing");
> + OutputController.getLogger().log("'.' codebase found. fixing");
> + OutputController.getLogger().log("all-permissions not found and app is signed.");
> + OutputController.getLogger().log("adding sdecurity element");
>
> Please use consistent punctuation and capitalization. s/jnlphreff/jnlphref s/dif/did s/sdecurity/security
ty! fixed.
>
> [...]
>
> + private AccessWarningPaneComplexReturn shouldCreateShortcut(ShortcutDesc sd) {
>
> This name is quite hard to understand. What is a ComplexReturn? Can you think of a name that describes exactly what it provides?
:( M imagination failed and stayed with this as with best possible name.
It is Return from AccessWarningPane, and is Complex(not plain number) So I really think
AccessWarningPaneComplexReturn is good. If you strongly disagree, you will have to invent one :P
>
> Maybe we should consider replacing AccessWarningPane with this class slightly expanded?
What do you mean?
>
> [...]
>
> + HtmlShortuctPanel htmlPanelDesktop;
> + HtmlShortuctPanel htmlPanelMenu;
> + RemeberPanel remberPanel;
>
> s/HtmlShortuctPanel/HtmlShortcutPanel s/RemeberPanel/RememberPanel
shamefully fixed.
>
> [...]
>
>
> + JRadioButton byApp = new JRadioButton("rembere by application");
> + JRadioButton byPage = new JRadioButton("rembere by domain");
> + JRadioButton dont = new JRadioButton("don't rember", true);
>
> s/rembere/remember s/rember/remember
also fixed.
>
> [...]
>
> + byApp.setToolTipText("<html>This application will never ask more permissions questions</html>");
>
> I think a more appropriate message would be "Remember permissions for this application and do not ask again"
>
> + byPage.setToolTipText("<html>All applications from this domain will stop asking, and will follow your current decission on all permissions</html>");
>
> "Remember permissions for all applications for this domain and do not ask again"
>
> + dont.setToolTipText("<html>Your decission will be used only for this single permission for this single run</html>");
>
> "Do not remember permissions"
>
I have improved the sentences a bit, but not by those of yours.
My reason was, that your sentences seems to me like just being more verbose copies of the text on
button their are tooltiping.
I would like to have the tooltip to complete the buttons description. so my sentences:
+EXAWrememberByAppTooltip=<html>This application will never ask more permissions questions</html>
It must be clear, that that *all* permissions are approved/declined by this *forever*, unless they
are changed in itweb-settings
+EXAWrememberByPageTooltip=<html>All applications from this domain will stop asking, and will follow
your current decision on all permissions</html>
Sae, with sound on *domain*
Both above are *not* *yet* implemented But I would like to do so for 1.6 The following one is
implemented (== no change in current behaviour)
+EXAWdontRememberTooltip=<html>Your decision will be used only for this single permission for this
single run</html>
It must be clear that it will keep asking for all other types of permissions.
So maybe add "use in this runtime for all permissions" is probably missing option :)
>
> [...]
>
> + public HtmlShortuctPanel() {
> + super(new FlowLayout(FlowLayout.CENTER, 1, 5));
> + this.setBorder(new EmptyBorder(0, 0, 0, 0));
> + this.add(browser);
> + browsers.setEditable(true);
> + browsers.setSelectedItem(XDesktopEntry.getBrowserBin());
> + this.add(browsers);
> + this.add(jnlpGen);
> + this.add(jnlpHref);
> + this.add(javawsHtml);
> + this.add(fix);
> + browser.setToolTipText("<html>Browser shortcut<br><li>This option will create shortcut to open your browser with current page loaded</li><li>If your browser support offline run, this is the safest option</li></html>");
> + browsers.setToolTipText("<html>browser used for lunching this applet (will lunch icedtea-web later)<br><li>The default browser was preset</li><li>Feel free to include browser of your choice</li></html>");
> + jnlpGen.setToolTipText("<html><li>The jnlp file will be generated from your current html page</li><li>Once you lunch jor shortcut, javaws will lunch this jnlp file</li><li>This applet will then run <b>without</b> the browser</li><li>However experimental, this is working surprisingly well.</li></html>");
> + jnlpHref.setToolTipText("<html>Some aplets are jsut pointing to jnlp file, which is containing actual informations about the resources of this app.<br><li>By selecting this option, this jnlp file will be saved and used for later lunches.</li><li>Javaws will be the luncher, and this applet will run <b>out</b> of browser</li><li>Howeer good htis sounds, this si surprisingly not working</li></html>");
> + javawsHtml.setToolTipText("<html>BY using -html switch, javaws can try to parse html and try to extract applet, and to lunch it out of browser<br><li>highly experimental</li><li>really cool</li><li>not yet implemented</li></html>");
> + fix.setToolTipText("<html>Some jnlps pointed from applet are not designed to be used as jnlp apps<br><li>This will add the known often missing elements to this file (if missing)</li></html>");
> + ButtonGroup bg = new ButtonGroup();
> + bg.add(browser);
> + bg.add(jnlpGen);
> + bg.add(jnlpHref);
> + bg.add(javawsHtml);
> + browser.addActionListener(al);
> + jnlpGen.addActionListener(al);
> + jnlpHref.addActionListener(al);
> + javawsHtml.addActionListener(al);
> + al.actionPerformed(null);
> + this.validate();
> +
> + }
>
> Can you add more spacing in this constructor to make it easier to read. As well, it would be great if you could split it into multiple functions with readable names. That way we can understand what each part is doing.
done
>
> E.g.
>
> public HtmlShortuctPanel() {
> addComponents()
> setupTooltipTexts()
> setupButtonGroup()
> setupActionListener()
done
> [...]
> }
>
> [..]
> diff -r 52160fef5621 netx/net/sourceforge/jnlp/security/dialogs/AccessWarningPaneComplexReturn.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/security/dialogs/AccessWarningPaneComplexReturn.java Fri Dec 05 19:57:14 2014 +0100
> @@ -0,0 +1,148 @@
> [...]
> +
> +
> +
> +
> +
> +
> +
> +
>
> Please remove these spaces :)
done.
>
> - Integer buttonIndex;
> + Object buttonIndex;
I must disagree. The object is the only possible return value here. See how this variable is handled
in other security.Dialogs. One is using this to returnn, one something else, some supports
remmbering, some do not....
From me its +1 for object and lets every dialog returns what it wants.
If those should be unified, it will be a lot of work.
>
> I think using Object directly is not very safe. How about making a new class to represent this?
>
> [...]
> diff -r 52160fef5621 netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> --- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Dec 05 18:21:52 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Dec 05 19:57:14 2014 +0100
> @@ -40,12 +40,14 @@
> [...]
> + }catch (Exception ex){
>
> Spacing here
> + OutputController.getLogger().log(ex);
> + }
> [...]
> + }catch (NonFileProtocolException ex){
>
> Same as above.
>
> + OutputController.getLogger().log(ex);
> + //default icon will be used later
> + }
> [...]
> -
> - String origLocation = location.substring("file:".length());
> + }else {
>
> Same as above.
>
> [...]
> + try{
> + URL favico = new URL(
...
> + //favicon 404 or similar
> + OutputController.getLogger().log(ex);
> + }
>
> Indentation is off.
>
> [...]
> - File f = PathsAndFiles.MENUS_DIR.getFile();
> + return findAndVerifyBasicDir(PathsAndFiles.MENUS_DIR.getFile(), " directroy for stroing menu entry cannot be created. You may expect failure");
>
> Same as above
>
> + if (stringEqualsOption(arg, OptionsDefinitions.OPTIONS.HTML)) {
> + throw new IllegalArgumentException("-html switch is not yet supported");
> + }
>
> Indentation for closing bracket
>
> [...]
> + @Test
> + public void stripClassNoClass() throws Exception {
>
> Indentation for @Test
All should be fixed.
>
>
>
> I like all the tests :D
tss :)
>
>
>
> So far I haven't found any bugs in manual user-testing of simple applets. I don't really expect this to work for complex things 100% of the time though.
>
> If I find anything new I'll let you know :D
ty!
>
>
>
> My one question is:
>
> What will happen when someone tries to do this and it doesn't work? Please make sure it is a good error message or some information, not just crashing.
Well - if generation fails, the icons will not be found, but the failure is mostly not fatal. So not
fatal exception is only logged, not showed.
I'm not sure if I wont to change it... Maybe yes... Well.. how? Some joptionpane? Where? hmhmh...
not sure baout this...
IF the run from shortcut fails, then it is properly reported.
ty for review!
(attached patch contains also fix for Jacob's
http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-December/030241.html)
>
>
> Great stuff! :D
>
>
>
> Regards,
>
> ----- Original Message -----
>>
>>
>> ----- Original Message -----
>>> not done - remembering of values from AccessWarningPane and -html switch
>>> (all
>>> will go as another
>>> changesets)
>>> not tested - create always setting (without asking user)
>>> in next iteration - localization of messages
>>>
>>> Ufghh.. this is tought one. It is adding possibility to create desktop/menu
>>> shortcut which leads to
>>> appelt directly, not via browser. To do so, many tweeks to previous patch
>>> were needed)
>>>
>>> long stroy short:
>>> set .config/icedtea-web/deployment.properties
>>> deployment.javaws.shortcut=ASK_USER
>>>
>>> and run some appelt in browser
>>>
>>> Get scared by new desktop shortcut dialogue (there is a lot of explaining
>>> tooltips!)
>>> And then prize be the light!
>>>
>>>
>>> disclaimer - although I have tested pretty hard, I doubt I have covered all
>>> cornercases of jnlp
>>> generation. Whatever will be found, I will try to fix now or in another
>>> chnagesets...
>>> Also note, that appelts using javascript or some long-into-bank applets
>>> will
>>> probably never work out
>>> of browser... But still, majority of applets *will* work. And those old
>>> simple applets are target
>>> audience of this patch.
>>>
>>> J.
>>>
>>> ps: I'm going to speak about this on defconf on start of february, and I'm
>>> on
>>> vacation 20.12-20.1 so
>>> if "anybody" (sorry Jie :) ) can look over it...I will really appreciate:(
>>> Sorry for late patch...
>>>
>>
>> Hello,
>>
>> I just wanted to let you know I've started reviewing and testing, however the
>> patch is quite large so it will take me some time.
>>
>>
>> Cheers,
>>
>>
>> --
>>
>> Jie Kang
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: appletsOutOfBrowserShortcuts-head-03.patch
Type: text/x-patch
Size: 84663 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141216/06604b69/appletsOutOfBrowserShortcuts-head-03-0001.patch>
More information about the distro-pkg-dev
mailing list