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.
Created attachment 184984 [details] Patch
Comment on attachment 184984 [details] Patch Eli, is that okay for the Blackberry port?
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()?
Created attachment 185199 [details] Patch
(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.
(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.
This looks great, thanks for doing this!
In a follow up patch, we should move the files out of the ng subdirectory and remove NG from the filenames.
Created attachment 185426 [details] Patch
(In reply to comment #9) > Created an attachment (id=185426) [details] > Patch Rebased, one more "ifdef's" replaced.
And a great choir of angles descended from the heavens and sang forth praises throughout the land!
(Thank you.) :)
Comment on attachment 185426 [details] Patch Clearing flags on attachment: 185426 Committed r141241: <http://trac.webkit.org/changeset/141241>
All reviewed patches have been landed. Closing bug.