329758 - changing markers via DOM does not cause a refresh

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Update a marker element via the DOM. The change takes place in the DOM but the browser window does not refresh to show the updated marker.

Bug 301628 changed the way updates work but misses out the new code to refresh the browser window when an update occurs.

Comment 1

[Robert Longson [:longsonr]]

Attachment: testcase - click on line

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[tor]

Comment on attachment 214419 [details] [diff] [review]
patch

That isn't the correct fix as far as I can see, and I'm not sure why it's working.  We really need to tell the refering geometry that the marker has changed and that it needs to invalidate/redraw.

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments

Hopefully this implements what was suggested by comment 3 if I understood it correctly.

Comment 5

[tor]

Marker destructor should send a mod_die to let dependent geometry know that the marker is going away.

Putting on the memory reduction hat, the markers seem like something that would be a good candidate for a frame property so we don't have keep the data around for non-marked frames (much more common).  Bug 330498 is an example of using properties if you're not familiar with them - I wasn't before writing that patch.

Comment 6

[Robert Longson [:longsonr]]

Attachment: address additional review comments

Comment 7

[tor]

Comment on attachment 215766 [details] [diff] [review]
address additional review comments

It seems that there might be a bit less code if the three marker frames were placed into a structure that's placed as a single property.

Unsetting the property is probably clearer if you use DeleteProperty instead of SetProperty with nsnull (which doesn't delete the key in the table, just sets the value to nsnull).

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comments

I have replaced the nsnull unset with DeleteProperty.
I have also reduced the patch size, reusing some common parts and updated it to the latest trunk.

I have not combined the frames into a single structure as that would cost 3 lots of memory even if only one was used. I'm not sure marker mid will be all that common compared to start/end. Also it would make the code more larger and more complicated as I would have to create and destroy the memory storing the combined marker pointers, wouldn't I?
If you still disagree I will try to rewrite it.

Finally while we're on the subject of properties... Should the other pointer member variables in nsSVGPathGeometry e.g. mFillGradient eventually become stored as properties?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with tor that it's probably better to put the marker data into one property. Sure, 'mid' may not be used very much, but the incremental cost of that is very low ... suppose 'mid' is never used, and 'start'/'end' are always used when markers are present. Then we waste one pointer-worth of data per pathgeometryframe *with markers*, which is small potatoes IMHO because now we're only talking about a small fraction of the pathgeometry frames.

+  if (markerStart) {
+    NS_REMOVE_SVGVALUE_OBSERVER(markerStart);
+  }
+  if (markerMid) {
+    NS_REMOVE_SVGVALUE_OBSERVER(markerMid);
+  }
+  if (markerEnd) {
+    NS_REMOVE_SVGVALUE_OBSERVER(markerEnd);
+  }

A tiny static helper function would simplify this (MaybeRemoveSVGValueObserver?).

+  if (markerStart) {
+    NS_REMOVE_SVGVALUE_OBSERVER(markerStart);
+    DeleteProperty(nsSVGAtoms::marker_start);
+  }
+  if (markerMid) {
+    NS_REMOVE_SVGVALUE_OBSERVER(markerMid);
+    DeleteProperty(nsSVGAtoms::marker_mid);
+  }
+  if (markerEnd) {
+    NS_REMOVE_SVGVALUE_OBSERVER(markerEnd);
+    DeleteProperty(nsSVGAtoms::marker_end);
+  }

Similarly.

+  if (!*markerEnd) {
+    aURI = GetStyleSVG()->mMarkerEnd;
+    if (aURI) {
+      if (NS_SUCCEEDED(NS_GetSVGMarkerFrame(markerEnd, aURI, mContent))) {
+        SetProperty(nsSVGAtoms::marker_end, *markerEnd);
+        NS_ADD_SVGVALUE_OBSERVER(*markerEnd);
+      }
+    }
+  }
+
+  if (!*markerMid) {
+    aURI = GetStyleSVG()->mMarkerMid;
+    if (aURI) {
+      if (NS_SUCCEEDED(NS_GetSVGMarkerFrame(markerMid, aURI, mContent))) {
+        SetProperty(nsSVGAtoms::marker_mid, *markerMid);
+        NS_ADD_SVGVALUE_OBSERVER(*markerMid);
+      }
+    }
+  }
   
+  if (!*markerStart) {
+    aURI = GetStyleSVG()->mMarkerStart;
+    if (aURI) {
+      if (NS_SUCCEEDED(NS_GetSVGMarkerFrame(markerStart, aURI, mContent))) {
+        SetProperty(nsSVGAtoms::marker_start, *markerStart);
+        NS_ADD_SVGVALUE_OBSERVER(*markerStart);
+      }
+    }
+  }

Again. Here you'll need to pass in nsIAtom* and the URI.

You should have a frame state bit that records whether the marker frames property (only one now) actually has been set, so you can avoid calling GetProperty when there are no markers.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> Finally while we're on the subject of properties... Should the other pointer
> member variables in nsSVGPathGeometry e.g. mFillGradient eventually become
> stored as properties?

Yes.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments

markers are now all in one property with a frame state bit.

I've not used helper functions for individual marker elements but the code for each is now much smaller as they all use one property and are therefore often treated as one thing.

Comment 12

[tor]

Comment on attachment 216056 [details] [diff] [review]
address review comments

Why did you put the marker helper functions in nsSVGUtils?  They're only ever going to be called from nsSVGPathGeometryFrame, so it makes sense to keep them there.

>@@ -459,16 +441,28 @@ nsSVGPathGeometryFrame::DidModifySVGObse
>+  if (marker) {
>+    if (aModType == nsISVGValue::mod_die) {
>+      DeleteProperty(nsGkAtoms::marker);
>+      RemoveStateBits(NS_STATE_SVG_HAS_MARKERS);
>+    }
>+    UpdateGraphic(nsISVGGeometrySource::UPDATEMASK_NOTHING);

This could be optimized by checking for the particular marker, and save dumping the whole property.  Probably not worth it, though.

>+void
>+nsSVGUtils::AddMarkerProperty(nsIFrame *aFrame)
...
>+
>+    if (markerStart || markerMid || markerEnd) {
>+      nsSVGMarkerProperty *property = new nsSVGMarkerProperty;

Check that the allocation succeeds.

>+      property->mMarkerStart = markerStart;
>+      property->mMarkerMid = markerMid;
>+      property->mMarkerEnd = markerEnd;

Comment 13

[Robert Longson [:longsonr]]

Attachment: address review comments

> Why did you put the marker helper functions in nsSVGUtils?  They're only ever
> going to be called from nsSVGPathGeometryFrame, so it makes sense to keep them
> there.

MarkerPropertyDtor cannot be a class member. It therefore can't use NS_REMOVE_SVGVALUE_OBSERVER. There is a useful helper function RemoveObserver in nsSVGUtils that I would have to either: 
a) repeat in nsSVGPathGeometryFrame.cpp 
b) make it (and probably AddObserver) into static class members available outside nsSVGUtils.

I chose to put the code in nsSVGUtils and leave RemoveObserver as it is. I could do b) instead if you wanted.

> This could be optimized by checking for the particular marker, and save dumping
> the whole property.  Probably not worth it, though.

Done. Further testing indicated that this was necessary as I had problems with shutdown with frames that had multiple markers with the same style. I now only have a single observer if a path geometry frame has multiple markers with the same style.

> Check that the allocation succeeds.

Done, also added check that filter allocation succeeds and that a presshell exists before calling functions that use it.

Comment 14

[tor]

(In reply to comment #13)
> MarkerPropertyDtor cannot be a class member. It therefore can't use
> NS_REMOVE_SVGVALUE_OBSERVER. There is a useful helper function RemoveObserver
> in nsSVGUtils that I would have to either: 
> a) repeat in nsSVGPathGeometryFrame.cpp 
> b) make it (and probably AddObserver) into static class members available
> outside nsSVGUtils.
> 
> I chose to put the code in nsSVGUtils and leave RemoveObserver as it is. I
> could do b) instead if you wanted.

I think option (b) would be the right thing to do.

> Done. Further testing indicated that this was necessary as I had problems with
> shutdown with frames that had multiple markers with the same style. I now only
> have a single observer if a path geometry frame has multiple markers with the
> same style.

While AddMarkerObserver does have that check, it doesn't prevent nsSVGUtils::AddMarkerProperty from adding multiple observer if a marker is used multiple times.  nsSVGUtils::AddMarkerProperty calls AddMarkerObserver on the marker frames before it fills in property fields, which means AddMarkerObservers is actually looking at uninitialized data.

Comment 15

[Robert Longson [:longsonr]]

Attachment: address review comments

actions in comment 14 have been implemented.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (property->mMarkerStart)
+    RemoveMarkerObserver(property, (nsIFrame *)aObject, property->mMarkerStart);

Move the if != null check into RemoveMarkerObserver.

+  if ((style->mMarkerStart || style->mMarkerMid || style->mMarkerEnd) &&
+      !(GetStateBits() & NS_STATE_SVG_HAS_MARKERS)) {

It's generally better style to write something like this as "if (...) return;" instead of nesting the whole function body, but I'll let this one slide...

+    if (style->mMarkerStart) {
+      NS_GetSVGMarkerFrame(&markerStart, style->mMarkerStart, content);
+      if (markerStart) {
+        AddMarkerObserver(property, this, markerStart);
+        property->mMarkerStart = markerStart;
+      }
+    }

This chunk of code and its clones can be moved into a single function, subsuming AddMarkerObserver, taking style->mMarkerStart and &property->mMarkerStart as parameters. Call it GetMarkerFromStyle or something.

+    if (markerStart || markerMid || markerEnd) {
+      SetProperty(nsGkAtoms::marker, property, MarkerPropertyDtor);
+      AddStateBits(NS_STATE_SVG_HAS_MARKERS);
+    } else {
+      delete property;
+    }

What happens if something changes in the document so the marker frame can be found?

I think what we need to do here is to create and keep the marker property whenever style says we have markers whether we find frames or not. Then in AddMarkerProperty (which should be renamed UpdateMarkerProperty, perhaps), if the property already exists, try to reresolve markers with a non-null style but a null frame in the property.

Comment 17

[Robert Longson [:longsonr]]

Attachment: address superreview comments

I addressed all the comments (except for the I'll let this one slide one)

Regarding the final sr comment:
> I think what we need to do here is to create and keep the marker property
> whenever style says we have markers whether we find frames or not. Then in
> AddMarkerProperty (which should be renamed UpdateMarkerProperty, perhaps), if
> the property already exists, try to reresolve markers with a non-null style > but
> a null frame in the property.

I implemented this as suggested. One consequence of this is that the property has to be deleted in the path geometry destructor if it exists. When the marker sends a mod_die the style still exists so you can't delete the property at that time.

Comment 18

[Robert Longson [:longsonr]]

For what its worth the latest patch is only different in nsSVGPathGeometryFrame.cpp and nsSVGPathGeometryFrame.h when compared to the previous one.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 217296 [details] [diff] [review]
address superreview comments

@@ -94,16 +100,20 @@ nsSVGPathGeometryFrame::~nsSVGPathGeomet
+    RemoveStateBits(NS_STATE_SVG_HAS_MARKERS);

No need to do this, the frame is being destroyed anyway.

+  nsSVGMarkerProperty *property = (nsSVGMarkerProperty *)aPropertyValue;
+  RemoveMarkerObserver(property, (nsIFrame *)aObject, property->mMarkerStart);
+  RemoveMarkerObserver(property, (nsIFrame *)aObject, property->mMarkerMid);
+  RemoveMarkerObserver(property, (nsIFrame *)aObject, property->mMarkerEnd);

NS_STATIC_CASTs, please. You probably want to hoist the nsIFrame* cast out.

 nsSVGPathGeometryFrame::GetMarkerFrames(nsISVGMarkerFrame **markerStart,
                                         nsISVGMarkerFrame **markerMid,
                                         nsISVGMarkerFrame **markerEnd)

This should really just return the property struct pointer or nsnull, but this can be done in a separate bug.

+        nsSVGUtils::AddObserver((nsIFrame *)this, marker);

NS_STATIC_CAST

+      property->mMarkerStart = property->mMarkerMid = property->mMarkerEnd = nsnull;

Slightly better style to set these in the nsSVGMarkerProperty constructor.

sr+ with those changes

Comment 20

[Robert Longson [:longsonr]]

Attachment: planned checkin

additional sr comments addressed, apart from the GetMarkerFrames change that was suggested as a separate bug.

Comment 21

[Robert Longson [:longsonr]]

Checked in.

Checking in nsSVGClipPathFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGClipPathFrame.cpp,v  <--  nsSVGClipPat
hFrame.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in nsSVGFilterFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGFilterFrame.cpp,v  <--  nsSVGFilterFra
me.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in nsSVGMarkerFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGMarkerFrame.cpp,v  <--  nsSVGMarkerFra
me.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in nsSVGMaskFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGMaskFrame.cpp,v  <--  nsSVGMaskFrame.c
pp
new revision: 1.6; previous revision: 1.5
done
Checking in nsSVGPathFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPathFrame.cpp,v  <--  nsSVGPathFrame.c
pp
new revision: 1.33; previous revision: 1.32
done
Checking in nsSVGPathGeometryFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp,v  <--  nsSVGPat
hGeometryFrame.cpp
new revision: 1.49; previous revision: 1.48
done
Checking in nsSVGPathGeometryFrame.h;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.h,v  <--  nsSVGPathG
eometryFrame.h
new revision: 1.27; previous revision: 1.26
done
Checking in nsSVGPolygonFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPolygonFrame.cpp,v  <--  nsSVGPolygonF
rame.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in nsSVGPolylineFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPolylineFrame.cpp,v  <--  nsSVGPolylin
eFrame.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in nsSVGUtils.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.cpp,v  <--  nsSVGUtils.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in nsSVGUtils.h;
/cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.h,v  <--  nsSVGUtils.h
new revision: 1.17; previous revision: 1.16
done