612488 - gfxFontStyle::featureSettings should be a direct nsTArray

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

nsTArray<gfxFontFeature> *featureSettings;

nsTArray uses only one word of storage for an empty array, so this should just be declared as an nsTArray directly. Simpler faster code. Easy fix :-).

Comment 1

[Martin Cerdeira (mRt)]

Attachment: hg Diff

Comment 2

[Martin Cerdeira (mRt)]

You mean, like this example (in the diff file attachment) ?

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to comment #2)
> You mean, like this example (in the diff file attachment) ?

No, he means that the relevant fields in gfxFontStyle and gfxFontEntry (see gfxFont.h) should actually be an nsTArray<gfxFontFeature> rather than a pointer to one; and then there'll be corresponding code changes where these are used.

Comment 4

[Adam]

Attachment: patch, make mFeatureSettings an array rather than a pointer to an array

Like this? It's my first bug. :D

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 493552 [details] [diff] [review]
patch, make mFeatureSettings an array rather than a pointer to an array

Thanks (and welcome to the codebase!)

This looks fine; a couple of minor points that can be cleaned up a little further:

>-    if (!aFeatureSettings.IsEmpty()) {
>-        featureSettings = new nsTArray<gfxFontFeature>;
>-        ParseFontFeatureSettings(aFeatureSettings, *featureSettings);
>-        if (featureSettings->Length() == 0) {
>-            delete featureSettings;
>-            featureSettings = nsnull;
>-        }
>-    }
>+    if (!aFeatureSettings.IsEmpty())
>+        ParseFontFeatureSettings(aFeatureSettings, featureSettings);

There's no need for the IsEmpty() test here; if the string is empty, ParseFontFeatureSettings() will return almost immediately anyhow. In the old code, checking IsEmpty() allowed us to avoid the allocation and deallocation of the nsTArray object in the (common) case of an empty string, but that's no longer an issue, so let's shorten/simplify the code here.

>-    if (aStyle.featureSettings) {
>-        featureSettings = new nsTArray<gfxFontFeature>;
>-        featureSettings->AppendElements(*aStyle.featureSettings);
>-    }
>+    if (!aStyle.featureSettings.IsEmpty())
>+        featureSettings.AppendElements(aStyle.featureSettings);

Likewise, no need to test IsEmpty() here; AppendElements() will handle an empty array for us.

>-            ((!featureSettings && !other.featureSettings) ||
>-             (featureSettings && other.featureSettings &&
>-              (*featureSettings == *other.featureSettings))) &&
>+            ((featureSettings.IsEmpty() && other.featureSettings.IsEmpty()) ||
>+             (!featureSettings.IsEmpty() && !other.featureSettings.IsEmpty() &&
>+              (featureSettings == other.featureSettings))) &&

Drop the IsEmpty() tests, just compare featureSettings and other.featureSettings -- this will handle empty arrays.

>     // css features need to be merged with the existing ones, if any
>     const nsTArray<gfxFontFeature> *cssFeatures =
>-        mFont->GetStyle()->featureSettings;
>-    if (!cssFeatures) {
>-        cssFeatures = mFont->GetFontEntry()->mFeatureSettings;
>-    }
>-    if (cssFeatures) {
>+        &mFont->GetStyle()->featureSettings;
>+    if (cssFeatures->IsEmpty())
>+        cssFeatures = &mFont->GetFontEntry()->mFeatureSettings;

Yes, we want to check cssFeatures->IsEmpty() here, as we only want the fontEntry's features to be used if there was no overriding setting in the style. However, our style guidelines call for braces around the body of the if-statement, even when it's only a single statement that is controlled. (Existing code is not always consistent with this, but we're trying to stick to it as new code is written/modified.)

>+    if (!cssFeatures->IsEmpty()) {
>         for (PRUint32 i = 0; i < cssFeatures->Length(); ++i) {

Here, you can drop the IsEmpty() test, as the loop will not execute anyway in that case (Length() will be zero).

>-    if (aFeatureSettings) {
>-        mFeatureSettings = new nsTArray<gfxFontFeature>;
>-        mFeatureSettings->AppendElements(*aFeatureSettings);
>-    }
>+    if (!aFeatureSettings.IsEmpty())
>+        mFeatureSettings.AppendElements(aFeatureSettings);

No need for the IsEmpty() test here.

>-            if (pe->mFeatureSettings) {
>-                fe->mFeatureSettings = new nsTArray<gfxFontFeature>;
>-                fe->mFeatureSettings->AppendElements(*pe->mFeatureSettings);
>-            }
>+            if (!pe->mFeatureSettings.IsEmpty())
>+                fe->mFeatureSettings.AppendElements(pe->mFeatureSettings);

And again.

>-                if (aProxyEntry->mFeatureSettings) {
>-                    fe->mFeatureSettings = new nsTArray<gfxFontFeature>;
>-                    fe->mFeatureSettings->AppendElements(*aProxyEntry->mFeatureSettings);
>-                }
>+                if (!aProxyEntry->mFeatureSettings.IsEmpty())
>+                    fe->mFeatureSettings.AppendElements(aProxyEntry->mFeatureSettings);

And again.

If you attach an updated patch and flag it for review, I'll also push it to tryserver so we can verify that all the unit tests pass (though I don't see any reason why they wouldn't, the changes look correct and safe). Thanks!

Comment 6

[Adam]

Attachment: patch, make mFeatureSettings an array rather than a pointer to an array #2

Updated using Jonathan's suggestions.

Comment 7

[John Daggett (:jtd)]

Comment on attachment 493600 [details] [diff] [review]
patch, make mFeatureSettings an array rather than a pointer to an array #2

Adam, to request a review set the review field to '?' and enter the requestee.  You can usually figure out the right person to review something by looking at the history of one of the files you're modifying.  For example:

http://hg.mozilla.org/mozilla-central/log/c194928bb4a9/gfx/thebes/gfxFont.cpp

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 493600 [details] [diff] [review]
patch, make mFeatureSettings an array rather than a pointer to an array #2

Looks fine to me. I've included this in a tryserver push to double-check that nothing unexpected happens.

Adam, we normally include the patch author's name (in the form "First Last <[e-mail]>") when in the log message when committing code to the repository. If you'd like to be credited in that form, please let me know the appropriate name to use. Thanks!

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 493600 [details] [diff] [review]
patch, make mFeatureSettings an array rather than a pointer to an array #2

Requesting approval to land for 2.0. This does not involve any change in behavior, it's purely cleanup to simplify/optimize the code.

Comment 10

[Jonathan Kew [:jfkthame]]

Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/4f0745429ccb