Summary: | [HarfBuzz] Remove the HarfBuzz-old code | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominik Röttsches (drott) <d-r> | ||||||||
Component: | Platform | Assignee: | Dominik Röttsches (drott) <d-r> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bashi, efidler, eric, fujisawa.jun, gyuyoung.kim, hausmann, junov, mrobinson, rakuco, senorblanco, tony, webkit.review.bot, zan | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 108170 | ||||||||||
Attachments: |
|
Description
Dominik Röttsches (drott)
2013-01-28 06:02:48 PST
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. |