[rfc][icedtea-web] bringing applets out of browser (part1)
Jiri Vanek
jvanek at redhat.com
Wed Dec 17 14:46:26 UTC 2014
I found that previous versions broken one family of unittests. Fixed.
Aslo I have disbaled jnlp htref radio button, if jnlphref is not presented.
Also last nit of Jacob is fixed.
On 12/16/2014 04:32 PM, Jiri Vanek wrote:
> 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-04.patch
Type: text/x-patch
Size: 90952 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141217/9af22463/appletsOutOfBrowserShortcuts-head-04-0001.patch>
More information about the distro-pkg-dev
mailing list