Bug 108077 - [HarfBuzz] Remove the HarfBuzz-old code
Summary: [HarfBuzz] Remove the HarfBuzz-old code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominik Röttsches (drott)
URL:
Keywords:
Depends on:
Blocks: 108170
  Show dependency treegraph
 
Reported: 2013-01-28 06:02 PST by Dominik Röttsches (drott)
Modified: 2013-01-30 02:47 PST (History)
13 users (show)

See Also:


Attachments
Patch (58.11 KB, patch)
2013-01-28 07:57 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Patch (58.37 KB, patch)
2013-01-29 01:39 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Patch (58.32 KB, patch)
2013-01-30 01:15 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2013-01-28 06:02:48 PST
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.
Comment 1 Dominik Röttsches (drott) 2013-01-28 07:57:08 PST
Created attachment 184984 [details]
Patch
Comment 2 Dominik Röttsches (drott) 2013-01-28 15:22:24 PST
Comment on attachment 184984 [details]
Patch

Eli, is that okay for the Blackberry port?
Comment 3 Benjamin Poulain 2013-01-29 01:04:55 PST
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()?
Comment 4 Dominik Röttsches (drott) 2013-01-29 01:39:38 PST
Created attachment 185199 [details]
Patch
Comment 5 Dominik Röttsches (drott) 2013-01-29 01:47:51 PST
(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.
Comment 6 Eli Fidler 2013-01-29 08:06:00 PST
(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.
Comment 7 Tony Chang 2013-01-29 12:01:42 PST
This looks great, thanks for doing this!
Comment 8 Tony Chang 2013-01-29 12:03:32 PST
In a follow up patch, we should move the files out of the ng subdirectory and remove NG from the filenames.
Comment 9 Dominik Röttsches (drott) 2013-01-30 01:15:13 PST
Created attachment 185426 [details]
Patch
Comment 10 Dominik Röttsches (drott) 2013-01-30 01:16:45 PST
(In reply to comment #9)
> Created an attachment (id=185426) [details]
> Patch

Rebased, one more "ifdef's" replaced.
Comment 11 Eric Seidel (no email) 2013-01-30 02:20:59 PST
And a great choir of angles descended from the heavens and sang forth praises throughout the land!
Comment 12 Eric Seidel (no email) 2013-01-30 02:21:17 PST
(Thank you.) :)
Comment 13 WebKit Review Bot 2013-01-30 02:47:31 PST
Comment on attachment 185426 [details]
Patch

Clearing flags on attachment: 185426

Committed r141241: <http://trac.webkit.org/changeset/141241>
Comment 14 WebKit Review Bot 2013-01-30 02:47:37 PST
All reviewed patches have been landed.  Closing bug.