Closed Bug 612488 Opened 9 years ago Closed 9 years ago

gfxFontStyle::featureSettings should be a direct nsTArray

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: adam)

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 1 obsolete file)

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 :-).
Attached file hg Diff
You mean, like this example (in the diff file attachment) ?
(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.
Like this? It's my first bug. :D
Attachment #493552 - Attachment description: hg diff → patch, make mFeatureSettings an array rather than a pointer to an array
Attachment #493552 - Flags: review?(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!
Attachment #493552 - Flags: review?(jfkthame)
Updated using Jonathan's suggestions.
Attachment #493552 - Attachment is obsolete: true
Attachment #493600 - Flags: review+
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
Attachment #493600 - Flags: review+ → review?(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 <email@example.com>") 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!
Attachment #493600 - Flags: review?(jfkthame) → review+
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.
Attachment #493600 - Flags: approval2.0?
Attachment #493600 - Flags: approval2.0? → approval2.0+
Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/4f0745429ccb
Assignee: nobody → adam
Status: NEW → RESOLVED
Closed: 9 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.