https://issues.apache.org/bugzilla/show_bug.cgi?id=46072
--- Comment #7 from G. Wade Johnson <[EMAIL PROTECTED]> 2008-11-06 04:58:04
PST ---
(In reply to comment #6)
Thanks Cameron,
I'll modify the code and update the patch this evening.
> (From update of attachment 22795 [details])
> Hi G. Wade.
>
> Thanks for working on the patch. I have a few comments. (Some are just
> style/formatting, and although there isn't a documented coding style, there's
> a
> de facto one that most of the code adheres to.)
>
> >Index: sources/org/apache/batik/swing/svg/JSVGComponent.java
> ...
> >- }
> >+ }
>
> Looks like an accidental space on the end of the line, which doesn't need to
> be
> there.
>
> >Index: sources/org/apache/batik/bridge/Location.java
> ...
> >+ ((UserAgent)bridgeContext.getUserAgent()).loadDocument( url );
>
> No spaces around "url".
>
> >+ public void reload() {
> >+ URL url = ((SVGOMDocument) bridgeContext.getDocument())
> >+ .getURLObject();
> >+ ((UserAgent)bridgeContext.getUserAgent()).loadDocument(
> >url.toString() );
>
> You can use getDocumentURI() on AbstractDocument to get the the current
> document's URI more directly than getting the URL object and converting it to
> a
> string. Note that you'll need to cast the returned Document to
Seems like I tried that and it didn't work. But, I may be misremembering.
> AbstractDocument so that the code will compile properly on JDKs that don't
> have
> the DOM Level 3 Core interfaces (since getDocumentURI() is a DOM 3 method).
>
> Unnecessary spaces around the loadDocument() argument here, too.
>
> >+ }
> >+
> >+ /**
> >+ * Returns the URL of this location as a String.
> >+ */
> >+ public String toString() {
> >+ URL url = ((SVGOMDocument) bridgeContext.getDocument())
> >+ .getURLObject();
> >+ return url.toString();
>
> Same as above; use getDocumentURI() instead.
>
> >Index: sources/org/apache/batik/bridge/ScriptingEnvironment.java
> ...
> > public Window(Interpreter interp, String lang) {
> > interpreter = interp;
> > language = lang;
> >+ location = null;
>
> This line is unnecessary, since location will be initialised to null
> automatically.
Habit. I tend to be explicit about these things. But, I'll change it.
> >+ public Location getLocation() {
> >+ if( location == null ) {
>
> Write this as 'if (location == null) {'.
>
> >+ location = new Location( bridgeContext );
>
> Remove the spaces.
>
> >Index: sources/org/apache/batik/script/rhino/WindowWrapper.java
> ...
> >+ this.defineProperty("location", WindowWrapper.class,
> >+ ScriptableObject.PERMANENT );
>
> Errant space before the ')'.
>
> > /**
> >+ * Return the Location for this Window.
> >+ */
> >+ public LocationWrapper getLocation() {
> >+ return new LocationWrapper( window.getLocation() );
> >+ }
>
> I don't think a wrapper class for the Location object is needed. Wrapper
> classes are useful if you need to give the JS object some special behaviour
> that you can't get from the automatic reflection of Java objects to their JS
> counterparts. Since the Location object is just a couple of simple methods,
> this shouldn't be needed, so you can make this method just return the Location
> object directly.
I know I tried that and could not get it to work. Rhino kept reporting that it
couldn't find the methods. I'll try it again, maybe I did something stupid.
> >+++ sources/org/apache/batik/script/rhino/LocationWrapper.java
> >(revision 0 ( https://svn.apache.org/viewcvs.cgi?view=rev&rev=0 ))
>
> This class then wouldn't be needed.
>
> >Index: sources/org/apache/batik/script/Window.java
> ...
> >+import org.w3c.dom.Location;
>
> This import isn't used.
Okay.
> With these addressed I'd be happy to check in the patch. I see on
> http://people.apache.org/~jim/committers.html that you don't have a
> Contributor
> License Agreemnt on file (see http://www.apache.org/licenses/#clas). You'll
> need to submit one of those before I can commit the patch.
I'll take care of it tonight.
Thanks.
G. Wade
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
