Last Comment Bug 325728 - changes to markerWidth/markerHeight and orient not handled properly
: changes to markerWidth/markerHeight and orient not handled properly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Longson
: Hixie (not reading bugmail)
Mentors:
Depends on: 327437
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-03 03:40 PST by Robert Longson
Modified: 2006-02-24 08:41 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
track changes to markerWidth/markerHeight and fix orient (15.67 KB, patch)
2006-02-03 03:41 PST, Robert Longson
no flags Details | Diff | Splinter Review
testcase demonstrating orient issue - click on line (1.03 KB, image/svg+xml)
2006-02-03 04:11 PST, Robert Longson
no flags Details
attach working version (15.49 KB, patch)
2006-02-03 04:31 PST, Robert Longson
no flags Details | Diff | Splinter Review
add missing optimisation (15.63 KB, patch)
2006-02-03 09:30 PST, Robert Longson
no flags Details | Diff | Splinter Review
make it actually work with optimisation (18.25 KB, patch)
2006-02-07 07:04 PST, Robert Longson
no flags Details | Diff | Splinter Review
update for bug 327437 and with memory reduction (12.75 KB, patch)
2006-02-22 04:17 PST, Robert Longson
no flags Details | Diff | Splinter Review
address review comments (13.12 KB, patch)
2006-02-23 02:45 PST, Robert Longson
no flags Details | Diff | Splinter Review
address additional review comments and remove misleading debug output (14.43 KB, patch)
2006-02-23 10:24 PST, Robert Longson
tor: review+
roc: superreview+
Details | Diff | Splinter Review
address superreview comments (14.39 KB, patch)
2006-02-24 02:59 PST, Robert Longson
no flags Details | Diff | Splinter Review

Description Robert Longson 2006-02-03 03:40:32 PST
See bug 320622 comment 10...

> And for that matter we should be checking for changes to markerWidth/Height in
> DidModifySVGObservable and calling SetCoordCtxRect there too. (And while on the
> subject of DidModifySVGObservable it should only be doing the sync mOrientAngle
> stuff when 'observable' is mOrientType. Note we aren't currently observing
> mMarkerWidth/Height, so you'd need to NS_ADD_SVGVALUE_OBSERVER() them in
> Init().)

Also orient cannot be changed to an angle. Since it is mapped to an enum,
setting to an angle it is rejected by the enum code and the change does not
take place.
Comment 1 Robert Longson 2006-02-03 03:41:54 PST
Created attachment 210577 [details] [diff] [review]
track changes to markerWidth/markerHeight and fix orient
Comment 2 Robert Longson 2006-02-03 04:11:50 PST
Created attachment 210580 [details]
testcase demonstrating orient issue - click on line
Comment 3 Robert Longson 2006-02-03 04:31:14 PST
Created attachment 210582 [details] [diff] [review]
attach working version
Comment 4 Robert Longson 2006-02-03 09:30:53 PST
Created attachment 210612 [details] [diff] [review]
add missing optimisation
Comment 5 Robert Longson 2006-02-07 05:21:18 PST
Comment on attachment 210612 [details] [diff] [review]
add missing optimisation

optimisation breaks things; currently investigating.
Comment 6 Robert Longson 2006-02-07 07:04:50 PST
Created attachment 211007 [details] [diff] [review]
make it actually work with optimisation

GetAttr does not work while the document is loading. Fortunately its possible to get the value directly. Also checked that nsSVGAngle and nsSVGOrient call WillModifySVGObservable and DidModifySVGObservable correctly.
Comment 7 Robert Longson 2006-02-17 04:20:52 PST
Comment on attachment 211007 [details] [diff] [review]
make it actually work with optimisation

need to wait for bug 327437 to be checked in now
Comment 8 Robert Longson 2006-02-22 04:17:19 PST
Created attachment 212734 [details] [diff] [review]
update for bug 327437 and with memory reduction
Comment 9 tor 2006-02-22 16:34:17 PST
Comment on attachment 212734 [details] [diff] [review]
update for bug 327437 and with memory reduction

Nice.

> nsSVGMarkerElement::~nsSVGMarkerElement()
> {
>+  if (mMarkerWidth) {
>+    NS_ADD_SVGVALUE_OBSERVER(mMarkerWidth);
>+  }
>+  if (mMarkerHeight) {
>+    NS_ADD_SVGVALUE_OBSERVER(mMarkerHeight);
>+  }

You probably meant NS_REMOVE... in those two cases.

> NS_IMETHODIMP
> nsSVGMarkerElement::DidModifySVGObservable (nsISVGValue* observable,
>                                             nsISVGValue::modificationType aModType)
> {
> #ifdef DEBUG
>-  printf("markerelement - viewport/viewbox/preserveAspectRatio have been changed\n");
>+  printf("markerelement - markerWidth/markerHeight/viewport/viewbox/preserveAspectRatio have been changed\n");
> #endif
> 
>   mViewBoxToViewportTransform = nsnull;
> 
>-  // need to sync mOrientAngle
>-  nsAutoString value;
>-  nsresult rv = GetAttribute(NS_LITERAL_STRING("orient"), value);
>-  if (NS_SUCCEEDED(rv)) {
>-    nsCOMPtr<nsISVGValue> target = do_QueryInterface(mOrientAngle);
>-    target->SetValueString(value);
>-  }
>+  // sync coordinate context with viewbox:
>+  nsIDOMSVGRect *viewbox;
>+  mViewBox->GetBaseVal(&viewbox);
>+  SetCoordCtxRect(viewbox);

Might want to add a test for observable being the viewBox, markerWidth, or MarkerHeight before doing the sync.
Comment 10 Robert Longson 2006-02-23 02:45:48 PST
Created attachment 212869 [details] [diff] [review]
address review comments
Comment 11 tor 2006-02-23 09:43:58 PST
Comment on attachment 212869 [details] [diff] [review]
address review comments

> nsSVGMarkerElement::~nsSVGMarkerElement()
> {
>+  if (mMarkerWidth) {
>+    NS_REMOVE_SVGVALUE_OBSERVER(mMarkerWidth);
>+  }
>+  if (mMarkerHeight) {
>+    NS_REMOVE_SVGVALUE_OBSERVER(mMarkerHeight);
>+  }
>   if (mPreserveAspectRatio) {
>     NS_REMOVE_SVGVALUE_OBSERVER(mPreserveAspectRatio);
>   }
>   if (mViewBox) {
>     NS_REMOVE_SVGVALUE_OBSERVER(mViewBox);
>   }
>-  if (mOrientType) {
>-    NS_REMOVE_SVGVALUE_OBSERVER(mOrientType);
>-  }
> }
> 
> nsresult
> nsSVGMarkerElement::Init()
> {
...
>   // add observers -------------------------- :
>+  NS_ADD_SVGVALUE_OBSERVER(mMarkerWidth);
>+  NS_ADD_SVGVALUE_OBSERVER(mMarkerHeight);
>   NS_ADD_SVGVALUE_OBSERVER(mViewBox);
>   NS_ADD_SVGVALUE_OBSERVER(mPreserveAspectRatio);
>-  NS_ADD_SVGVALUE_OBSERVER(mOrientType);
> 
>   return NS_OK;
> }

I forgot that mapped values are implicitly set as observers, so they don't need to be manually added and removed.
Comment 12 Robert Longson 2006-02-23 10:24:24 PST
Created attachment 212920 [details] [diff] [review]
address additional review comments and remove misleading debug output
Comment 13 tor 2006-02-23 10:36:18 PST
Comment on attachment 212920 [details] [diff] [review]
address additional review comments and remove misleading debug output

> nsSVGMarkerElement::~nsSVGMarkerElement()
> {
>-  if (mPreserveAspectRatio) {
>-    NS_REMOVE_SVGVALUE_OBSERVER(mPreserveAspectRatio);
>-  }
>-  if (mViewBox) {
>-    NS_REMOVE_SVGVALUE_OBSERVER(mViewBox);
>-  }
>-  if (mOrientType) {
>-    NS_REMOVE_SVGVALUE_OBSERVER(mOrientType);
>-  }
> }

If we're going to remove all the guts of the destructor, might as well remove the  method entirely.  With that change, r=tor.
Comment 14 Robert Longson 2006-02-23 10:49:21 PST
content has empty destructors in lots of files:
nsSVGCircleElement.cpp
nsSVGDefsElement.cpp
nsSVGTSpanElement.cpp
etc, etc.

I assume these could/should be fixed throughout but I think that's a separate issue. Can we go with this patch as-is and have a followup bug for the removal of empty destructors from non-inherited classes throughout content.
Comment 15 tor 2006-02-23 11:18:34 PST
Ok, please file a bug about removing those stub methods.
Comment 16 Robert Longson 2006-02-24 02:03:52 PST
bug 328433 raised for the stub destructor issue.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-02-24 02:43:53 PST
Comment on attachment 212920 [details] [diff] [review]
address additional review comments and remove misleading debug output

+  if (aValueAsString.Equals(NS_LITERAL_STRING("auto")))
...
+  if (value.Equals(NS_LITERAL_STRING("auto"))) {
...
+  if (!value.Equals(NS_LITERAL_STRING("auto")))

use EqualsLiteral
Comment 18 Robert Longson 2006-02-24 02:59:17 PST
Created attachment 213027 [details] [diff] [review]
address superreview comments
Comment 19 tor 2006-02-24 08:41:52 PST
Checked in for Robert.

Note You need to log in before you can comment on or make changes to this bug.