301628 - Remove SVG observer usage from layout/svg

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Start of the SVG code cleanup.

Comment 1

[tor]

Attachment: remove most usage of svg observers from layout/svg

This patch removes most of the SVG observers from layout/svg, leaving them for
graphic elements that need to keep tabs on gradients, some internal state for
outer SVG, and for tracking the attributes of gradients (which needs to deal
with the gradient attribute cascade).

Comment 2

[tor]

This could use testing to make sure I didn't break any scripted dynamic SVG.

Comment 3

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

That looks really nice :-)

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 190052 [details] [diff] [review]
remove most usage of svg observers from layout/svg

>-  return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValue, attrValue,
>-                          modification, hasListeners, aNotify);
>+  if (!svg_value)
>+    return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValue, attrValue,
>+                            modification, hasListeners, aNotify);
>+
>+  return NS_OK;
> }

I'll let sicking comment on this one.


> NS_IMETHODIMP
>-nsSVGCircleFrame::DidModifySVGObservable(nsISVGValue* observable,
>-                                         nsISVGValue::modificationType aModType)
>-{
>-  nsCOMPtr<nsIDOMSVGLength> l = do_QueryInterface(observable);
>-  if (l && (mCx==l || mCy==l || mR==l)) {
>+nsSVGCircleFrame::AttributeChanged(nsIContent*     aChild,
>+                                   PRInt32         aNameSpaceID,
>+                                   nsIAtom*        aAttribute,
>+                                   PRInt32         aModType)
>+{
>+  if (aAttribute == nsSVGAtoms::cx ||
>+      aAttribute == nsSVGAtoms::cy ||
>+      aAttribute == nsSVGAtoms::r)
>     UpdateGraphic(nsISVGPathGeometrySource::UPDATEMASK_PATH);
>-    return NS_OK;
>-  }
>-  // else
>-  return nsSVGPathGeometryFrame::DidModifySVGObservable(observable, aModType);
>-}
>+  else
>+    nsSVGPathGeometryFrame::AttributeChanged(aChild, aNameSpaceID,
>+                                             aAttribute, aModType);
> 
>+  return NS_OK;
>+}

How about returning the rv from nsSVGPathGeometryFrame::AttributeChanged?
Same for:

nsSVGEllipseFrame
nsSVGImageFrame
nsSVGLineFrame
nsSVGMarkerFrame
nsSVGPathFrame
nsSVGPolygonFrame
nsSVGPolylineFrame
nsSVGRectFrame


> nsSVGDefsFrame::~nsSVGDefsFrame()
> {
>-  nsCOMPtr<nsIDOMSVGTransformable> transformable = do_QueryInterface(mContent);
>-  if (!transformable)
>-    return;
>-
>-  nsCOMPtr<nsIDOMSVGAnimatedTransformList> transforms;
>-  transformable->GetTransform(getter_AddRefs(transforms));
>-  nsCOMPtr<nsISVGValue> value = do_QueryInterface(transforms);
>-  NS_ASSERTION(value, "interface not found");
>-  if (value)
>-    value->RemoveObserver(this);
> }

Why not remove the dtor entirely? it's gone from nsSVGMarkerFrame.
And also what about removing the nsISVGValueObserver methods from this class?


>Index: layout/svg/base/src/nsSVGForeignObjectFrame.cpp

You haven't removed the WillModifySVGObservable and DidModifySVGObservable
declarations from this file.
Can also stop inheriting nsISVGValueObserver and nsSupportsWeakReference
and remove the NS_INTERFACE_MAP_ENTRYs for them.


>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v
>retrieving revision 1.30
>diff -u -8 -p -r1.30 nsSVGGlyphFrame.cpp
>--- layout/svg/base/src/nsSVGGlyphFrame.cpp	1 Jul 2005 06:11:00 -0000	1.30
>+++ layout/svg/base/src/nsSVGGlyphFrame.cpp	21 Jul 2005 22:06:36 -0000
>@@ -228,20 +228,20 @@ nsSVGGlyphFrame::nsSVGGlyphFrame()
>     : mGeometryUpdateFlags(0), mMetricsUpdateFlags(0),
>       mCharOffset(0), mFragmentTreeDirty(PR_FALSE)
> {
> }
> 
> nsSVGGlyphFrame::~nsSVGGlyphFrame()
> {
>   if (mFillGradient) {
>-    NS_ADD_SVGVALUE_OBSERVER(mFillGradient);
>+    NS_REMOVE_SVGVALUE_OBSERVER(mFillGradient);
>   }
>   if (mStrokeGradient) {
>-    NS_ADD_SVGVALUE_OBSERVER(mStrokeGradient);
>+    NS_REMOVE_SVGVALUE_OBSERVER(mStrokeGradient);
>   }
> }

Oops! Nice catch.


>+nsSVGRectFrame::AttributeChanged(nsIContent*     aChild,
>+                                 PRInt32         aNameSpaceID,
>+                                 nsIAtom*        aAttribute,
>+                                 PRInt32         aModType)
>+{
>+  if (aAttribute == nsSVGAtoms::x ||
>+      aAttribute == nsSVGAtoms::y ||
>+      aAttribute == nsSVGAtoms::width ||
>+      aAttribute == nsSVGAtoms::height ||
>+      aAttribute == nsSVGAtoms::rx ||
>+      aAttribute == nsSVGAtoms::ry)
>+    UpdateGraphic(nsISVGPathGeometrySource::UPDATEMASK_PATH);
>+  else
>+    nsSVGPathGeometryFrame::AttributeChanged(aChild, aNameSpaceID,
>+                                             aAttribute, aModType);
>+
>+  return NS_OK;
> }

Can you return from nsSVGPathGeometryFrame::AttributeChanged
and can you put this after InitSVG please?


> class nsSVGTSpanFrame : public nsSVGTSpanFrameBase,
>                         public nsISVGTextContainerFrame,
>                         public nsISVGGlyphFragmentNode,
>                         public nsISVGChildFrame,
>-                        public nsISVGContainerFrame,
>-                        public nsISVGValueObserver,
>-                        public nsSupportsWeakReference
>+                        public nsISVGContainerFrame
> {
>   friend nsresult
>   NS_NewSVGTSpanFrame(nsIPresShell* aPresShell, nsIContent* aContent,
>                       nsIFrame* parentFrame, nsIFrame** aNewFrame);
> protected:
>   nsSVGTSpanFrame();
>   virtual ~nsSVGTSpanFrame();

Can you remove the dtor declaration?


> //----------------------------------------------------------------------
>-// nsISVGValueObserver methods:
>+// nsIFrame methods:
> 
> NS_IMETHODIMP
>-nsSVGTSpanFrame::WillModifySVGObservable(nsISVGValue* observable,
>-                                         nsISVGValue::modificationType aModType)
>-{
>-  return NS_OK;
>-}

There is actually already a
  // nsIFrame methods:
comment above this.

Comment 5

[tor]

Attachment: apply jwatt's comments, miscellaneous cleanup

Comment 6

[tor]

Attachment: different nsSVGElement::SetAttr change based on conversation with sicking

Comment 7

[tor]

Attachment: comment, namespace checking

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 190191 [details] [diff] [review]
comment, namespace checking

r=me with the following addressed.


>+nsSVGCircleFrame::AttributeChanged(nsIContent*     aChild,
>+                                   PRInt32         aNameSpaceID,
>+                                   nsIAtom*        aAttribute,
>+                                   PRInt32         aModType)
>+{
>+  if (aNameSpaceID == kNameSpaceID_None &&
>+      (aAttribute == nsSVGAtoms::cx ||
>+       aAttribute == nsSVGAtoms::cy ||
>+       aAttribute == nsSVGAtoms::r))
>     UpdateGraphic(nsISVGPathGeometrySource::UPDATEMASK_PATH);
>-    return NS_OK;
>-  }
>-  // else
>-  return nsSVGPathGeometryFrame::DidModifySVGObservable(observable, aModType);
>-}
>+  else
>+    return nsSVGPathGeometryFrame::AttributeChanged(aChild, aNameSpaceID,
>+                                                    aAttribute, aModType);
> 
>+  return NS_OK;
>+}

I prefer the following structure since I think it's a lot clearer:

nsSVGCircleFrame::AttributeChanged(nsIContent*	   aChild,
				   PRInt32	   aNameSpaceID,
				   nsIAtom*	   aAttribute,
				   PRInt32	   aModType)
{
  if (aNameSpaceID == kNameSpaceID_None &&
      (aAttribute == nsSVGAtoms::cx ||
       aAttribute == nsSVGAtoms::cy ||
       aAttribute == nsSVGAtoms::r)) {
    UpdateGraphic(nsISVGPathGeometrySource::UPDATEMASK_PATH);
    return NS_OK;
  }

  return nsSVGPathGeometryFrame::AttributeChanged(aChild, aNameSpaceID,
						  aAttribute, aModType);
}

but I suppose I'm picking nits. It'd be nice if you could structure all the
AttributeChanged functions your adding in this way though. I.e. in the files:

nsSVGEllipseFrame
nsSVGImageFrame
nsSVGLineFrame
nsSVGMarkerFrame
nsSVGPathFrame
nsSVGPolygonFrame
nsSVGPolylineFrame
nsSVGRectFrame
nsSVGUseFrame


> nsIAtom *
> nsSVGDefsFrame::GetType() const
> {
>   return nsLayoutAtoms::svgDefsFrame;
> }
> 
> //----------------------------------------------------------------------
>-// nsISVGValueObserver methods:
>+// nsIFrame methods:

You can remove this comment as the above functions are all nsIFrame methods,
and the same comment exists above them.

Can you add a comment to nsSVGDefsFrame::InitSVG noting that it isn't called by
it's specialisations (at least nsSVGMarkerFrame) since it does nothing. Just in
case something gets added to it at some point.


>+  NS_IMETHOD  AttributeChanged(nsIContent*     aChild,
>+                               PRInt32         aNameSpaceID,
>+                               nsIAtom*        aAttribute,
>+                               PRInt32         aModType);
>+

I just noticed you have two spaces between the "NS_IMETHOD" and the
"AttributeChanged". If you're bored waiting for your other reviews you could
always make it one and remove a space from before each of the other lines to
make the other args line up. ;-)


>-NS_IMETHODIMP
>-nsSVGInnerSVGFrame::AttributeChanged(nsIContent*    aChild,
>-                                     PRInt32        aNameSpaceID,
>-                                     nsIAtom*       aAttribute,
>-                                     PRInt32        aModType)
>-{
>-#ifdef DEBUG
>-    nsAutoString str;
>-    aAttribute->ToString(str);
>-    printf("** nsSVGInnerSVGFrame::AttributeChanged(%s)\n",
>-           NS_LossyConvertUCS2toASCII(str).get());
>-#endif
>-
>-  return NS_OK;
>-}
>-
> nsIAtom *
> nsSVGInnerSVGFrame::GetType() const
> {
>   return nsLayoutAtoms::svgInnerSVGFrame;
> }
> 
> //----------------------------------------------------------------------
> // nsISVGChildFrame methods
>@@ -686,26 +649,8 @@ nsSVGInnerSVGFrame::GetCoordContextProvi
>   nsSVGCoordCtxProvider *provider;
>   mContent->QueryInterface(NS_GET_IID(nsSVGCoordCtxProvider), (void**)&provider);
> 
>   NS_IF_ADDREF(provider);
> 
>   return provider;
> }
> 
>-//----------------------------------------------------------------------
>-// nsISVGValueObserver methods:
>-
>-NS_IMETHODIMP
>-nsSVGInnerSVGFrame::WillModifySVGObservable(nsISVGValue* observable,
>-                                            nsISVGValue::modificationType aModType)
>-{
>-  return NS_OK;
>-}
>-	
>-NS_IMETHODIMP
>-nsSVGInnerSVGFrame::DidModifySVGObservable (nsISVGValue* observable,
>-                                            nsISVGValue::modificationType aModType)
>-{
>-  NotifyViewportChange();
>-  
>-  return NS_OK;
>-}

Shouldn't AttributeChanged be kept and call NotifyViewportChange?


> nsIAtom *
> nsSVGLineFrame::GetType() const
> {
>   return nsLayoutAtoms::svgLineFrame;
> }
> 
> //----------------------------------------------------------------------
>-// nsISVGValueObserver methods:
>+// nsIFrame methods:
> 
> NS_IMETHODIMP
>-nsSVGLineFrame::DidModifySVGObservable(nsISVGValue* observable,
>-                                       nsISVGValue::modificationType aModType)

Can you move the comment above GetType? It's an nsIFrame methods too.


>@@ -416,23 +409,22 @@ nsresult nsSVGOuterSVGFrame::Init()
>   float mmPerPx = GetTwipsPerPx() / TWIPS_PER_POINT_FLOAT / (72.0f * 0.03937f);
>   SetCoordCtxMMPerPx(mmPerPx, mmPerPx);
>   
>   nsCOMPtr<nsISVGSVGElement> SVGElement = do_QueryInterface(mContent);
>   NS_ASSERTION(SVGElement, "wrong content element");
>   SVGElement->SetParentCoordCtxProvider(this);
> 
>   SVGElement->GetZoomAndPanEnum(getter_AddRefs(mZoomAndPan));
>-  NS_ADD_SVGVALUE_OBSERVER(mZoomAndPan);
>+
>   SVGElement->GetCurrentTranslate(getter_AddRefs(mCurrentTranslate));
>   NS_ADD_SVGVALUE_OBSERVER(mCurrentTranslate);
>   SVGElement->GetCurrentScaleNumber(getter_AddRefs(mCurrentScale));
>   NS_ADD_SVGVALUE_OBSERVER(mCurrentScale);
>   
>-  AddAsWidthHeightObserver();
>   SuspendRedraw();
>   return NS_OK;
> }

Can you add a comment noting why we don't observe mZoomAndPan, and width and
height?


> #ifdef DEBUG
>   NS_IMETHOD GetFrameName(nsAString& aResult) const
>   {
>     return MakeFrameName(NS_LITERAL_STRING("SVGPolyline"), aResult);
>   }
> #endif
> 
>-  // nsISVGValueObserver interface:
>-  NS_IMETHOD DidModifySVGObservable(nsISVGValue* observable,
>-                                    nsISVGValue::modificationType aModType);
>+  // nsIFrame interface:
>+  NS_IMETHOD  AttributeChanged(nsIContent*     aChild,
>+                               PRInt32         aNameSpaceID,
>+                               nsIAtom*        aAttribute,
>+                               PRInt32         aModType);

Can you move the "// nsIFrame interface:" above GetFrameName?


> nsIAtom *
> nsSVGPolylineFrame::GetType() const
> {
>   return nsLayoutAtoms::svgPolylineFrame;
> }
> 
> //----------------------------------------------------------------------
>-// nsISVGValueObserver methods:
>+// nsIFrame methods:
> 
> NS_IMETHODIMP
>-nsSVGPolylineFrame::DidModifySVGObservable(nsISVGValue* observable,
>-                                           nsISVGValue::modificationType aModType)
>+nsSVGPolylineFrame::AttributeChanged(nsIContent*     aChild,

Can you move the "// nsIFrame methods:" comment above GetType?

Comment 9

[tor]

> >-NS_IMETHODIMP
> >-nsSVGInnerSVGFrame::DidModifySVGObservable (nsISVGValue* observable,
> >-                                            nsISVGValue::modificationType
aModType)
> >-{
> >-  NotifyViewportChange();
> >-  
> >-  return NS_OK;
> >-}
> 
> Shouldn't AttributeChanged be kept and call NotifyViewportChange?

Not necessary, as the nsSVGSVGElement has observers (implicit through
AddMapped...) that call NotifyViewportChange when values are changed.

> >@@ -416,23 +409,22 @@ nsresult nsSVGOuterSVGFrame::Init()
> >   float mmPerPx = GetTwipsPerPx() / TWIPS_PER_POINT_FLOAT / (72.0f * 0.03937f);
> >   SetCoordCtxMMPerPx(mmPerPx, mmPerPx);
> >   
> >   nsCOMPtr<nsISVGSVGElement> SVGElement = do_QueryInterface(mContent);
> >   NS_ASSERTION(SVGElement, "wrong content element");
> >   SVGElement->SetParentCoordCtxProvider(this);
> > 
> >   SVGElement->GetZoomAndPanEnum(getter_AddRefs(mZoomAndPan));
> >-  NS_ADD_SVGVALUE_OBSERVER(mZoomAndPan);
> >+
> >   SVGElement->GetCurrentTranslate(getter_AddRefs(mCurrentTranslate));
> >   NS_ADD_SVGVALUE_OBSERVER(mCurrentTranslate);
> >   SVGElement->GetCurrentScaleNumber(getter_AddRefs(mCurrentScale));
> >   NS_ADD_SVGVALUE_OBSERVER(mCurrentScale);
> >   
> >-  AddAsWidthHeightObserver();
> >   SuspendRedraw();
> >   return NS_OK;
> > }
> 
> Can you add a comment noting why we don't observe mZoomAndPan, and width and
> height?

Hmm, in my upcoming patch I've removed the currentTranslate/Scale observers
because of the nsSVGSVGElement observer call to NotifyViewportChange.

Comment 10

[tor]

Attachment: address jwatt's comments

Comment 11

[tor]

Attachment: update to trunk

Update patch to trunk.	Doesn't address gradients, filters, or patterns.

Comment 12

[tor]

Attachment: update to trunk

gradients, filters, pattern, and mask left mainly alone and will be handled in a seperate pass as they have more complex behavior.

Comment 13

[Scooter Morris]

Comment on attachment 212792 [details] [diff] [review]
update to trunk

Throughout -- as Jonathan pointed out on IRC, probably should include
nsGkAtoms.h rather than nsSVGAtoms.h.

>Index: content/svg/content/src/nsSVGElement.cpp
>===================================================================

>@@ -527,16 +533,20 @@ nsSVGElement::DidModifySVGObservable(nsI
> {
>   // Return without setting DOM attributes as markup attributes if the
>   // attribute's element is being inserted into an SVG document fragment,
>   // which provides a context which percentage lengths are relative to.
>   // Bug 274886
>   if (aModType == nsISVGValue::mod_context)
>     return NS_OK;
> 
>+  // Return without setting DOM attribute 
>+  if (mSuppressNotification)
>+    return NS_OK;
>+

Could these two cases be combined?
  if (aModType == nsISVGValue::mod_context || mSuppressNotification)
    return NS_OK;
... with appropriate changes to the comment, obviously.


>Index: layout/svg/base/src/nsSVGGFrame.cpp
>===================================================================

Please provide a comment that WillModifySVGObservable is still needed until
filters no longer use the SVGObserver mechanism.

>@@ -331,21 +336,19 @@ nsSVGGFrame::WillModifySVGObservable(nsI
>   if (filter && mFilterRegion) {
>     nsISVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(this);
>     if (!outerSVGFrame)
>       return NS_ERROR_FAILURE;
> 
>     nsCOMPtr<nsISVGRendererRegion> region;
>     nsSVGUtils::FindFilterInvalidation(this, getter_AddRefs(region));
>     outerSVGFrame->InvalidateRegion(region, PR_TRUE);
>-
>-    return NS_OK;
>   }
> 

Probably need to make a note that TextPathFrame is still using SVGObserver
for mSegments.

>Index: layout/svg/base/src/nsSVGTextPathFrame.cpp
>===================================================================


>Index: layout/svg/base/src/nsSVGUseFrame.cpp
  
>+    for (nsIFrame* kid = mFrames.FirstChild(); kid;
>+         kid = kid->GetNextSibling()) {
>+      nsISVGChildFrame* SVGFrame=nsnull;
>+      kid->QueryInterface(NS_GET_IID(nsISVGChildFrame),(void**)&SVGFrame);
>+      if (SVGFrame)
>+        SVGFrame->NotifyCanvasTMChanged(PR_FALSE);
>+    }
>+    return NS_OK;
>+  }
>+
>+  return nsSVGGFrame::AttributeChanged(aNameSpaceID,
>+                                       aAttribute, aModType);

this should be nsSVGUseFrameBase::AttributeChanged, should't it?

>

With that r=me

Comment 14

[Jonathan Watt [:jwatt]]

> Could these two cases be combined?
>   if (aModType == nsISVGValue::mod_context || mSuppressNotification)
>     return NS_OK;
> ... with appropriate changes to the comment, obviously.

I would prefer them to be left as they are. The gain is negligible, and the reasons for returning are quite different. Plus the second check is only there until we get rid of the old observer mechanism, at which point we'd have to separate out the comments again.

Comment 15

[Jonathan Watt [:jwatt]]

You seem to be jumping the gun in removing the interface map entries for nsISVGValueObserver and nsSupportsWeakReference from nsSVGForeignObjectFrame.

Comment 16

[Jonathan Watt [:jwatt]]

Instead of just removing the declaration of AttributeChanged from nsInnerSVGFrame can you instead replace it with a comment along the lines of:

// We don't define an AttributeChanged method since changes to the 'x', 'y',
// 'width' and 'height' attributes of our content object are handled in
// nsSVGSVGElement::DidModifySVGObservable

Comment 17

[tor]

(In reply to comment #13)
> Throughout -- as Jonathan pointed out on IRC, probably should include
> nsGkAtoms.h rather than nsSVGAtoms.h.

Do we want to do that in a later global sweep, since all uses of the atoms need to be likewise changed?

> Please provide a comment that WillModifySVGObservable is still needed until
> filters no longer use the SVGObserver mechanism.
> 
> Probably need to make a note that TextPathFrame is still using SVGObserver
> for mSegments.

We'll probably need to use an observer scheme of some variety for things like filters and path segment lists.

Comment 18

[tor]

Attachment: update for review comments

Comment 19

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

Comment on attachment 213469 [details] [diff] [review]
update for review comments

 class nsSVGTextFrame : public nsSVGTextFrameBase,
                        public nsISVGTextFrame, // : nsISVGTextContainerFrame
                        public nsISVGChildFrame,
                        public nsISVGContainerFrame,
                        public nsISVGValueObserver,
                        public nsISVGTextContentMetrics,
                        public nsSupportsWeakReference

Did you mean to remove nsSupportsWeakReference/nsISVGValueObserver here?

Comment 20

[tor]

(In reply to comment #19)
> (From update of attachment 213469 [details] [diff] [review] [edit])
>  class nsSVGTextFrame : public nsSVGTextFrameBase,
>                         public nsISVGTextFrame, // : nsISVGTextContainerFrame
>                         public nsISVGChildFrame,
>                         public nsISVGContainerFrame,
>                         public nsISVGValueObserver,
>                         public nsISVGTextContentMetrics,
>                         public nsSupportsWeakReference
> 
> Did you mean to remove nsSupportsWeakReference/nsISVGValueObserver here?

No - <svg:text> can have a filter applied to it, and we currently use the observer mechanism to drive that.  Changes to filters/patterns/gradients/masks are going to be addressed after this.

Comment 21

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

I guess removing the rest of the observers can happen when we get around to reworking the way filter, fill and stroke relationship are managed.

Comment 22

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

(In reply to comment #20)
> No - <svg:text> can have a filter applied to it, and we currently use the
> observer mechanism to drive that.  Changes to filters/patterns/gradients/masks
> are going to be addressed after this.

Then probably you shouldn't be removing the #include files up above.

Comment 23

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

Comment on attachment 213469 [details] [diff] [review]
update for review comments

lovely!

Comment 24

[Jonathan Watt [:jwatt]]

(In reply to comment #17)
> (In reply to comment #13)
> > Throughout -- as Jonathan pointed out on IRC, probably should include
> > nsGkAtoms.h rather than nsSVGAtoms.h.
> 
> Do we want to do that in a later global sweep, since all uses of the atoms need
> to be likewise changed?

I'd rather we change them in the code as we touch it for the sake of the blame records. (We can also do a sweep of anything not touched later.)

Comment 25

[tor]

Attachment: updated patch

Add includes back for nsSVGTextFrame.

Switch to nsGkAtoms.

Fix textPath initialization which was broken by the previous patches.

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 213599 [details] [diff] [review]
updated patch

Looks good. r=me

Comment 27

[tor]

Checked in.

Comment 28

[Robert Longson [:longsonr]]

I think this has broken DOM update of marker elements. The browser window no longer refreshes when the update is actioned. Try the testcase of bug 325728 to see the problem. Refreshing the browser window (by obscuring or minimising it) corrects the display.