111496 - implement preserveAspectRatio

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[mop]

the viewBox is one of the main features of svg. Would be really nice if this
could be implemented.

Comment 1

[Alex Fritze]

viewBox is now implemented, but the preserveAspectRatio attribute (which goes
hand in hand with viewBox) isn't yet -> changing summary.

Comment 2

[Jonathan Watt [:jwatt]]

*** Bug 215891 has been marked as a duplicate of this bug. ***

Comment 3

[Jonathan Watt [:jwatt]]

I'd like preliminary reviews on the following four untested files from anyone
that has the time. I assume the header files can just be cvs added now, but it's
maybe best to wait until my XXX comments in the source files have been addressed
before adding them. (Or if you add them now I can change them in my upcomming
patch alex.) 

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.h

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.cpp

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.h

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.cpp

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch without changes to GetViewboxToViewportTransform

And here's what I think I subsequently have to do to the existing files, but
without the necessary changes to GetViewboxToViewportTransform. Again, nothing
tested yet, but feedback sought. 

I also have to add SVG_ATOM(preserveAspectRatio, "preserveAspectRatio") to
content/shared/public/nsSVGAtomList.h but I can't seem to get cvs to pick up
that change at the moment.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch without changes to GetViewboxToViewportTransform v2

After making a lot of changes I managed to build yesterday. Changes to the four
new files will be attached later.

Comment 10

[Jonathan Watt [:jwatt]]

The #include "nsIDOMSVGAnimatedPreserveAspectRatio.h" shouldn't be there. 

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.h

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.cpp

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.h

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.cpp

Comment 15

[Jonathan Watt [:jwatt]]

Alex, if the four files I attached look okay to you can you cvs add them. 

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: the rest of it

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.h

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.cpp

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.h

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.cpp

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: patch for existing code - tested win2k

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: TESTCASE

Comment 23

[Alex Fritze]

Comment on attachment 151778 [details]
nsSVGAnimatedPreserveAspectRatio.h

r=afri but you'll have to rename the file. The Mac has a 31 character limit on
file names.

> * Portions created by the Initial Developer are Copyright (C) 2001

Should probably be 2004

Comment 24

[Alex Fritze]

Comment on attachment 151779 [details]
nsSVGAnimatedPreserveAspectRatio.cpp

r=afri, but again needs to be renamed to <=31chars.
Also:
> * Portions created by the Initial Developer are Copyright (C) 2001
2004

>nsSVGAnimatedPreserveAspectRatio::~nsSVGAnimatedPreserveAspectRatio()
>{

>  if (!mBaseVal) return;

Can be removed. See below.

>void
>nsSVGAnimatedPreserveAspectRatio::Init(nsIDOMSVGPreserveAspectRatio* aBaseVal)
>{
>  mBaseVal = aBaseVal;

>  if (!mBaseVal) return;

Can be removed. See below.

>////////////////////////////////////////////////////////////////////////
>// Exported creation functions:
>
>nsresult
>NS_NewSVGAnimatedPreserveAspectRatio(nsIDOMSVGAnimatedPreserveAspectRatio** result,
>                                     nsIDOMSVGPreserveAspectRatio* baseVal)
>{
>  *result = nsnull;

You should add a test here to ensure baseVal!=null, because several of the
methods assume that mBaseVal is valid:
   if (!baseVal) {
     NS_ERROR("need baseVal");
     return NS_ERROR_FAILURE;
   }

Alternatively you could make nsSVGAnimatedPreserveAspaceRatio return a failure,
or code "if(!mBaseVal)return;" into all relevant methods.

Comment 25

[Alex Fritze]

Comment on attachment 151780 [details]
nsSVGPreserveAspectRatio.h

r=afri
> * Portions created by the Initial Developer are Copyright (C) 2001
2004

Comment 26

[Alex Fritze]

Comment on attachment 151781 [details]
nsSVGPreserveAspectRatio.cpp

r=afri with the following changes:

> * Portions created by the Initial Developer are Copyright (C) 2001
2004

>  virtual ~nsSVGPreserveAspectRatio();

Doesn't need to be virtual unless you inherit Release().

>NS_IMETHODIMP
>nsSVGPreserveAspectRatio::SetValueString(const nsAString& aValue)

>
>  if (token && !strcmp(token, "defer"))

Can you add an NS_ERROR("'defer' not implemented yet"); here please? At some
stage we will have to implement defer.

     [...]
>    else
>      mAlign = SVG_PRESERVEASPECTRATIO_UNKNOWN;

We should leave mAlign unmodified and return an error in this case.

The specs say:
 SVG_PRESERVEASPECTRATIO_UNKNOWN		The enumeration was set to a
value that is not one of predefined types. It is invalid to attempt to define a
new value of this type or to attempt to switch an existing value to this type.

My reading of this is: "You can never get a value
SVG_PRESERVEASPECTRATIO_UNKNOWN"

     [...]
>    else  // no value for meetOrSlice - give mMeetOrSlice the default value
>      mMeetOrSlice = SVG_MEETORSLICE_MEET;

As above.

>
>NS_IMETHODIMP
>nsSVGPreserveAspectRatio::GetValueString(nsAString& aValue)
>{
>  // XXX: defer isn't stored
>
>  switch (mAlign) {

>    case SVG_PRESERVEASPECTRATIO_UNKNOWN:
>      aValue.AssignLiteral("");
>      return NS_OK;  // return empty string

Since we can never validly get a SVG_PRESERVERASPECTRATIO_UNKNOWN, falling
through to default is ok for this case.

>      case SVG_MEETORSLICE_UNKNOWN:
>        break;  // do nothing

as above

>NS_IMETHODIMP nsSVGPreserveAspectRatio::SetAlign(PRUint16 aAlign)
>{
>  WillModify();
>  mAlign = aAlign > SVG_PRESERVEASPECTRATIO_XMAXYMAX?
>                    SVG_PRESERVEASPECTRATIO_UNKNOWN: aAlign;
>  DidModify();
>  return NS_OK;
>}
>

I think instead of silently allowing invalid values here you should return an
error and leave the value unmodified.

>NS_IMETHODIMP nsSVGPreserveAspectRatio::SetMeetOrSlice(PRUint16 aMeetOrSlice)
>{
>  WillModify();
>  mMeetOrSlice = aMeetOrSlice > SVG_MEETORSLICE_SLICE?
>                                SVG_MEETORSLICE_UNKNOWN: aMeetOrSlice;
>  DidModify();
>  return NS_OK;
>}

Again, I think we should return an error for invalid values.

Comment 27

[Alex Fritze]

Comment on attachment 151782 [details] [diff] [review]
patch for existing code - tested win2k

r=afri

>@@ -296,22 +302,34 @@ nsSVGSVGElement::Init()
>     
>     nsCOMPtr<nsIDOMSVGRect> wrapperRect;
>     rv = NS_NewSVGRectPrototypeWrapper(getter_AddRefs(wrapperRect), mViewport);
>     NS_ENSURE_SUCCESS(rv,rv);
>     rv = NS_NewSVGAnimatedRect(getter_AddRefs(mViewBox), wrapperRect);
>     NS_ENSURE_SUCCESS(rv,rv);
>     rv = AddMappedSVGValue(nsSVGAtoms::viewBox, mViewBox);
>     NS_ENSURE_SUCCESS(rv,rv);

>+
>+    nsCOMPtr<nsIDOMSVGPreserveAspectRatio> preserveAspectRatio;
>+    rv = NS_NewSVGPreserveAspectRatio(getter_AddRefs(preserveAspectRatio));
>+    NS_ENSURE_SUCCESS(rv,rv);
>+    rv = NS_NewSVGAnimatedPreserveAspectRatio(
>+                                          getter_AddRefs(mPreserveAspectRatio),
>+                                          preserveAspectRatio);
>+    NS_ENSURE_SUCCESS(rv,rv);
>+    rv = AddMappedSVGValue(nsSVGAtoms::preserveAspectRatio,
>+                                      mPreserveAspectRatio);
>+    NS_ENSURE_SUCCESS(rv,rv);
>   }

Can you please add a comment above these lines:
  // DOM property: preserveAspectRatio , #IMPLIED attrib: preserveAspectRatio

Comment 28

[Alex Fritze]

Comment on attachment 151782 [details] [diff] [review]
patch for existing code - tested win2k

sicking: Could you please ok the dom/ changes?

Comment 29

[Alex Fritze]

jwatt: tor says the filename limit only applies to mac-os < 10, which we don't
support anymore. So please ignore my comments on renaming those files.

Comment 30

[Jonathan Watt [:jwatt]]

(In reply to comment #26)
>      [...]
> >    else  // no value for meetOrSlice - give mMeetOrSlice the default value
> >      mMeetOrSlice = SVG_MEETORSLICE_MEET;
> 
> As above.

I don't agree with that. The spec allows meetOrSlice to be omitted, and in that
case the only sensible action seems to be for it to take the default value.
Certainly it isn't an error. 


(In reply to comment #27)
> Can you please add a comment above these lines:
>   // DOM property: preserveAspectRatio , #IMPLIED attrib: preserveAspectRatio

Actually not easily, I accidentally messed up my tree and I don't want to
inadvertantly introduce changes to other parts of the patch. Do you mind if I
add that as a seperate patch just after this stuff is checked in? 


Other than that I think I will have addressed all your comments in the following
four patches. 

Comment 31

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.h

Comment 32

[Jonathan Watt [:jwatt]]

Attachment: nsSVGAnimatedPreserveAspectRatio.cpp

Comment 33

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.h

Comment 34

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.cpp

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 151782 [details] [diff] [review]
patch for existing code - tested win2k

sorry, i don't have sr powers.

Comment 36

[Alex Fritze]

(In reply to comment #30)
> (In reply to comment #26)
> >      [...]
> > >    else  // no value for meetOrSlice - give mMeetOrSlice the default value
> > >      mMeetOrSlice = SVG_MEETORSLICE_MEET;
> > 
> > As above.
> 
> I don't agree with that. The spec allows meetOrSlice to be omitted, and in that
> case the only sensible action seems to be for it to take the default value.
> Certainly it isn't an error. 

Sorry, I meant the branch before that:
> else
>        mMeetOrSlice = SVG_MEETORSLICE_UNKNOWN;

Also, setting mAlign & mMeetOrSlice should be atomic, so if there is an error
parsing either you shouldn't call WillModify()/DidModify() and
mAlign/mMeetOrSlice should remain at their previous values.

Comment 37

[Jonathan Watt [:jwatt]]

Comment on attachment 151782 [details] [diff] [review]
patch for existing code - tested win2k

requesting sr= from jst for dom/ changes

Comment 38

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.cpp

Comment 39

[Alex Fritze]

Comment on attachment 151936 [details] [diff] [review]
nsSVGPreserveAspectRatio.cpp


>    else
>      rv = NS_ERROR_FAILURE;
>
>
>    token = nsCRT::strtok(rest, delimiters, &rest);

In case of an error in parsing align, there's no need to examine the next
token.

>    if (token) {
>      if (!strcmp(token, "meet"))
>        meetOrSlice = SVG_MEETORSLICE_MEET;
>      else if (!strcmp(token, "slice"))
>        meetOrSlice = SVG_MEETORSLICE_SLICE;
>      else
>        rv = NS_ERROR_FAILURE;
>    }
>    else
>      mMeetOrSlice = SVG_MEETORSLICE_MEET;

 Should be meetOrSlice, not mMeetOrSlice

>    if (NS_SUCCEEDED(rv)) {
>      WillModify();
>      mAlign = align;
>      mMeetOrSlice = meetOrSlice;
>      DidModify();
>    }
>  }
>  else  // align not specified
>    rv = NS_ERROR_FAILURE;
>
>  if (nsCRT::strtok(rest, delimiters, &rest))  // there's more
>    rv = NS_ERROR_FAILURE;

This should come before WillModify()/DidModify().

>NS_IMETHODIMP nsSVGPreserveAspectRatio::SetAlign(PRUint16 aAlign)
>{
>  if (aAlign > SVG_PRESERVEASPECTRATIO_XMAXYMAX)

You should also test for aAlign<= SVG_PRESERVEASPECTRATIO_UNKNOWN.

>NS_IMETHODIMP nsSVGPreserveAspectRatio::SetMeetOrSlice(PRUint16 aMeetOrSlice)
>{
>  if (aMeetOrSlice > SVG_MEETORSLICE_SLICE)
>    return NS_ERROR_FAILURE;

And for aMeetOrSlice<=SVG_MEETORSLICE_UNKNOWN

Comment 40

[Jonathan Watt [:jwatt]]

(In reply to comment #39)
> You should also test for aAlign<= SVG_PRESERVEASPECTRATIO_UNKNOWN.
> And for aMeetOrSlice<=SVG_MEETORSLICE_UNKNOWN

Since these variables are unsigned, do you mind if I use < *_NONE?

Comment 41

[Jonathan Watt [:jwatt]]

Err, that didn't make much sense. I mean is the following okay by you:

  if (aAlign < SVG_PRESERVEASPECTRATIO_NONE ||
      aAlign > SVG_PRESERVEASPECTRATIO_XMAXYMAX)

and 

  if (aMeetOrSlice < SVG_MEETORSLICE_MEET ||
      aMeetOrSlice > SVG_MEETORSLICE_SLICE)

Comment 42

[Jonathan Watt [:jwatt]]

Attachment: nsSVGPreserveAspectRatio.cpp

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 151782 [details] [diff] [review]
patch for existing code - tested win2k

sr=jst

Comment 44

[Alex Fritze]

checked in.

Comment 45

[Jonathan Watt [:jwatt]]

Attachment: tidy nsSVGSVGElement.cpp

Comment 46

[Alex Fritze]

Comment on attachment 152108 [details] [diff] [review]
tidy nsSVGSVGElement.cpp

checked in.