554704 - Rename "aCanCache" param (for nsISMILAttr::ValueFromString)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

The function nsISMILAttr::ValueFromString() has an outparam |aCanCache|, which basically indicates "is this string always guaranteed to parse to the same nsSMILValue, regardless of context?"

The name |aCanCache| is a little misleading, since the value isn't actually ever "cached" per se.  (Rather, we use this flag to know whether to bother recomputing the animated value).

<jwatt> aValueOnlyDependsOnStr?
<jwatt> a little long, but much less misleading
<jwatt> and pretty clear

"aValueOnlyDependsOnStr" seems like an improvement to me.

Comment 2

[Felipe Corrêa da Silva Sanches]

wouldn't it be better to call it aValueOnlyDependsOnString ?

How do I assign this bug to myself? I am not finding a way to setup this on the bugzilla ui.

Comment 3

[Robert Longson [:longsonr]]

Do you see (edit) after assigned to?

Assigned To: Nobody; OK to take it and work on it (edit) 

If not, then you probably need editbugs in order to get that button to appear. http://www.gerv.net/hacking/before-you-mail-gerv.html

Comment 4

[Robert Longson [:longsonr]]

(In reply to comment #2)
> wouldn't it be better to call it aValueOnlyDependsOnString ?
> 

Yes.

Comment 5

[Felipe Corrêa da Silva Sanches]

Attachment: CanCache -> ValueOnlyDependsOnString

Comment 6

[Felipe Corrêa da Silva Sanches]

what about some other places where "CanCache" is used in the codebase such as aCanCacheSoFar in these files:

content/smil/nsSMILAnimationFunction.cpp
content/smil/nsSMILAnimationFunction.h

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> what about some other places where "CanCache" is used in the codebase

Yes -- as long as it's related to this same API in our SVG/SMIL code (which it is in the nsSMILAnimationFunction place you brought up), you should apply the same CanCache --> ValueOnlyDependsOnString transform there as well.

Patch looks good, aside from a few places that break 80 characters per line & could easily be fixed.  I've highlighted them below.
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Line_Length

There are a few other places where you're *just* over 80 characters, and there's no clean way to avoid it, and the contextual code is often already over 80-chars too.  I'm less concerned about those.  If you fix the ones below (and do the rename you mentioned on e.g. aCanCacheSoFar, without breaking 80chars where possible), I'll be happy. :)

>+++ b/content/smil/nsISMILAttr.h	Tue May 25 23:29:07 2010 +0200
>@@ -69,16 +69,16 @@
>    *                    animateTransform where the 'type' attribute is needed to
>    *                    parse the value.
>    * @param[out] aValue Outparam for storing the parsed value.
>-   * @param[out] aCanCache Outparam for indicating whether the parsed value
>-   *                       can be reused in future samples -- i.e. whether the
>-   *                       given string is always guaranteed to compute
>-   *                       to the same nsSMILValue.
>+   * @param[out] aValueOnlyDependsOnString Outparam for indicating whether the parsed value
>+   *                                       can be reused in future samples -- i.e. whether the
>+   *                                       given string is always guaranteed to compute
>+   *                                       to the same nsSMILValue.

Put "Outparam for indicating..." on its own line, and align its block with the block of documentation immediately above (ending with "parse the value.")

>+++ b/content/smil/nsSMILParserUtils.cpp	Tue May 25 23:29:07 2010 +0200
>@@ -548,26 +548,26 @@
>     nsresult rv = mSMILAttr->ValueFromString(aValueStr, mSrcElement,
>-                                             newValue, tmpCanCache);
>+                                             newValue, tmpValueOnlyDependsOnString);

Newline between "newValue," and "tmpValueOnlyDependsOnString", please.


>+++ b/content/svg/content/src/nsSVGLength2.cpp	Tue May 25 23:29:07 2010 +0200
>@@ -522,7 +522,7 @@
>-  aCanCache = (unitType != nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE &&
>+  aValueOnlyDependsOnString = (unitType != nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE &&
>                unitType != nsIDOMSVGLength::SVG_LENGTHTYPE_EMS &&

Add newline after "aValueOnlyDependsOnString =".

Comment 8

[Jonathan Watt [:jwatt]]

I think the reason I said "Str" is because the name of the argument is "aStr", not "aString". Using "String" is fine by me though.

Comment 9

[Felipe Corrêa da Silva Sanches]

Attachment: max 80 chars + fix aCanCacheSoFar

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 448469 [details] [diff] [review]
max 80 chars + fix aCanCacheSoFar

>+++ b/content/smil/nsSMILAnimationFunction.cpp	Tue Jun 01 03:12:10 2010 +0200
>+    PRBool ValueOnlyDependsOnString;
>     nsresult rv = aSMILAttr.ValueFromString(attValue, mAnimationElement,
>-                                            aResult, canCache);
>+                                            aResult, ValueOnlyDependsOnString);
>     if (NS_FAILED(rv))
>       return PR_FALSE;
> 
>-    if (!canCache) {
>-      aCanCacheSoFar = PR_FALSE;
>+    if (!ValueOnlyDependsOnString) {
>+      aValueOnlyDependsOnStringSoFar = PR_FALSE;

The variable "ValueOnlyDependsOnString" should start with a lowercase 'v'.

r=dholbert with that change.

Comment 11

[Felipe Corrêa da Silva Sanches]

Attachment: lowercase "valueOnlyDepends..."

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 448538 [details] [diff] [review]
lowercase "valueOnlyDepends..."

r=dholbert

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 448538 [details] [diff] [review]
lowercase "valueOnlyDepends..."

Sorry for dropping this after the review -- meant to flag jwatt for additional review.

Comment 14

[Daniel Holbert [:dholbert]]

(I verified that the patch still applies cleanly, FWIW)

Comment 15

[Jonathan Watt [:jwatt]]

So given the offlist email you just sent me, it seems like aValueOnlyDependsOnString is also a misleading name, since we'd need to set that variable to PR_FALSE in the case of a percentage unit in a length list, even though the nsSMILValue itself won't change if the viewport changes.

Would aPreventSandwichCaching be a better name?

Comment 16

[Jonathan Watt [:jwatt]]

Or aPreventCachingOfSandwich, to make it clearer that it's the whole sandwich that can't be cached, not just the one layer.

Comment 17

[Daniel Holbert [:dholbert]]

Both of those sound fine to me, though they have inverted meaning w.r.t. aCanCache / aValueOnlyDependsOnString (and hence would require more code changes).

Comment 18

[Jonathan Watt [:jwatt]]

The logic is inverted, but I think that's necessary to get accurate naming, no?

Comment 19

[Daniel Holbert [:dholbert]]

Well, not strictly necessary -- e.g. something like aDoesntPreventCachingOfSandwich would be an accurate name without inverting the logic. :) But that's super awkward-sounding.

I think aPreventCachingOfSandwich (with logic inverted) is the most sensible name I've heard yet.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch with rename to aPreventCachingOfSandwich and inverted logic

It's not really fair to get Felipe to redo the patch, so I did it for him. I've left the credits as his in the patch too.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 453357 [details] [diff] [review]
patch with rename to aPreventCachingOfSandwich and inverted logic

Nicely done! Looks good to me.

r=dholbert

Comment 22

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/683467d7a222