Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 101156 Chrome shouldn't use WebKitSystemInterface
Starred by 3 users Project Member Reported by a...@chromium.org, Oct 21 2011 Back to list
Status: Fixed
Owner: rsesek@chromium.org
Closed: May 2013
Cc: a...@chromium.org, jeremy@chromium.org, rsesek@chromium.org, eseidel@chromium.org, pinkerton@chromium.org, jrg@chromium.org, dglazkov@chromium.org, darin@chromium.org, tha...@chromium.org, mark@chromium.org, mac-bugs-priority@chromium.org
Components:
OS: Mac
Pri: 1
Type: Bug


Sign in to add a comment
Right now, deleting WebKitSystemInterface causes 93 link errors. The list is attached. Some shouldn't be there, but others need to be cleaned up upstream.

See also issue 48371.
 
Comment 1 by a...@chromium.org, Oct 21 2011
Cc: rsesek@chromium.org
Comment 2 by rsesek@chromium.org, Oct 21 2011
Spoke with Avi about this. He thinks that switching to Skia should shave a lot of those errors off, so let's not act on this until we make that jump. After that, I'd maybe be interested in taking this on as a project.
Comment 3 by jeremy@chromium.org, Oct 23 2011
Looks like some of the functions we link against may be trivially removable and aren't related to Skia, e.g:

WKSetHTTPCookiesForURL
WKCopyCFURLResponseSuggestedFilename
Comment 4 by a...@chromium.org, Oct 23 2011
If they're trivially removable, why did they use the WKSI? What is the WebKit procedure for removing things?

Can you verify that the behavior is the same, either by test or by disassembly?
Comment 6 by a...@chromium.org, Oct 25 2011
We aren't using many of these. We shouldn't be trying to link them in.
Comment 7 by a...@chromium.org, Oct 25 2011
Why are we linking in functions we don't use?

Inside the WebKit source are a whole bunch of function pointers, wkFoo. There is a function InitWebCoreSystemInterface that links them up to the corresponding functions WKFoo. InitWebCoreSystemInterface links up against a ton of symbols, just to put them into function pointers that we might or might not use.

So a much better test of what we're using from WebKitSystemInterface isn't to delete that library, but to go into Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm and comment out the contents of InitWebCoreSystemInterface. Then comment out the contents of Source/WebCore/platform/mac/WebCoreSystemInterface.mm. Attempt to build :)

The _real_ list of functions we use from WebKitSystemInterface are:

wkCGContextGetShouldSmoothFonts
wkCGPatternCreateWithImageAndTransform
wkCreateCTLineWithUniCharProvider
wkDrawBezeledTextArea
wkDrawBezeledTextFieldCell
wkDrawCapsLockIndicator
wkDrawFocusRing
wkDrawMediaSliderTrack
wkDrawMediaUIPart
wkGetFontInLanguageForCharacter
wkGetFontInLanguageForRange
wkGetGlyphsForCharacters
wkGetGlyphTransformedAdvances
wkGetUserToBaseCTM
wkGetVerticalGlyphsForCharacters
wkMeasureMediaUIPart
wkSetBaseCTM
wkSetCGFontRenderingMode
wkSetPatternPhaseInUserSpace
wkSetUpFontCache

Adjusting the spreadsheet to match.
Comment 8 by a...@chromium.org, Oct 25 2011
(Note that this, when complete, will obsolete issue 28320 and issue 48371.
Comment 9 by kareng@google.com, Apr 5 2012
Status: Available
Cc: darin@chromium.org tha...@chromium.org
Labels: -Pri-2 Pri-1
Apple has made it clear that they will break the Chromium build with impunity because of this dependency.  We need someone to drive removing this dependency to completion.
See also issue 101387. rsesek knows what to do here.

I'm not sure if we can completely remove our dependency on this library, some of the functions we need are only exposed through that library.
In that case, we need to reach some sort of understanding with Apple.  The current situation isn't tenable.
Comment 13 by mark@chromium.org, Nov 7 2012
Cc: eseidel@chromium.org
Comment 14 by a...@chromium.org, Nov 7 2012
I re-checked our dependencies as in comment 7; here's the new list:

wkAdvanceDefaultButtonPulseAnimation
wkCreateCTLineWithUniCharProvider
wkDrawBezeledTextArea
wkDrawBezeledTextFieldCell
wkDrawCapsLockIndicator
wkDrawMediaSliderTrack
wkDrawMediaUIPart
wkGetFontInLanguageForCharacter
wkGetFontInLanguageForRange
wkGetGlyphsForCharacters
wkGetGlyphTransformedAdvances
wkGetVerticalGlyphsForCharacters
wkMeasureMediaUIPart
wkSetUpFontCache

There seems to be two main parts to that list. One is CoreText; now that we're using Skia, why are we linking in these CT calls?

The other part is RenderThemeMac. That RenderTheme uses several SPIs. Why? The main argument I have with Apple here is that they say that the API that exists is sufficient to do native-look rendering. Well then, why the heck don't they use them?! Rather than write our own RenderTheme that uses API, we should rewrite RenderThemeMac.

Ugh. I wish Apple would stop adding more and more SPI use.

(BTW, some of them are clearly not used by us, like the media ui. We do our own media theme drawing; can we avoid linking in their code?)
Owner: rsesek@chromium.org
Status: Assigned
Labels: WebKit-ID-101634
Project Member Comment 17 by bugdroid1@chromium.org, Nov 8 2012
Labels: -WebKit-ID-101634 WebKit-ID-101634-ASSIGNED
https://bugs.webkit.org/show_bug.cgi?id=101634
Project Member Comment 18 by bugdroid1@chromium.org, Nov 9 2012
Labels: -WebKit-ID-101634-ASSIGNED WebKit-ID-101634-REOPENED WebKit-Rev-134004
https://bugs.webkit.org/show_bug.cgi?id=101634
http://trac.webkit.org/changeset/134004
Project Member Comment 19 by bugdroid1@chromium.org, Nov 9 2012
Labels: WebKit-Rev-134016
http://trac.webkit.org/changeset/134016
Project Member Comment 20 by bugdroid1@chromium.org, Nov 14 2012
Labels: -WebKit-ID-101634-REOPENED WebKit-ID-101634-RESOLVED WebKit-Rev-134520
https://bugs.webkit.org/show_bug.cgi?id=101634
http://trac.webkit.org/changeset/134520
Project Member Comment 21 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-WebKit -WebKit-Core Cr-Content Cr-Content-Core
Project Member Comment 22 by bugdroid1@chromium.org, Apr 6 2013
Labels: -Cr-Content Cr-Blink
Project Member Comment 23 by bugdroid1@chromium.org, Apr 22 2013
------------------------------------------------------------------------
r148796 | rsesek@chromium.org | 2013-04-22T00:05:08.566824Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebSystemInterface.mm?r1=148796&r2=148795&pathrev=148796
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.h?r1=148796&r2=148795&pathrev=148796
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.mm?r1=148796&r2=148795&pathrev=148796

Trim the WebKitSystemInterface list to just what's used in Blink.

BUG= 101156 

Review URL: https://chromiumcodereview.appspot.com/14378002
------------------------------------------------------------------------
Project Member Comment 24 by bugdroid1@chromium.org, Apr 22 2013
------------------------------------------------------------------------
r148861 | rsesek@chromium.org | 2013-04-22T22:10:02.745025Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.h?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/FontCacheMac.mm?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/SimpleFontDataMac.mm?r1=148861&r2=148860&pathrev=148861
   D http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.mm?r1=148861&r2=148860&pathrev=148861
   D http://src.chromium.org/viewvc/blink/trunk/Tools/Scripts/copy-webkitlibraries-to-product-directory?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/ComplexTextControllerCoreText.mm?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gyp/core.gyp?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/GlyphPageTreeNodeMac.cpp?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/WebKit/chromium/src/WebKit.cpp?r1=148861&r2=148860&pathrev=148861
   D http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebSystemInterface.h?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gypi?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/ThemeMac.mm?r1=148861&r2=148860&pathrev=148861
   D http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebSystemInterface.mm?r1=148861&r2=148860&pathrev=148861
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderThemeMacShared.mm?r1=148861&r2=148860&pathrev=148861

Remove the dynamic initialization of WebKitSystemInterface. Just call into the library directly.

BUG= 101156 

Review URL: https://codereview.chromium.org/14325012
------------------------------------------------------------------------
Project Member Comment 25 by bugdroid1@chromium.org, Apr 26 2013
------------------------------------------------------------------------
r149201 | rsesek@chromium.org | 2013-04-26T14:43:12.422031Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderTheme.h?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.h?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderButton.cpp?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderThemeChromiumDefault.cpp?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSValueKeywords.in?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValueMappings.h?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderThemeChromiumMac.mm?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderButton.h?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/ThemeMac.mm?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderTheme.cpp?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/ThemeTypes.h?r1=149201&r2=149200&pathrev=149201
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderThemeChromiumWin.cpp?r1=149201&r2=149200&pathrev=149201

Remove -webkit-appearance:default-button. This was only used in Safari.

This was added to WebKit at r32881 and r35950. When ApplicationChromeMode was
removed in Blink r148011 this became dead code. Also removes a call into
WebKitSystemInterface.

BUG= 101156 

Review URL: https://codereview.chromium.org/14413014
------------------------------------------------------------------------
Project Member Comment 27 by bugdroid1@chromium.org, May 3 2013
------------------------------------------------------------------------
r149652 | rsesek@chromium.org | 2013-05-03T15:10:05.062784Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.h?r1=149652&r2=149651&pathrev=149652
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderThemeChromiumMac.mm?r1=149652&r2=149651&pathrev=149652

Draw the caps lock indicator manually, rather than using WKDrawCapsLockIndicator.

BUG= 101156 
R=thakis@chromium.org

Review URL: https://codereview.chromium.org/14865002
------------------------------------------------------------------------
Project Member Comment 28 by bugdroid1@chromium.org, May 3 2013
------------------------------------------------------------------------
r149659 | rsesek@chromium.org | 2013-05-03T18:29:07.283502Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/ScrollbarThemeMac.mm?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/ScrollAnimatorMac.mm?r1=149659&r2=149658&pathrev=149659
   D http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/WebCoreSystemInterface.h?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/FontCacheMac.mm?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/SimpleFontDataMac.mm?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/mac/ScrollElasticityController.mm?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/cocoa/FontPlatformDataCocoa.mm?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/ComplexTextControllerCoreText.mm?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gyp/core.gyp?r1=149659&r2=149658&pathrev=149659
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/graphics/mac/GlyphPageTreeNodeMac.cpp?r1=149659&r2=149658&pathrev=149659
   D http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gyp/mac/adjust_visibility.sh?r1=149659&r2=149658&pathrev=149659

Finish removing WebKitSystemInterface from Blink.

Replace some more WebKitSystemInterface wrappers with direct calls to SPI. This
also removes the library from the link list and deletes the now-unneeded
adjust_visibility.sh script.

This is a partial revert of http://trac.webkit.org/changeset/9311, which
many years ago replaced these SPI calls with the WKSI ones.

This inlines a the call to WKGetVerticalGlyphsForCharacters(), which was a
noop that just returned false.

BUG= 101156 
R=thakis@chromium.org

Review URL: https://codereview.chromium.org/14856004
------------------------------------------------------------------------
Project Member Comment 29 by bugdroid1@chromium.org, May 6 2013
------------------------------------------------------------------------
r198497 | rsesek@chromium.org | 2013-05-06T19:06:21.363257Z

Changed paths:
   D http://src.chromium.org/viewvc/chrome/trunk/src/third_party/apple_webkit?r1=198497&r2=198496&pathrev=198497

Delete third_party/apple_webkit/ since WebKitSystemInterface is no longer needed.

BUG= 101156 
TBR=thakis@chromium.org

Review URL: https://codereview.chromium.org/14663006
------------------------------------------------------------------------
Status: Fixed
Sign in to add a comment