Bug 108077 – [HarfBuzz] Remove the HarfBuzz-old code

• Component: Platform
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

As discussed in https://bugs.webkit.org/show_bug.cgi?id=37984#c26 it seems that no port needs the harfbuzz-old code any more.
If there are no objections, let's remove it.

[Dominik Röttsches (drott)]

Created attachment 184984 [details]
Patch

[Dominik Röttsches (drott)]

Comment on attachment 184984 [details]
Patch

Eli, is that okay for the Blackberry port?

[Benjamin Poulain]

Comment on attachment 184984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184984&action=review

Thanks for cleaning WebCore!

Please give some time for responses on webkit-dev before landing, EWS do not cover everyone unfortunately.

> Source/WebCore/ChangeLog:17
> +        (FontPlatformData): Removing USE(HARFBUZZ_NG) ifdef's.

"ifdef's"?
I have the feeling "ifdefs" is a more common notation(?).

Ditto * 4.

> Source/WebCore/platform/graphics/freetype/FontPlatformData.h:68
>      HarfBuzzNGFace* harfbuzzFace() const;

Shouldn't this be harfBuzzFace()?

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:-43
> -#if USE(HARFBUZZ_NG)
>  #include "HarfBuzzNGFace.h"
> -#else

You keep that header unsorted?

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:51
>  class FontDescription;
>  
> -#if USE(HARFBUZZ_NG)
>  class HarfBuzzNGFace;

Move the declarations together.

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:111
>      HarfBuzzNGFace* harfbuzzFace() const;

Shouldn't this be harfBuzzFace()?

[Dominik Röttsches (drott)]

Created attachment 185199 [details]
Patch

[Dominik Röttsches (drott)]

(In reply to comment #3)
> (From update of attachment 184984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184984&action=review

Thanks for the review, Benjamin.

> > Source/WebCore/ChangeLog:17
> > +        (FontPlatformData): Removing USE(HARFBUZZ_NG) ifdef's.
> 
> "ifdef's"?
> I have the feeling "ifdefs" is a more common notation(?).
> 
> Ditto * 4.

Changed to ifdefs.

> > Source/WebCore/platform/graphics/freetype/FontPlatformData.h:68
> >      HarfBuzzNGFace* harfbuzzFace() const;
> 
> Shouldn't this be harfBuzzFace()?

It should - but when looking at this, I found a couple of other naming inconsistencies so that I would like to address those in a separate patch once we know there are no objections to in fact removing the code. I filed bug 108170 for the renames.

> > Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:-43
> > -#if USE(HARFBUZZ_NG)
> >  #include "HarfBuzzNGFace.h"
> > -#else
> 
> You keep that header unsorted?

Merged and sorted with the other ones.

> > Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:51
> >  class FontDescription;
> >  
> > -#if USE(HARFBUZZ_NG)
> >  class HarfBuzzNGFace;
> 
> Move the declarations together.

Yes, empty line removed.

> > Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:111
> >      HarfBuzzNGFace* harfbuzzFace() const;
> 
> Shouldn't this be harfBuzzFace()?

See above.

[Eli Fidler]

(In reply to comment #2)
> (From update of attachment 184984 [details])
> Eli, is that okay for the Blackberry port?

Yes, it's fine with us. We've moved to HarfbuzzNG. We can remove the CMakeLists stuff for BlackBerry/Harfbuzz (not-NG) later.

[Tony Chang]

This looks great, thanks for doing this!

[Tony Chang]

In a follow up patch, we should move the files out of the ng subdirectory and remove NG from the filenames.

[Dominik Röttsches (drott)]

Created attachment 185426 [details]
Patch

[Dominik Röttsches (drott)]

(In reply to comment #9)
> Created an attachment (id=185426) [details]
> Patch

Rebased, one more "ifdef's" replaced.

[Eric Seidel (no email)]

And a great choir of angles descended from the heavens and sang forth praises throughout the land!

[Eric Seidel (no email)]

(Thank you.) :)

[WebKit Review Bot]

Comment on attachment 185426 [details]
Patch

Clearing flags on attachment: 185426

Committed r141241: <http://trac.webkit.org/changeset/141241>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.