484176 - SVG: SMIL 3.0 allowReorder="yes"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[jonathan chetwynd]

Attachment: allowReorder demo

SMIL 3.0 includes:
allowReorder="yes"
to meet the needs of user language preference

Doug confirmes 
http://lists.w3.org/Archives/Public/www-svg/2009Mar/0166.html

that  SMIL 3.0 includes:
allowReorder="yes"
to meet the needs of user language preference
http://www.w3.org/TR/SMIL3/smil-content.html#adef-allowReorder

he suggests

"it is in a Recommendation, and SMIL is the canonical (and normative) source for the <switch> element in SVG, so it should be perfectly cromulent to implement it on this basis"

"I would guess this would be a pretty trivial patch, and would have no negative impact on existing content."

**"I agree that this would best be filed as a new bug,"

Comment 1

[Doug Schepers]

See Bug 326375 for some background on this issue.  This feature should be implemented for SVG content, as well (so, it is not just a SMIL patch, and it is not directly related to animation).  The SVG WG intends to add this to the next version of the specification.

Comment 2

[jonathan chetwynd]

to test either the attachment or the URI,

set mozilla preferences, content, languages**

include Italian, but give another language such as English or French a higher preference.

the users most prefered language, that the author provides, 
should now be displayed when using these examples.

**some operating systems such as mac os x allow the user to rank languages according to their preference, and this information is available to native applications.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Jonathan Watt [:jwatt]]

So we have two types of conditional processing.

 1) standalone where requiredFeatures, requiredExtensions or systemLanguage
    is found on an element that is not a child of a <switch>

 2) switch processing, which is kinda special since you have this thing
    where whether you render an element or not also depends on it's siblings,
    and now you can even "reorder"

Everything that's in nsSVGFeatures.cpp currently is used for both cases, but your patch adds <switch> specific code to the file. I think it would be better to keep <switch> specific code in nsSVGSwitchElement.cpp.

I think ideally we'd just tweak NS_SVG_PassesConditionalProcessingTests slightly to allow us to do any of the following three things:


  // check that everything is true:

  NS_SVG_PassesConditionalProcessingTests(content)


  // check that everything is true, passing in prefetched pref:

  NS_SVG_PassesConditionalProcessingTests(content, acceptLangs)


  // ignore systemLanguage for when <switch> handles that for "reordering":

  NS_SVG_PassesConditionalProcessingTests(content, kIgnoreSystemLanguage)


Which we can do by changing it's signature to:

  NS_SVG_PassesConditionalProcessingTests(nsIContent *aContent,
                                          nsSubstring *aAcceptLangs = nsnull)

and defining kIgnoreSystemLanguage to be some special, unused pointer, in nsSVGFeatures.h (such as 0x1). We've used this trick in other places where it leads to code that's more readable and does what the reader expects at the call sites. The function can then return true early if it encounters kIgnoreSystemLanguage, else if aAcceptLangs is nsnull, it fetches the pref value, else it just uses aAcceptLangs.

That done, GetBestLanguagePreferenceIndex and NS_SVG_GetChildPassingConditionalProcessingTests would move to nsSVGSwitchElement.cpp (the latter would simply become the contend of UpdateActiveChild).

Also, in NS_SVG_GetChildPassingConditionalProcessingTests, can't you just use:

  PRBool allowReorder = aContent->AttrValueIs(kNameSpaceID_None,
                                              nsGkAtoms::allowReorder,
                                              "yes", eCaseMatters);

And we don't need the StripWhitespace() calls since nsCommaSeparatedTokenizer essentially does that for us.

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #4)

> and defining kIgnoreSystemLanguage to be some special, unused pointer, in
> nsSVGFeatures.h (such as 0x1). We've used this trick in other places where it
> leads to code that's more readable and does what the reader expects at the call
> sites. The function can then return true early if it encounters
> kIgnoreSystemLanguage, else if aAcceptLangs is nsnull, it fetches the pref
> value, else it just uses aAcceptLangs.

Not sure where this is done so I've made something up. Also if you're going to make me create an nsSVGFeatures.h I may as well make the whole thing a class and get rid of all the NS_SVG_ stuff.

Comment 6

[Robert Longson [:longsonr]]

Attachment: updated patch

Contains an optimisation. If we've found the best possible match in the allowReorder case then we can stop looking.

Comment 7

[Jonathan Watt [:jwatt]]

> Not sure where this is done so I've made something up.

You've done exactly what I meant with:

>+  if (aAcceptLangs == (void *)kIgnoreSystemLanguage) {
>+    return PR_TRUE;
>+  }
>+
>+  const nsAdoptingString& acceptLangs = aAcceptLangs ? *aAcceptLangs :
>+    nsContentUtils::GetLocalizedStringPref("intl.accept_languages");

Maybe my description was rubbish. :-)



> Also if you're going to
> make me create an nsSVGFeatures.h I may as well make the whole thing a class
> and get rid of all the NS_SVG_ stuff.

Well I don't intend to force you. ;-) If you don't like my suggestions (or you don't want to do them in a particular patch) then feel free do push back and make me reconsider. :-)

Some other feedback:

Can you put /*static*/ comments before the return types in nsSVGFeatures.cpp? So instead of:

  PRBool
  nsSVGFeatures::HaveFeature(const nsAString& aFeature)

we'd have:

  /*static*/ PRBool
  nsSVGFeatures::HaveFeature(const nsAString& aFeature)

I find that helpful in the other places we do that.

GetBestLanguagePreferenceIndex is <switch> specific code, so I think that should also go in nsSVGSwitchElement.cpp.

Why have:

  if (aAcceptLangs == (void *)kIgnoreSystemLanguage) {

Doesn't just:

  if (aAcceptLangs == kIgnoreSystemLanguage) {

work since the pointers are of the same type?

> static const nsAdoptingString * kIgnoreSystemLanguage;

Can you make that:

  static const nsAdoptingString * const kIgnoreSystemLanguage;

since nobody should be changing kIgnoreSystemLanguage to point somewhere else.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #7)
> 
> Maybe my description was rubbish. :-)

The description was fine. What I was looking for was where you had seen it so I could lazily copy it :-)

> Well I don't intend to force you. ;-) If you don't like my suggestions (or you
> don't want to do them in a particular patch) then feel free do push back and
> make me reconsider. :-)

Indeed, in fact read on... ;-)

> Can you put /*static*/ comments before the return types in nsSVGFeatures.cpp?

Done.

> GetBestLanguagePreferenceIndex is <switch> specific code, so I think that
> should also go in nsSVGSwitchElement.cpp.

Not done. I originally wanted to have the code like it is now but without your neat argument munging trick I couldn't make it work in a simple fashion.

The reasons I wanted to get to where we are now are:

nsSVGFeatures - analyses conditional processing attributes for an element. Only this code knows how to parse them to do that.

nsSVGSwitch - knows it needs to get the best child but doesn't know what the best child is as that would mean analyzing the conditional processing features.

If I move the method out of nsSVGFeatures I'll have language attribute parsing code in two different places and that doesn't seem very elegant compared to the current layered approach.

> 
> Why have:
> 
>   if (aAcceptLangs == (void *)kIgnoreSystemLanguage) {

Oops, this was inadvertently leftover from an earlier attempted solution that I abandoned.

> > static const nsAdoptingString * kIgnoreSystemLanguage;
> 
> Can you make that:
> 
>   static const nsAdoptingString * const kIgnoreSystemLanguage;

Done.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 371086 [details] [diff] [review]
address review comments

Okay, I don't really agree, but r=jwatt with the following changes. :-)

>+        switch (index) {
>+        case 0:
>+          // best possible match
>+          return 0;
>+        case -1:
>+          // not found
>+          break;
>+        default:
>+          if (lowestIndex == -1 || index < lowestIndex) {
>+            lowestIndex = index;
>+            if (lowestIndex == 0) {
>+              // found the best possible match
>+              return lowestIndex;
>+            }

Need to delete this |if| statement.


>   if (aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::systemLanguage,
>                         value)) {
>     // Get our language preferences
>-    nsAutoString langPrefs(nsContentUtils::GetLocalizedStringPref("intl.accept_languages"));
>-    if (!langPrefs.IsEmpty()) {
>-      langPrefs.StripWhitespace();
>+    if (!acceptLangs.IsEmpty()) {
>       value.StripWhitespace();
>-      return MatchesLanguagePreferences(value, langPrefs);
>+      return MatchesLanguagePreferences(value, acceptLangs);
>     } else {
>       // For now, evaluate to true.
>       NS_WARNING("no default language specified for systemLanguage conditional test");
>       return !value.IsEmpty();
>     }

No need for the StripWhitespace() (you might want to skim the rest of the code for that), and the |else| is bogus since we return from the |if|.


>+  /**
>+   * Check whether the conditional processing attributes requiredFeatures,
>+   * requiredExtensions and systemLanguage all "return true" if they apply to
>+   * and are specified on the given element. Returns true if this element
>+   * should be rendered, false if it should not.
>+   *
>+   * @param aContent the element to test
>+   * @param aAcceptLangs the current language preferences

Please expand on aAcceptLangs:

 * @param aAcceptLangs Optional parameter to pass in the value of the intl.accept_languages
 *    preference if the caller has it cached. Alternatively, pass in kIgnoreSystemLanguage
 *    to skip the systemLanguage check if the caller is giving that special treatment.


>+  /**
>+   * Check whether we support the given extension string.
>+   *
>+   * @param aExtension the URI of an extension. Known extensions are
>+   *   MathML and XHTML.

Might as well be clear and say: "http://www.w3.org/1999/xhtml" and "http://www.w3.org/1998/Math/MathML"


>+          switch (languagePreferenceIndex) {
>+          case 0:
>+            // best possible match
>+            return child;
>+          case -1:
>+            // not found
>+            break;
>+          default:
>+            if (bestLanguagePreferenceIndex == -1 ||
>+                languagePreferenceIndex < bestLanguagePreferenceIndex) {
>+              bestLanguagePreferenceIndex = languagePreferenceIndex;
>+              bestChild = child;
>+            } else if (!bestChild) {
>+              bestChild = child;
>+            }

You can't hit the |else if|, since that would mean that we have bestLanguagePreferenceIndex != -1, in which case we must have a bestChild.

Comment 10

[Robert Longson [:longsonr]]

Attachment: for check-in

Comment 11

[Robert Longson [:longsonr]]

(In reply to comment #9)
> >+          default:
> >+            if (bestLanguagePreferenceIndex == -1 ||
> >+                languagePreferenceIndex < bestLanguagePreferenceIndex) {
> >+              bestLanguagePreferenceIndex = languagePreferenceIndex;
> >+              bestChild = child;
> >+            } else if (!bestChild) {
> >+              bestChild = child;
> >+            }
> 
> You can't hit the |else if|, since that would mean that we have
> bestLanguagePreferenceIndex != -1, in which case we must have a bestChild.

Very clever of you to notice that! I spotted another line like it where I was testing index against -1. I've changed it to this...

+  PRInt32 lowestIndex = -1;
+
+  while (attributeTokenizer.hasMoreTokens()) {
+    const nsSubstring &attributeToken = attributeTokenizer.nextToken();
+    nsCommaSeparatedTokenizer languageTokenizer(aAcceptLangs);
+    PRInt32 index = 0;
+    while (languageTokenizer.hasMoreTokens()) {
+      if (nsStyleUtil::DashMatchCompare(attributeToken,
+                                        languageTokenizer.nextToken(),
+                                        defaultComparator)) {
+        if (index == 0) {
+          // best possible match
+          return 0;
+        } else if (lowestIndex == -1 || index < lowestIndex) {
+          lowestIndex = index;
+        }
+      }
+      ++index;
+    }
+  }

Comment 12

[Jonathan Watt [:jwatt]]

Looks good, although you could drop the |else| since the |if| returns.

Comment 13

[Robert Longson [:longsonr]]

Sure, I'll do that on check-in.

Comment 14

[Robert Longson [:longsonr]]

Attachment: address final review comment

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 371783 [details] [diff] [review]
address final review comment

You could have used "namespace mozilla::SVGFeatures" instead of a class nsSVGFeatures.

This needs tests. You can set the intl.accept_languages temporarily during the test if that helps, see use_large_cache.js for example.

Is this really an important thing to be working on?

Is this really

Comment 16

[Robert Longson [:longsonr]]

I don't think I can do a test roc. 

reftests can't enable privileges so I can't get or set the accept_languages preference and I can only do a visual test as there's no DOM call that tells me what the active switch element is so I can't do a mochitest can I?

Comment 17

[Robert Longson [:longsonr]]

FWIW there are reftests already for everything except systemLanguage i.e. requiredExtensions and requiredFeatures.

Comment 18

[Robert Longson [:longsonr]]

I don't know how to write tests roc. See comment 16.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Reftests can enable privileges. See http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/448987.html for example

Comment 20

[Robert Longson [:longsonr]]

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/reftest.list

# == 448987.html 448987-ref.html  # Disabled for now - it needs privileges

However, I have figured out how to do this. The bounding box of a switch is the bounding box of its active child so if I make the children different sizes I can figure out which one is active.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, sorry.

Mochitests can do reftest-like-things using WindowSnapshot.js. See
http://mxr.mozilla.org/mozilla-central/search?string=windowsnapshot

Comment 22

[Robert Longson [:longsonr]]

Attachment: with tests

This patch contains a change to the allowReorder algorithm i.e. the GetBestLanguagePreferenceRank method.

I realised that if the user has specified en-gb,en as language preferences and the switch contains en-us and en-gb elements that it makes sense to display en-gb rather than the prefix-only match of en-us. The GetBestLanguagePreferenceRank algorithm now ranks exact matches above prefix matches.

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 373504 [details] [diff] [review]
with tests

Good point.

I think messing with the number to essentially make the LO bit a flag for prefix match is a bit yucky (returning a simple struct might be better), but okay. r=jwatt

Comment 24

[Jonathan Watt [:jwatt]]

Going to check this in? :-)

Comment 25

[Robert Longson [:longsonr]]

checked in http://hg.mozilla.org/mozilla-central/rev/135d7cdcd783