329600 - Eliminate SVG observer usage in nsSVGStopFrame

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Eliminate SVG observer usage in nsSVGStopFrame.

Comment 1

[Scooter Morris]

Attachment: Remove observers

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 214453 [details] [diff] [review]
Remove observers

Might as well loose the protected: declaration from the class declaration, but there should be a public: declaration in there for many of those functions.

>   /**
>    * Get the "type" of the frame
>    *
>    * @see nsLayoutAtoms::svgStopFrame
>    */

This should say nsGkAtoms now.


> NS_INTERFACE_MAP_BEGIN(nsSVGStopFrame)
>-  NS_INTERFACE_MAP_ENTRY(nsISVGValueObserver)
>-  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
> NS_INTERFACE_MAP_END_INHERITING(nsSVGStopFrameBase)

I wonder if there's a prefered macro for when there are no interface map entries.


>+nsSVGStopFrame::AttributeChanged(PRInt32         aNameSpaceID,
>+                                 nsIAtom*        aAttribute,
>+                                 PRInt32         aModType)
>+{
>+  if (aNameSpaceID == kNameSpaceID_None &&
>+      aAttribute == nsGkAtoms::offset) {

Uh, what about the Begin/EndBatchUpdate thing?

Comment 3

[Scooter Morris]

Attachment: Address review comments

Comment 4

[Jonathan Watt [:jwatt]]

Can you put AttributeChanged above GetType, and DidSetStyleContext plus the nsIFrame comment above that? (That's the order they appear in in nsIFrame.h, and it puts the nsIFrame comment in the right place.) Also can you put an:

  // nsIFrameDebug interface:

comment above the definition of GetFrameName (after the ifdef).

You can also loose the "protected:" above the friend declaration.

Can you replace |mParent->QueryInterface| with |CallQueryInterface|, and can you change |value| to |svgParent| to give it a more helpful name.

Comment 5

[Scooter Morris]

Attachment: Address second round of comments

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 214566 [details] [diff] [review]
Address second round of comments

r=me. It is now a thing of beauty. :-)

Comment 7

[Jonathan Watt [:jwatt]]

Although when I suggested using CallQueryInterface I actually meant like this:

  if (NS_SUCCEEDED(CallQueryInterface(mParent, &svgParent))) {

Comment 8

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

Comment on attachment 214566 [details] [diff] [review]
Address second round of comments

This is OK, but the usage of Begin/EndBatchUpdate is hackish. nsSVGGradientFrame really needs a method to notify it that the stop has changed. We could make nsSVGGradientFrame public in its own .h file, and give it an IID so you can QI to it and call the method. Please file a followup bug on this.

Comment 9

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

Comment on attachment 214566 [details] [diff] [review]
Address second round of comments

This is OK, but the usage of Begin/EndBatchUpdate is hackish. nsSVGGradientFrame really needs a method to notify it that the stop has changed. We could make nsSVGGradientFrame public in its own .h file, and give it an IID so you can QI to it and call the method. Please file a followup bug on this.

Comment 10

[Scooter Morris]

Checking in layout/svg/base/src/nsSVGStopFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGStopFrame.cpp,v  <--  nsSVGStopFrame.cpp
new revision: 1.9; previous revision: 1.8
done

checked in on trunk
r=jwatt/sr=roc

Comment 11

[Scooter Morris]

Attachment: Patch checked in