Bug 39672 – Make sure skia is not Chromium specific

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

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.

[David Levin]

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.

[Kwang Yul Seo]

(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.

[Eric Seidel (no email)]

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.

[Kwang Yul Seo]

(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.

[Patrick R. Gansterer]

Created attachment 62408 [details]
Patch (add typedef for canvas)

This is only a part of attachment 57011 [details].

[Eric Seidel (no email)]

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.

[Stephen White]

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.

[Kwang Yul Seo]

(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.

[Stephen White]

(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.

[Adam Barth]

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.

[Kwang Yul Seo]

Created attachment 66669 [details]
Move FontCustomPlatformData to platform/skia

Move FontCustomPlatformData to platform/skia. There is no chromium specific code in FontCustomPlatformData.

[Kwang Yul Seo]

> 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.

[James Robinson]

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.

[Kwang Yul Seo]

Created attachment 66841 [details]
Move FontCustomPlatformData to platform/skia

The same patch, but used 'svn mv' this time as James suggested.

[Kwang Yul Seo]

Created attachment 67055 [details]
Move Image::loadPlatformResource to ImageChromium.cpp

Move Chromium-specific Image::loadPlatformResource out of ImageSkia.cpp to ImageChromium.cpp.

[WebKit Review Bot]

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.

[Kwang Yul Seo]

Created attachment 67056 [details]
Move Image::loadPlatformResource to ImageChromium.cpp

Fix style errors.

[James Robinson]

Comment on attachment 66841 [details]
Move FontCustomPlatformData to platform/skia

Looks good!  Thanks.

[James Robinson]

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.

[Kwang Yul Seo]

(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.

[WebKit Commit Bot]

Comment on attachment 66841 [details]
Move FontCustomPlatformData to platform/skia

Clearing flags on attachment: 66841

Committed r67109: <http://trac.webkit.org/changeset/67109>

[James Robinson]

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

[WebKit Commit Bot]

Comment on attachment 67056 [details]
Move Image::loadPlatformResource to ImageChromium.cpp

Clearing flags on attachment: 67056

Committed r67187: <http://trac.webkit.org/changeset/67187>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kwang Yul Seo]

Reopen the bug as I have more patches.

[Kwang Yul Seo]

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.

[James Robinson]

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?

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

Comment on attachment 67199 [details]
Move FontCacheLinux.cpp to platform/graphics/skia/FontCacheSkia.cpp

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

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?

[James Robinson]

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.

[James Robinson]

Comment on attachment 68133 [details]
Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp

R=me, cr-linux is happy.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67918 might have broken Chromium Win Release

[James Robinson]

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 ==========

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

BTW, why is there no status bubble for cr-win?

[Kwang Yul Seo]

Reopen the bug

[Kwang Yul Seo]

Created attachment 71180 [details]
Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp

Second try! I built it on Chromium Win and Linux.

[Eric Seidel (no email)]

(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.

[James Robinson]

Comment on attachment 71180 [details]
Move GlyphPageTreeNodeLinux.cpp to platform/graphics/skia/GlyphPageTreeNodeSkia.cpp

Looks good.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kwang Yul Seo]

We open the bug as I have more patches.

[Kwang Yul Seo]

Created attachment 71787 [details]
Move SimpleFontDataLinux.cpp to platform/graphics/skia/SimpleFontDataSkia.cpp

[Adam Barth]

Please new bugs for new patches.

[Kwang Yul Seo]

(In reply to comment #49)
> Please new bugs for new patches.

Oops. Okay.