DO NOT REPLY [Bug 46072] Implement Window and Location from 1.2T

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=46072





--- Comment #6 from Cameron McCormack <ca...@apache.org>  2008-11-05 19:16:55 PST ---
(From update of attachment 22795)
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
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.

>+        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.

>+++ 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.


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.


-- 
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: batik-dev-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: batik-dev-help@xmlgraphics.apache.org