My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 101156: Chrome shouldn't use WebKitSystemInterface
3 people starred this issue and may be notified of changes. Back to list
 
Project Member Reported by avi@chromium.org, Oct 21, 2011
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 .
Oct 21, 2011
#1 avi@chromium.org
(No comment was entered for this change.)
Cc: rsesek@chromium.org
Oct 21, 2011
#2 rsesek@chromium.org
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.
Oct 23, 2011
#3 jeremy@chromium.org
Looks like some of the functions we link against may be trivially removable and aren't related to Skia, e.g:

WKSetHTTPCookiesForURL
WKCopyCFURLResponseSuggestedFilename
Oct 23, 2011
#4 avi@chromium.org
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?
Oct 25, 2011
#6 avi@chromium.org
We aren't using many of these. We shouldn't be trying to link them in.
Oct 25, 2011
#7 avi@chromium.org
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.
Oct 25, 2011
#8 avi@chromium.org
(Note that this, when complete, will obsolete issue 28320 and  issue 48371 .
Apr 5, 2012
#9 kar...@google.com
(No comment was entered for this change.)
Status: Available
Nov 6, 2012
#10 abarth@chromium.org
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.
Cc: darin@chromium.org thakis@chromium.org
Labels: -Pri-2 Pri-1
Nov 6, 2012
#11 thakis@chromium.org
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.
Nov 6, 2012
#12 abarth@chromium.org
In that case, we need to reach some sort of understanding with Apple.  The current situation isn't tenable.
Nov 7, 2012
#13 mark@chromium.org
(No comment was entered for this change.)
Cc: esei...@chromium.org
Nov 7, 2012
#14 avi@chromium.org
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?)
Nov 7, 2012
#15 rsesek@chromium.org
(No comment was entered for this change.)
Status: Assigned
Owner: rsesek@chromium.org
Nov 8, 2012
#16 rsesek@chromium.org
(No comment was entered for this change.)
Labels: WebKit-ID-101634
Nov 8, 2012
#17 bugdro...@chromium.org
https://bugs.webkit.org/show_bug.cgi?id=101634
Labels: -WebKit-ID-101634 WebKit-ID-101634-ASSIGNED
Nov 8, 2012
#18 bugdro...@chromium.org
https://bugs.webkit.org/show_bug.cgi?id=101634
http://trac.webkit.org/changeset/134004
Labels: -WebKit-ID-101634-ASSIGNED WebKit-ID-101634-REOPENED WebKit-Rev-134004
Nov 13, 2012
#20 bugdro...@chromium.org
https://bugs.webkit.org/show_bug.cgi?id=101634
http://trac.webkit.org/changeset/134520
Labels: -WebKit-ID-101634-REOPENED WebKit-ID-101634-RESOLVED WebKit-Rev-134520
Mar 10, 2013
#21 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-WebKit -WebKit-Core Cr-Content Cr-Content-Core
Apr 5, 2013
#22 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Cr-Content Cr-Blink
Apr 21, 2013
#23 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
Apr 22, 2013
#24 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
Apr 26, 2013
#25 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
May 3, 2013
#27 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
May 3, 2013
#28 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
May 6, 2013
#29 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
May 6, 2013
#30 rsesek@chromium.org
(No comment was entered for this change.)
Status: Fixed
Sign in to add a comment

Powered by Google Project Hosting