platform/graphics/skia is hardcoded with Chromimum specific code. Add PLATFORM(CHROMIUM) guard for skia/ext (PlatformCanvas and resampling) so other ports can use skia. This is a bit ugly.
Created attachment 57011 [details] Patch Don't use skia/ext/platform_canvas.h and skia/ext/image_operations.h which are only available in Chromium source tree. This patch is for BREW MP right now.
I'm pretty sure this isn't the way to do re-use these files but I'm cc'ing someone who should be more familiar with this area than I.
(In reply to comment #2) > I'm pretty sure this isn't the way to do re-use these files but I'm cc'ing someone who should be more familiar with this area than I. Thank you. I need to someone to discuss this issue.
Comment on attachment 57011 [details] Patch This approach feels wrong to me too. r- based on above comments. You'll need to track someone down over in #chromium or on chromium-dev.
(In reply to comment #4) > (From update of attachment 57011 [details]) > This approach feels wrong to me too. r- based on above comments. You'll need to track someone down over in #chromium or on chromium-dev. Okay. I will try to find somebody to discuss this issue on #chromium.
Created attachment 62408 [details] Patch (add typedef for canvas) This is only a part of attachment 57011 [details].
Comment on attachment 62408 [details] Patch (add typedef for canvas) WebCore/platform/graphics/skia/PlatformContextSkia.cpp:594 + return m_canvas->getTopPlatformDevice().IsVectorial(); Why is this Chromium only? WebCore/platform/graphics/skia/PlatformContextSkia.h:48 + typedef SkCanvas PlatformContextCanvasType; Is skia not normally namespaced? Looks OK, I think.
Not sure what the status of this patch is, but it looks like an ok workaround for now. Ideally, we'd upstream all the stuff from the chromium repo (chromium/src/skia/ext) into Skia proper, then everyone could use it. Let me know if you're interested in working on that, and I can help you land it in Skia.
(In reply to comment #8) > Not sure what the status of this patch is, but it looks like an ok workaround for now. > > Ideally, we'd upstream all the stuff from the chromium repo (chromium/src/skia/ext) into Skia proper, then everyone could use it. Let me know if you're interested in working on that, and I can help you land it in Skia. I think it is a good idea to upstream Chromium's chromium/src/skia/ext to Skia repo so that other WebKit ports can use it. Please help me do the job.
(In reply to comment #9) > (In reply to comment #8) > > Not sure what the status of this patch is, but it looks like an ok workaround for now. > > > > Ideally, we'd upstream all the stuff from the chromium repo (chromium/src/skia/ext) into Skia proper, then everyone could use it. Let me know if you're interested in working on that, and I can help you land it in Skia. > > I think it is a good idea to upstream Chromium's chromium/src/skia/ext to Skia repo so that other WebKit ports can use it. Please help me do the job. After looking at it a bit, it seems as if this is not quite as clean as I thought. It seems that skia/ext depends on some things in chromium/base. So the first job would be to get rid of those dependencies. This will require some knowledge of the chromium project, and some ideas of how to replace those dependencies (I don't have all the answers here). After that, it would be a matter of reformatting the code to be more skia-friendly (getting rid of chrome-specific relative paths, for example), and then creating a patch for Skia to add the content of the (new) /ext directory, and uploading it to codereview.appspot.com for review.
Comment on attachment 62408 [details] Patch (add typedef for canvas) I'm not an expert here, but this doesn't look like the right solution. Moving skia/ext out of the chromium repo is probably better than these ifdefs.
Created attachment 66669 [details] Move FontCustomPlatformData to platform/skia Move FontCustomPlatformData to platform/skia. There is no chromium specific code in FontCustomPlatformData.
> After looking at it a bit, it seems as if this is not quite as clean as I thought. It seems that skia/ext depends on some things in chromium/base. So the first job would be to get rid of those dependencies. This will require some knowledge of the chromium project, and some ideas of how to replace those dependencies (I don't have all the answers here). > > After that, it would be a matter of reformatting the code to be more skia-friendly (getting rid of chrome-specific relative paths, for example), and then creating a patch for Skia to add the content of the (new) /ext directory, and uploading it to codereview.appspot.com for review. I will build Chromium and will do the job as you suggested. There are other patches not related to skia/ext such as moving FontCustomPlatformData to platform/skia. I will submit those patches first.
Comment on attachment 66669 [details] Move FontCustomPlatformData to platform/skia This looks great, but could you do an "svn mv" instead of a remove/add pair so that metadata is preserved for these files? Thanks.
Created attachment 66841 [details] Move FontCustomPlatformData to platform/skia The same patch, but used 'svn mv' this time as James suggested.
Created attachment 67055 [details] Move Image::loadPlatformResource to ImageChromium.cpp Move Chromium-specific Image::loadPlatformResource out of ImageSkia.cpp to ImageChromium.cpp.
Attachment 67055 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/WebCore.gyp/WebCore.gyp:1230: Line contains tab character. [whitespace/tab] [5] WebCore/platform/graphics/chromium/ImageChromium.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/chromium/ImageChromium.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/chromium/ImageChromium.cpp:45: One space before end of line comments [whitespace/comments] [5] Total errors found: 4 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 67056 [details] Move Image::loadPlatformResource to ImageChromium.cpp Fix style errors.
Comment on attachment 66841 [details] Move FontCustomPlatformData to platform/skia Looks good! Thanks.
The loadPlatformResource patch also looks all right, but I'll let the EWS bot compile check it first. Have you compile checked this on Chromium mac? I can check it myself if you don't have the hardware available if you give me a day or two.
(In reply to comment #20) > The loadPlatformResource patch also looks all right, but I'll let the EWS bot compile check it first. Have you compile checked this on Chromium mac? I can check it myself if you don't have the hardware available if you give me a day or two. I checked it on Chromium Linux, but not on Mac.
Comment on attachment 66841 [details] Move FontCustomPlatformData to platform/skia Clearing flags on attachment: 66841 Committed r67109: <http://trac.webkit.org/changeset/67109>
Comment on attachment 67056 [details] Move Image::loadPlatformResource to ImageChromium.cpp Compile tested on chromium mac, seems to work fine here as well. R=me
Comment on attachment 67056 [details] Move Image::loadPlatformResource to ImageChromium.cpp Clearing flags on attachment: 67056 Committed r67187: <http://trac.webkit.org/changeset/67187>
All reviewed patches have been landed. Closing bug.
Reopen the bug as I have more patches.
Created attachment 67199 [details] Move FontCacheLinux.cpp to platform/graphics/skia/FontCacheSkia.cpp FontCacheLinux is not Linux specific implementation of FontCache. Move it to platform/graphics/skia and rename it to FontCacheSkia.cpp.
Comment on attachment 67199 [details] Move FontCacheLinux.cpp to platform/graphics/skia/FontCacheSkia.cpp View in context: https://bugs.webkit.org/attachment.cgi?id=67199&action=prettypatch I'm not so sure about this one - the file still is clearly Chromium specific (it directly uses ChromiumBridge for one) and isn't really more skia specific than linux specific. Moving a chromium-specific file from /chromium to /skia seems like it would be a regression for your purposes, no?
(In reply to comment #28) > (From update of attachment 67199 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67199&action=prettypatch > > I'm not so sure about this one - the file still is clearly Chromium specific (it directly uses ChromiumBridge for one) and isn't really more skia specific than linux specific. Moving a chromium-specific file from /chromium to /skia seems like it would be a regression for your purposes, no? Yes, you are right. FontCache::getFontDataForCharacters replies on ChromiumBridge. In Brew MP, I changed the method: const SimpleFontData* FontCache::getFontDataForCharacters(const Font& font, const UChar* characters, int length) { return font.primaryFont(); } Except for this method, there is no chromium specific code. I will submit the patch again after separating Chromium specific code. Thanks for your review.
Comment on attachment 67199 [details] Move FontCacheLinux.cpp to platform/graphics/skia/FontCacheSkia.cpp
Created attachment 68133 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp GlyphPageTreeNodeLinux does not depend on Linux or Chromium. Move GGlyphPageTreeNodeLinux to platform/graphics/skia and rename it to GlyphPageTreeNodeSkia.cpp so that other ports can use it.
My idea is to move FontCacheLinux.cpp, GlyphPageTreeNodeLinux.cpp, SimpleFontDataLinux.cpp to skia because they are not Linux-specific nor Chromium-specific. I successfully compiled those files in Brew MP without modification. FontCache::getFontDataForCharacters was the only exception because it uses ChromiumBridge::getFontFamilyForCharacters. What's your opinion, James? Is this a good direction or do you want me to duplicate the code for Brew MP?
Comment on attachment 68133 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp Looks pretty good. I'll let the cr-linux bot compile check this before touching the review? flag. Overall I prefer this direction to duplicating code. Just keep in mind that until the BREWMP port gets a buildbot on build.webkit.org, any changes to common files like this may break your build in a way that the patch contributor will probably not notice.
Comment on attachment 68133 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp R=me, cr-linux is happy.
Comment on attachment 68133 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp Clearing flags on attachment: 68133 Committed r67918: <http://trac.webkit.org/changeset/67918>
http://trac.webkit.org/changeset/67918 might have broken Chromium Win Release
Reverting..that's not a very good start. Compiler output: 3>------ Build started: Project: webcore_platform, Configuration: Release Win32 ------ 3>Compiling... 3>GlyphPageTreeNodeSkia.cpp 3>..\platform\graphics\skia\GlyphPageTreeNodeSkia.cpp(51) : error C2039: 'setupPaint' : is not a member of 'WebCore::FontPlatformData' 3> c:\webkitbuildslave\chromium-win-release\build\webcore\platform\graphics\chromium\FontPlatformDataChromiumWin.h(50) : see declaration of 'WebCore::FontPlatformData' 3>Build log was saved at "file://C:\WebKitBuildSlave\chromium-win-release\build\WebKit\chromium\Release\obj\webcore_platform\BuildLog.htm" 3>webcore_platform - 1 error(s), 0 warning(s) ========== Build: 2 succeeded, 1 failed, 80 up-to-date, 0 skipped ==========
(In reply to comment #38) > Reverting..that's not a very good start. > > Compiler output: > > 3>------ Build started: Project: webcore_platform, Configuration: Release Win32 ------ > 3>Compiling... > 3>GlyphPageTreeNodeSkia.cpp > 3>..\platform\graphics\skia\GlyphPageTreeNodeSkia.cpp(51) : error C2039: 'setupPaint' : is not a member of 'WebCore::FontPlatformData' > 3> c:\webkitbuildslave\chromium-win-release\build\webcore\platform\graphics\chromium\FontPlatformDataChromiumWin.h(50) : see declaration of 'WebCore::FontPlatformData' > 3>Build log was saved at "file://C:\WebKitBuildSlave\chromium-win-release\build\WebKit\chromium\Release\obj\webcore_platform\BuildLog.htm" > 3>webcore_platform - 1 error(s), 0 warning(s) > ========== Build: 2 succeeded, 1 failed, 80 up-to-date, 0 skipped ========== It seems "['exclude', 'platform/graphics/skia/GlyphPageTreeNodeSkia\\.cpp$']" does not exclude the file. I will test the Chromium Win too before submitting the patch. Sorry for the inconvenience.
BTW, why is there no status bubble for cr-win?
Reopen the bug
Created attachment 71180 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp Second try! I built it on Chromium Win and Linux.
(In reply to comment #40) > BTW, why is there no status bubble for cr-win? Setting up a chromium build with visual studio express is nearly impossible. But if someone is interested in trying, there are instructions and I can get you access to an EC2 instance with windows on it.
Comment on attachment 71180 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp Looks good.
Comment on attachment 71180 [details] Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp Clearing flags on attachment: 71180 Committed r70287: <http://trac.webkit.org/changeset/70287>
We open the bug as I have more patches.
Created attachment 71787 [details] Move SimpleFontDataLinux.cpp to platform/graphics/skia/SimpleFontDataSkia.cpp
Please new bugs for new patches.
(In reply to comment #49) > Please new bugs for new patches. Oops. Okay.