Reduce SVG observer usage by nsSVGPatternFrame*

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Scooter Morris, Assigned: Scooter Morris)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

13 years ago
After the approach outlined in Bug 302243.
(Assignee)

Comment 1

13 years ago
Created attachment 213270 [details] [diff] [review]
Work in progress
(Assignee)

Updated

13 years ago
Blocks: 328481
(Assignee)

Comment 2

13 years ago
Created attachment 213824 [details] [diff] [review]
Updated to trunk
Assignee: general → scootermorris
Attachment #213270 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #213824 - Flags: review?(tor)

Comment 3

13 years ago
Comment on attachment 213824 [details] [diff] [review]
Updated to trunk

Generally, many of the review comments of bug 302243 apply to this patch as well.  To pick out a few:

>   // Internal methods for handling referenced patterns
>-  PRBool checkURITarget(nsIAtom *);
>-  PRBool checkURITarget();
>+  PRPackedBool checkURITarget(nsIAtom *);
>+  PRPackedBool checkURITarget();

These return types shouldn't be changed.

>+NS_IMETHODIMP
>+nsSVGPatternFrame::AttributeChanged(PRInt32         aNameSpaceID,
>+                                    nsIAtom*        aAttribute,
>+                                    PRInt32         aModType)
>+{
>+  if (aNameSpaceID == kNameSpaceID_None &&
...
>+  } else if (aNameSpaceID == kNameSpaceID_XLink &&
>+             aAttribute == nsGkAtoms::href) {
>+    if (mNextPattern)
>+      mNextPattern->RemoveObserver(this);
>+    mNextPattern = nsnull;
>+    checkURITarget();
>+  }

Should this call to checkURITarget() be here?

>-PRBool
>+PRPackedBool
> nsSVGPatternFrame::checkURITarget(void) {
>   nsIFrame *nextPattern;
>   mLoopFlag = PR_TRUE; // Set our loop detection flag
>   // Have we already figured out the next Pattern?
>   if (mNextPattern != nsnull) {
>     return PR_TRUE;
>   }
> 
>+  nsAutoString nextPatternStr;
>+  mHref->GetAnimVal(nextPatternStr);
>   // Do we have URI?
>-  if (mNextPatternStr.Length() == 0) {
>+  if (nextPatternStr.Length() == 0) {

Use IsEmpty().
(Assignee)

Comment 4

13 years ago
Created attachment 214246 [details] [diff] [review]
Picked up changes from gradient review
Attachment #213824 - Attachment is obsolete: true
Attachment #214246 - Flags: review?(tor)
Attachment #213824 - Flags: review?(tor)
Comment on attachment 214246 [details] [diff] [review]
Picked up changes from gradient review

The |else| in |AttributeChanged| is bogus.

In some of the GetXXX functions you null out the out param at the top of the function body. It would be better done in the |if| that checks for failure IMO. Also only some of the functions do this, so please make the others (to which this applies) do it too. For those to which nulling out doesn't apply you can probably do something similar instead (like set out param floats to zero) to make sure failure behaviour is consistant and won't result in uninitialized variables. (A new bug to do the same for gradients would be good.)

s/patter to/pattern to/

Can you "fix" the |if (nsnull == it)|.
(Assignee)

Comment 6

13 years ago
Created attachment 214329 [details] [diff] [review]
Incorporate review comments
Attachment #214246 - Attachment is obsolete: true
Attachment #214329 - Flags: review?(jwatt)
Attachment #214246 - Flags: review?(tor)
(Assignee)

Comment 7

13 years ago
Created attachment 214478 [details] [diff] [review]
Added missing WillModify/DidModify
Attachment #214329 - Attachment is obsolete: true
Attachment #214478 - Flags: review?(jwatt)
Attachment #214329 - Flags: review?(jwatt)
Comment on attachment 214478 [details] [diff] [review]
Added missing WillModify/DidModify

Can you give nsSVGPatternFrame the same treatement as nsSVGStopFrame and reorder some of the function declarations? Move AttributeChanged up above GetType and move DidSetStyleContext (with nsIFrame comment) down to just above that? And can you add an nsIDebugFrame comment please?

You could also move the public: declaration up to where the top protected: declaration is to make the class declaration easier to scan.

Can you put a blank line between the two |if| blocks in AttributeChanged to make the fact that the first one returns stand out?

Please move the definition of AttributeChanged above the definition of GetType.

No point in having the |*kid = nsnull| since the very next line assigns it mFrames.FirstChild().

You're using nsSVGAtoms instead of nsGkAtoms in the new code you're adding.
The WillModify() call in the dtor should be passed mod_die too.
(Assignee)

Comment 10

13 years ago
Created attachment 214690 [details] [diff] [review]
Address review comments
Attachment #214478 - Attachment is obsolete: true
Attachment #214690 - Flags: review?(jwatt)
Attachment #214478 - Flags: review?(jwatt)
Comment on attachment 214690 [details] [diff] [review]
Address review comments

> NS_IMETHODIMP
> nsSVGPatternFrame::GetPatternUnits(PRUint16 *aUnits)
> {
>-  if (!mPatternUnits) {
>-    PrivateGetPatternUnits(getter_AddRefs(mPatternUnits));
>-    if (!mPatternUnits)
>-      return NS_ERROR_FAILURE;
>-    NS_ADD_SVGVALUE_OBSERVER(mPatternUnits);
>+  nsCOMPtr<nsIDOMSVGPatternElement> patternElement = do_QueryInterface(mContent);
>+  NS_ASSERTION(patternElement, "Wrong content element (not pattern)");
>+  if (!patternElement) {
>+    return NS_ERROR_FAILURE;
>   }

No need for this null check. The frame wouldn't even exist if the QI was going to fail since you did the check already in the constructor. If you stick it down in the |if| statement below we won't even QI if we need to use the referenced pattern.


>-  mPatternUnits->GetAnimVal(aUnits);
>+  // See if we need to get the value from another pattern
>+  if (!checkURITarget(nsGkAtoms::patternUnits)) {
>+    // No, return the values
>+    nsCOMPtr<nsIDOMSVGAnimatedEnumeration> units;
>+    patternElement->GetPatternUnits(getter_AddRefs(units));
>+    units->GetAnimVal(aUnits);
>+  } else {
>+    // Yes, get it from the target
>+    mNextPattern->GetPatternUnits(aUnits);
>+  }  
>+  mLoopFlag = PR_FALSE;
>   return NS_OK;
> }

The comment about the QI applies to many of the other methods too.

Other than that, this all looks good. r=me.

It would be nice to have a good patterns stress test to give to vlad for Tsvg so we can check what effect this has on performance.
Attachment #214690 - Flags: review?(jwatt) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 215429 [details] [diff] [review]
Address review comments
Attachment #214690 - Attachment is obsolete: true
Attachment #215429 - Flags: superreview?(roc)
Doesn't this need to include the checkURITarget changes that we did for gradients? I.e. a function to search for a frame that has a particular attribute, avoiding loops?
Those changes aren't done yet. I'm waiting on review. Once we've sorted out the gradient changes and I've addressed any review comments then I'll give patterns the same treatment.
(Assignee)

Comment 15

12 years ago
(In reply to comment #13)
> Doesn't this need to include the checkURITarget changes that we did for
> gradients? I.e. a function to search for a frame that has a particular
> attribute, avoiding loops?
> 

This patch is the first one of two (I guess if Jonathan is going to restructure it, three).  The next one (bug #328481) is a significant rework of some of the rendering logic that makes patterns much more robust.  I would prefer not making major revisions to the code until after that bug gets checked in (it has a dependency on this bug already).
Comment on attachment 215429 [details] [diff] [review]
Address review comments

I'm not excited about checking in code that we're going to replace almost immediately, but I guess we need to reduce the number of patches in flight.
Attachment #215429 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 17

12 years ago
Checked in on trunk:
Checking in layout/svg/base/src/nsSVGPatternFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPatternFrame.cpp,v  <--  nsSVGPatternFrame.cpp
new revision: 1.16; previous revision: 1.15
done
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.