[rfc][icedtea-web] bringing applets out of browser (part1)
Jie Kang
jkang at redhat.com
Wed Dec 17 16:05:44 UTC 2014
----- Original Message -----
> 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?
I meant the downloading part which I guess is what you wanted. Can you add to the comments of the function that if the file isn't in cache, it will block until downloaded and then return.
> >
> > 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?
I meant: Now that we have something that can handle complex returns, can't it also handle the old integer returns as well? Why do we need both AccessWarningPane and AccessWarningPaneComplexReturn?
Definitely not for this patch though.
> >
> >>
> >> [...]
> >>
> >> + 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.
Okay.
> >
> > 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>
s/ask more/ask for more
> >
> > 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 :)
Yeah. If you'd like you can add this in another patch later.
> >
> >
> >
> >>
> >> [...]
> >>
> >> + 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.
- Integer buttonIndex;
+ Object buttonIndex;
SecurityDialog dialog;
public SetValueHandler(SecurityDialog dialog, int buttonIndex) {
The change to Object is pretty useless if the single constructor for the class accepts an integer. You should change the 'int buttonIndex' to Object.
At the same time, looking at the code, there is only one use of the constructor:
protected ActionListener createSetValueListener(SecurityDialog dialog, int buttonIndex) {
return new SetValueHandler(dialog, buttonIndex);
}
This only accepts integers as well. So you'd need to change this to Object as well.
Otherwise in the end, it always accepts integers, and you should NOT change Integer buttonIndex to Object buttonIndex.
Looking at your changes, you just override this listener anyways.
+ //override the stupid createSetValueListener mechanism
+ run.addActionListener(new ActionListener() {
+
+ @Override
+ public void actionPerformed(ActionEvent e) {
+ parent.setValue(getModifier(0)); //according to createSetValueListener 0 is ok and 1 cancel
+ parent.dispose();
+ }
+ });
It seems you don't even want to use the SetValueHandler at all. Might as well remove it's use where you don't need it.
Reading over your changeset, I am nearly 100% sure this change isn't needed.
Having Objects will eventually require users to expect Objects as parameters and here's a discussion-thread on why Objects should be avoided as parameters: http://programmers.stackexchange.com/questions/198085/is-it-poor-programming-practice-to-pass-parameters-as-objects.
Fixing at this point would probably require a lot of work, so up to you.
> >>
> >> 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.
> >
It's fine if there are logs that report failure of generation. It would be great if the user could know whether or nut it was successfully created without checking logs. Just throw up a message dialog that tells them whether or not it worked. Allowing users to attempt to create shortcuts, and not telling them if it worked or not seems really poor.
AS well, a few tiny nits in-line below:
}
+
+
+ public String createJnlpVendorValue() {
I think there is an extra line here
+ //override the stupid createSetValueListener mechanism
+ run.addActionListener(new ActionListener() {
I'd remove the word 'stupid', it seems very unprofessional
+
+ private void setTolltips() {
+ browser.setToolTipText(R("EXAWbrowserTolltip"));
s/Toll/Tool
+ //note this time is in ESST timezone, and so is expecting the strring ouutput below
s/ESST/EST/ s/strring/string/ s/ouutput/output/
Regards,
> > 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
> >>>
> >>
> >
>
>
--
Jie Kang
More information about the distro-pkg-dev
mailing list