Hi Daniel,

Nice to see your contribution. ;-)

Without much knowledge on Batik's internals, I'm only reviewing the patch in terms of coding principles (at least for now).


>  import org.apache.batik.dom.svg.SVGGraphicsElement;
> -
>  import org.w3c.dom.Node;

This apparently redundant line separating (java from batik from w3c) includes is expected. See, for example, "sources/org/apache/batik/bridge/BaseScriptingEnvironment.java".


>      protected Node newNode() {
> -        return new BindableElement(null, null, namespaceURI, localName);
> +    	return new BindableElement();

Indenting issue, please use 4 spaces for consistency.


> -    /**
> +	/**

Indenting issue, please use 4 spaces instead of tab character.


By the way, patches can be acknowledged by adding the "PatchAvailable" keyword to the report (which I've done). Adding the word to the report's title is also commonly seen, and I believe using both won't hurt (although I can be wrong). ;-)


Finally, I'm not sure if the patch should include a modification to the "CHANGES" file or if this is to be addressed by whoever commits it (although I'd assume the latter).


Regards,
 Helder Magalhães

Created attachment 23322 [details]
Test case (SVG 1.2) - this raises the exception

Reduced test case which reproduces the issue. By removing the version property (version="1.2") the document is set into SVG 1.1 mode and the exception is avoided.

Created attachment 23323 [details]
Test case (SVG 1.1) - this does *not* raise the exception (only for comparison purposes)

Hi Helder,

thanks for your help and for providing the two test cases. As for your review of the patch, I was not aware of that and trusted my IDE all too much. :-) In the future, I will take care of that.

Should I update this patch based on your review or is that okay for now?

I have a question on the patch.

The main effect of the patch is to bypass the
baseclass call to 'initializeAllLiveAttributes'
the question I have is when does that get called?
If it isn't ever called then Animated attributes won't work.

On that point, Cameron - should the initialization of 
the live attributes be tied to the booting of the SVG
and CSS DOM?

Hi Thomas,

the initializeAllLiveAttributes method is still called by the deepCopyInto method of SVGOMElement. When the newNode() method of BindableElement used the parameterized constructor, the initializeAllLiveAttributes method would actually have been called twice, if that first call had not caused the NPE due to the owner document not being set yet.

(In reply to comment #5)
> On that point, Cameron - should the initialization of 
> the live attributes be tied to the booting of the SVG
> and CSS DOM?

That would be better, since you could then avoid constructing the various SVGLength, etc. objects which wouldn't be useful without the SVG DOM being initialised anyway.

Before animation was implemented, these objects were lazily created.  So it'd be good to go back to something like that.

(In reply to comment #6)
> the initializeAllLiveAttributes method is still called by the deepCopyInto
> method of SVGOMElement. When the newNode() method of BindableElement used the
> parameterized constructor, the initializeAllLiveAttributes method would
> actually have been called twice, if that first call had not caused the NPE due
> to the owner document not being set yet.

I only took a very quick look, and I think Daniel is correct.  The pattern of overloading those four methods to ensure some extra data is copied across to the cloned node is also used in SVGOMElement.

(In reply to comment #4)
> Should I update this patch based on your review or is that okay for now?

Yes, I believe updating the patch would be good. :-)


(In reply to comment #7)
> So it'd be good to go back to something like that.

This sounds like a good general improvement. Nevertheless, I'd vote for fixing this specific issue and creating a different issue for dealing with the improvement, as I suspect it may become somehow lengthy, in both time and code changes... :-|  Naturally, this comes from the assumption that the trunk should be as stable as possible for daily use, I may be wrong in assuming that. ;-)


Updating the platform to "All" as this was reproduced in Windows also (and, given that it is Batik-related, it should be reproducible in all supported platforms).

Created attachment 23327 [details]
Patch for the bug with correct code formatting.

I added a new version of the patch (attachment 23321 [details]). The only difference is in the code formatting, which was not quite correct in the original version.

I want to look at committing this patch.  Thomas, do you think it is large enough need a CLA to be submitted?

(In reply to comment #10)
> I want to look at committing this patch.  Thomas, do you think it is large
> enough need a CLA to be submitted?

  It's certainly on the borderline.  If it weren't for the fact that
the change is almost identical to the GenericElementNS code I'd probably
say yes.  Given the uncertainty I think it's best if we get a CLA.

  Daniel can you file a Contributors License Agreement?
     http://www.apache.org/licenses/

(In reply to comment #11)
 >   Daniel can you file a Contributors License Agreement?
>      http://www.apache.org/licenses/

Since I created this particular patch as part of my job, I need to clear this up with my employer. I'm afraid this might take a couple of days. Of course, if you consider this patch trivial enough that it's viable to forego a CLA, then please do it. Sorry for the delay. I was not aware that a CLA might be necessary for contributing this patch.

(In reply to comment #12)
> Since I created this particular patch as part of my job, I need to clear this
> up with my employer. I'm afraid this might take a couple of days.

A little more than a couple of days have passed: in fact, quite more than a couple of months! :-D

Daniel, please don't leave this unattended in your TODO stack forever: I already saw a couple of good patches rotting in the bug tracker due to lack of a CLA. :-|

Before reading the full Apache v2 license agreement, you may be interested in taking a quick look at a human-readable summary [1]. ;-)


[1] http://www.codeproject.com/info/Licenses.aspx#ctl00_MC_LR_ctl08_N

(In reply to comment #13)
 > Daniel, please don't leave this unattended in your TODO stack forever: I
> already saw a couple of good patches rotting in the bug tracker due to lack of
> a CLA. :-|

Helder, I am very sorry that this process takes such a long time, much longer than I myself had expected. In fact, I attended to this issue back in March already, but I had problems communicating this to those in charge of deciding about it.

However, I pushed this topic once more now and was told that I can expect feedback within a couple of weeks.

I hope you understand that I cannot sign the CLA until I have clearance from my employer.

(In reply to comment #14)
> Helder, I am very sorry that this process takes such a long time, much longer
> than I myself had expected. In fact, I attended to this issue back in March
> already, but I had problems communicating this to those in charge of deciding
> about it.

No problem, I have similar things happening on my side. Whenever it's required management intervention, things can take a while (and/or even get a bit complicated)... :-|


> However, I pushed this topic once more now and was told that I can expect
> feedback within a couple of weeks.

Great, thanks. :-)


> I hope you understand that I cannot sign the CLA until I have clearance from my
> employer.

Yes, of course that's reasonable: I was just trying to avoid (given the lack of an update) that this would be hanging forever. ;-)

(In reply to comment #15)
> > I hope you understand that I cannot sign the CLA until I have clearance from my
> > employer.
> 
> Yes, of course that's reasonable: I was just trying to avoid (given the lack of
> an update) that this would be hanging forever. ;-)

@Daniel:
Ping! Again trying to push this up in the stack, possibly taking advantage of employers returning from vacation with extra-energy to deal with these sort of things. ;-)

Setting this issue to block the 1.8 release (bug 50045). It's a relevant bug, there's a fix already available (and almost ready for integration) and SVG 1.2 documents are probably starting to rise as SVG 1.1 is getting solidly deployed everywhere.