changes to markerWidth/markerHeight and orient not handled properly

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

Assignee

Description

14 years ago
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.
Assignee

Comment 1

14 years ago
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #210577 - Flags: review?(tor)
Assignee

Comment 3

14 years ago
Posted patch attach working version (obsolete) — Splinter Review
Attachment #210577 - Attachment is obsolete: true
Attachment #210582 - Flags: review?(tor)
Attachment #210577 - Flags: review?(tor)
Assignee

Comment 4

14 years ago
Posted patch add missing optimisation (obsolete) — Splinter Review
Attachment #210582 - Attachment is obsolete: true
Attachment #210612 - Flags: review?(tor)
Attachment #210582 - Flags: review?(tor)
Assignee

Comment 5

14 years ago
Comment on attachment 210612 [details] [diff] [review]
add missing optimisation

optimisation breaks things; currently investigating.
Attachment #210612 - Flags: review?(tor)
Assignee

Comment 6

14 years ago
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.
Attachment #210612 - Attachment is obsolete: true
Attachment #211007 - Flags: review?(tor)
Assignee

Comment 7

14 years ago
Comment on attachment 211007 [details] [diff] [review]
make it actually work with optimisation

need to wait for bug 327437 to be checked in now
Attachment #211007 - Flags: review?(tor)
Assignee

Updated

14 years ago
Depends on: 327437
Assignee

Updated

14 years ago
Attachment #211007 - Attachment is obsolete: true
Assignee

Comment 8

14 years ago
Attachment #212734 - Flags: review?(tor)

Comment 9

14 years ago
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.
Assignee

Comment 10

14 years ago
Posted patch address review comments (obsolete) — Splinter Review
Attachment #212734 - Attachment is obsolete: true
Attachment #212869 - Flags: review?(tor)
Attachment #212734 - Flags: review?(tor)

Comment 11

14 years ago
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.
Assignee

Comment 12

14 years ago
Attachment #212869 - Attachment is obsolete: true
Attachment #212920 - Flags: review?(tor)
Attachment #212869 - Flags: review?(tor)

Comment 13

14 years ago
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.
Attachment #212920 - Flags: review?(tor) → review+
Assignee

Comment 14

14 years ago
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

14 years ago
Ok, please file a bug about removing those stub methods.
Assignee

Comment 16

14 years ago
bug 328433 raised for the stub destructor issue.
Assignee

Updated

14 years ago
Attachment #212920 - Flags: superreview?(roc)
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
Attachment #212920 - Flags: superreview?(roc) → superreview+

Comment 19

14 years ago
Checked in for Robert.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.