RESOLVED FIXED 89228
[Chromium] Change back "linuxish" to "linux" and include WebFontRendering.cpp on Android 
https://bugs.webkit.org/show_bug.cgi?id=89228
Summary [Chromium] Change back "linuxish" to "linux" and include WebFontRendering.cpp...
Xianzhu Wang
Reported 2012-06-15 09:52:39 PDT
In the patch of bug 88802 I moved public/linux/WebFontRendering.h to public/linuxish to share it by Linux and Android. However the name 'linuxish' looks not good because its meaning is unclear. Actually the implementation of the API depends on WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp, so it seems better to name it 'harfbuzz'. In fact, FontPlatformDataHarfBuzz.cpp itself doesn't depend on harfbuzz, but depend on Skia's freetype port. However, it is part of harfbuzz-based font rendering implementation. Naming the API directory 'freetype' may lead confusion with WebCore/platform/graphics/freetype which has a very different implementation and is not used by Chromium.
Attachments
patch (17.72 KB, patch)
2012-06-15 10:27 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
patch v2 (27.29 KB, patch)
2012-06-15 15:23 PDT, Xianzhu Wang 		tony: review-
tony: commit-queue-
DetailsFormatted DiffDiff
patch v3 (27.74 KB, patch)
2012-06-15 16:04 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
re-apply (fixed a include on Android) (28.18 KB, patch)
2012-06-15 19:05 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cq-02 (1.73 MB, application/zip)
2012-06-16 03:33 PDT, WebKit Review Bot 		no flags
Details
Patch (16.61 KB, patch)
2012-06-17 14:45 PDT, Xianzhu Wang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Xianzhu Wang
Comment 1 2012-06-15 10:27:52 PDT
Created attachment 147851 [details] patch
WebKit Review Bot
Comment 2 2012-06-15 10:32:00 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Xianzhu Wang
Comment 3 2012-06-15 12:06:59 PDT
'harfbuzz' also doesn't look good. Actually third_party/WebKit/Source/WebCore/platform/graphics/skia/FontPlatformDataSkia.cpponly depends on Skia. Though the related features may depend on some libraries used by Skia, the interfaces are available in Skia on all platforms. I'll do the following: 1. Move third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp to third_party/WebKit/Source/WebCore/platform/graphics/skia/FontPlatformDataSkia.cpp. 2. Move WebFontRendering.h and cpp out of subdirectories to make them available for all platforms.
Tony Chang
Comment 4 2012-06-15 13:53:00 PDT
Doesn't FontPlatformDataHarfBuzz.cpp depend on platform/graphics/harfbuzz/HarfBuzzSkia.h for HarfbuzzFace?
Xianzhu Wang
Comment 5 2012-06-15 14:00:49 PDT
(In reply to comment #4) > Doesn't FontPlatformDataHarfBuzz.cpp depend on platform/graphics/harfbuzz/HarfBuzzSkia.h for HarfbuzzFace? This is the only dependency and can be easily separated out. However, for now I don't want to do the step 2 in this bug. I'll reverse the dependency from WebFontRendering to FontPlatformDataHarfBuzz first.
Xianzhu Wang
Comment 6 2012-06-15 15:23:02 PDT
Created attachment 147913 [details] patch v2
Xianzhu Wang
Comment 7 2012-06-15 15:30:56 PDT
Comment on attachment 147851 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=147851&action=review > Source/WebKit/chromium/WebKit.gyp:779 > + 'public/harfbuzz', Sorry this should be removed. Will upload a new patch soon.
Xianzhu Wang
Comment 8 2012-06-15 15:32:30 PDT
(In reply to comment #7) > (From update of attachment 147851 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147851&action=review > > > Source/WebKit/chromium/WebKit.gyp:779 > > + 'public/harfbuzz', > > Sorry this should be removed. Will upload a new patch soon. Sorry again. Looked at wrong patch. Please ignore. The current patch is for review.
Tony Chang
Comment 9 2012-06-15 15:46:19 PDT
Comment on attachment 147913 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=147913&action=review > Source/WebCore/ChangeLog:13 > + * platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp: > + (WebCore::FontPlatformData::setupPaint): > + * platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h: > + (FontPlatformData): Please describe your changes here. > Source/WebKit/chromium/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + * WebKit.gyp: > + * public/WebFontRendering.h: Copied from Source/WebKit/chromium/public/linuxish/WebFontRendering.h. Please provide an overview of what is happening. I would probably use the word refactor in the description. > Source/WebKit/chromium/public/WebFontRendering.h:61 > + static SkPaint::Hinting getHinting(); > + static bool getAutoHint(); > + static bool getUseBitmaps(); > + static bool getAntiAlias(); > + static bool getSubpixelRendering(); > + static bool getSubpixelPositioning(); We normally don't prefix functions with 'get' in WebKit code.
Xianzhu Wang
Comment 10 2012-06-15 16:04:11 PDT
Created attachment 147918 [details] patch v3
Xianzhu Wang
Comment 11 2012-06-15 16:06:04 PDT
Comment on attachment 147913 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=147913&action=review >> Source/WebCore/ChangeLog:13 >> + (FontPlatformData): > > Please describe your changes here. Added description in the overview. >> Source/WebKit/chromium/ChangeLog:9 >> + * public/WebFontRendering.h: Copied from Source/WebKit/chromium/public/linuxish/WebFontRendering.h. > > Please provide an overview of what is happening. I would probably use the word refactor in the description. Done. Sorry for missing that. I should have added that in this patch. >> Source/WebKit/chromium/public/WebFontRendering.h:61 >> + static bool getSubpixelPositioning(); > > We normally don't prefix functions with 'get' in WebKit code. Done.
Tony Chang
Comment 12 2012-06-15 16:40:59 PDT
Comment on attachment 147918 [details] patch v3 Please verify that this works properly on Chromium Mac or Chromium Win before landing, or watch the bots when you land.
Xianzhu Wang
Comment 13 2012-06-15 16:47:18 PDT
Comment on attachment 147918 [details] patch v3 Thanks!
WebKit Review Bot
Comment 14 2012-06-15 17:24:26 PDT
Comment on attachment 147918 [details] patch v3 Clearing flags on attachment: 147918 Committed r120511: <http://trac.webkit.org/changeset/120511>
WebKit Review Bot
Comment 15 2012-06-15 17:24:32 PDT
All reviewed patches have been landed. Closing bug.
Xianzhu Wang
Comment 16 2012-06-15 17:36:39 PDT
Broke builder. Reverting change.
WebKit Review Bot
Comment 17 2012-06-15 17:41:05 PDT
Re-opened since this is blocked by 89255
Xianzhu Wang
Comment 18 2012-06-15 19:05:18 PDT
Created attachment 147938 [details] re-apply (fixed a include on Android)
WebKit Review Bot
Comment 19 2012-06-16 03:33:24 PDT
Comment on attachment 147938 [details] re-apply (fixed a include on Android) Rejecting attachment 147938 [details] from commit-queue. New failing tests: svg/custom/focus-ring.svg svg/W3C-SVG-1.1/types-basicDOM-01-b.svg svg/custom/group-opacity.svg svg/custom/createImageElement2.xhtml scrollbars/listbox-scrollbar-combinations.html svg/as-border-image/svg-as-border-image.html scrollbars/overflow-scrollbar-combinations.html svg/custom/createImageElement.svg Full output: http://queues.webkit.org/results/12961826
WebKit Review Bot
Comment 20 2012-06-16 03:33:29 PDT
Created attachment 147962 [details] Archive of layout-test-results from ec2-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Xianzhu Wang
Comment 21 2012-06-16 08:16:42 PDT
Comment on attachment 147938 [details] re-apply (fixed a include on Android) Layout test failures seem unrelated. Retry.
WebKit Review Bot
Comment 22 2012-06-16 16:29:10 PDT
Comment on attachment 147938 [details] re-apply (fixed a include on Android) Clearing flags on attachment: 147938 Committed r120536: <http://trac.webkit.org/changeset/120536>
WebKit Review Bot
Comment 23 2012-06-16 16:29:25 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 24 2012-06-16 17:58:51 PDT
Re-opened since this is blocked by 89296
Adam Barth
Comment 25 2012-06-16 18:02:34 PDT
third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:43:37:error: public/WebFontRendering.h: No such file or directory third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:46:error: 'WebKit' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp: In member function 'void WebCore::FontPlatformData::setupPaint(SkPaint*) const': third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:146:error: 'WebFontRendering' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:149:error: 'WebFontRendering' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:159:error: 'WebFontRendering' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:164:error: 'WebFontRendering' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:165:error: 'WebFontRendering' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:167:error: 'WebFontRendering' has not been declared third_party/WebKit/Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:168:error: 'WebFontRendering' has not been declared
Xianzhu Wang
Comment 29 2012-06-17 09:31:47 PDT
Despite of the reason of the failure, I think I made the things complex with bug 88802. The simpler way might be to use the target_conditions to include the original linux files on Android like that we discussed in chromium-dev. Now I want to revert the change in 88802 and use target_conditions. Adam, what's your opinion?
Xianzhu Wang
Comment 30 2012-06-17 09:40:58 PDT
(In reply to comment #29) > Despite of the reason of the failure, I think I made the things complex with bug 88802. The simpler way might be to use the target_conditions to include the original linux files on Android like that we discussed in chromium-dev. Now I want to revert the change in 88802 and use target_conditions. Adam, what's your opinion? And Tony? :)
Adam Barth
Comment 31 2012-06-17 10:14:28 PDT
> Adam, what's your opinion? Doesn't matter to me. Whatever makes other folks happy.
Xianzhu Wang
Comment 32 2012-06-17 14:45:33 PDT
Created attachment 148026 [details] Patch
Xianzhu Wang
Comment 33 2012-06-17 15:04:24 PDT
Comment on attachment 148026 [details] Patch Builds succeeded on chromium's android and linux_rel try bots.
WebKit Review Bot
Comment 34 2012-06-18 09:55:37 PDT
Comment on attachment 148026 [details] Patch Clearing flags on attachment: 148026 Committed r120603: <http://trac.webkit.org/changeset/120603>
WebKit Review Bot
Comment 35 2012-06-18 09:55:48 PDT
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.