1187145 - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are nine occurrences of Enumerate() in this directory.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::Data() rather than nsBaseHashtable::Iterator::UserData(). (The latter should be used when replacing nsBaseHashtable::EnumerateRead()).

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Update nits.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Improved readability of types.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8662051 [details] [diff] [review]
patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Review of attachment 8662051 [details] [diff] [review]:
-----------------------------------------------------------------

I have some drive-by comments...

::: gfx/thebes/gfxFT2FontList.cpp
@@ +1158,5 @@
>          // so we just maintain the existing order)
> +        for (auto iter = mFontFamilies.Iter(); !iter.Done(); iter.Next()) {
> +            nsStringHashKey::KeyType key = iter.Key();
> +            nsRefPtr<gfxFontFamily>& family = iter.Data();
> +            FinalizeFamilyMemberList(key, family, false /* aSortFaces */);

I usually write cases like this as "/* aSortFaces = */ false".

@@ +1331,5 @@
>  void
>  gfxFT2FontList::GetSystemFontList(InfallibleTArray<FontListEntry>* retValue)
>  {
> +    for (auto iter = mFontFamilies.Iter(); !iter.Done(); iter.Next()) {
> +        FT2FontFamily *family = static_cast<FT2FontFamily*>(iter.Data().get());

Put the '*' on the left? I know this file in inconsistent, but on-the-left is official Mozilla style so you might as well do it for changed code :)

Actually, you might as well use |auto| here instead. I think it's better when the type is obvious due to a static_cast.

@@ +1335,5 @@
> +        FT2FontFamily *family = static_cast<FT2FontFamily*>(iter.Data().get());
> +        family->AddFacesToFontList(retValue, FT2FontFamily::kVisible);
> +    }
> +    for (auto iter = mHiddenFontFamilies.Iter(); !iter.Done(); iter.Next()) {
> +        FT2FontFamily *family = static_cast<FT2FontFamily*>(iter.Data().get());

Ditto.

@@ +1477,5 @@
>      // hence @font-face { src:local(...) } will not find hidden fonts.
> +    for (auto iter = mFontFamilies.Iter(); !iter.Done(); iter.Next()) {
> +        nsStringHashKey::KeyType key = iter.Key();
> +        nsRefPtr<gfxFontFamily>& fontFamily = iter.Data();
> +        fontEntry = FindFullName(key, fontFamily, fullName);

I would inline FindFullName() here.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +283,5 @@
>      TimeStamp start = TimeStamp::Now();
> +    bool timedOut = false;
> +    // if mFirstChar is not 0, only load facenames for families
> +    // that start with this character
> +    char16_t firstChar(0);

|char16_t firstChar = 0;| looks better.

Comment 5

[Sotaro Ikeda [:sotaro]]

Thanks for the view, I''s update in a next patch.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8665118 [details] [diff] [review]
patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Review of attachment 8665118 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFT2FontList.cpp
@@ +1460,5 @@
>  
>      // Note that we only check mFontFamilies here, not mHiddenFontFamilies;
>      // hence @font-face { src:local(...) } will not find hidden fonts.
> +    for (auto iter = mFontFamilies.Iter(); !iter.Done(); iter.Next()) {
> +        // Check family name, based on the assumption that the 

Nit: trailing whitespace.

@@ +1482,5 @@
> +                }
> +                if (fe->Name().Equals(fullName,
> +                                      nsCaseInsensitiveStringComparator())) {
> +                    fontEntry = static_cast<FT2FontEntry*>(fe);
> +                    break;

If you have a label at the end of the loop you could |goto| it from here...

@@ +1489,5 @@
> +        }
> +
> +        if (fontEntry) {
> +            break;
> +        }

... avoiding the need for this check.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Replace nsBaseHashtable::Enumerate() calls in gfx/ with iterators

Apply the comments.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2149155361c

Comment 10

[Nicholas Nethercote [inactive]]

Thank you for doing this bug.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2149155361c