WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
108077
[HarfBuzz] Remove the HarfBuzz-old code
https://bugs.webkit.org/show_bug.cgi?id=108077
Summary
[HarfBuzz] Remove the HarfBuzz-old code
Dominik Röttsches (drott)
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Dominik Röttsches (drott)
Comment 1
2013-01-28 07:57:08 PST
Created
attachment 184984
[details]
Patch
Dominik Röttsches (drott)
Comment 2
2013-01-28 15:22:24 PST
Comment on
attachment 184984
[details]
Patch Eli, is that okay for the Blackberry port?
Benjamin Poulain
Comment 3
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()?
Dominik Röttsches (drott)
Comment 4
2013-01-29 01:39:38 PST
Created
attachment 185199
[details]
Patch
Dominik Röttsches (drott)
Comment 5
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.
Eli Fidler
Comment 6
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.
Tony Chang
Comment 7
2013-01-29 12:01:42 PST
This looks great, thanks for doing this!
Tony Chang
Comment 8
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.
Dominik Röttsches (drott)
Comment 9
2013-01-30 01:15:13 PST
Created
attachment 185426
[details]
Patch
Dominik Röttsches (drott)
Comment 10
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.
Eric Seidel (no email)
Comment 11
2013-01-30 02:20:59 PST
And a great choir of angles descended from the heavens and sang forth praises throughout the land!
Eric Seidel (no email)
Comment 12
2013-01-30 02:21:17 PST
(Thank you.) :)
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2013-01-30 02:47:37 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug