Bug 50641

Created attachment 75836 [details] refactoring

(In reply to comment #1) > Created an attachment (id=75836) [details] > refactoring Once this patch is in, we can get rid of -webkit-font-size-delta entirely in the next step.

(In reply to comment #2) > Once this patch is in, we can get rid of -webkit-font-size-delta entirely in the next step. How will you determine if Mac OS X WebKit clients are using -webkit-font-size-delta with the -[WbeView applyStyle:] method?

(In reply to comment #3) > (In reply to comment #2) > > Once this patch is in, we can get rid of -webkit-font-size-delta entirely in the next step. > > How will you determine if Mac OS X WebKit clients are using -webkit-font-size-delta with the -[WbeView applyStyle:] method? Mn... that's a good point. But we've never exposed font-size-delta to the Web. Unless people have looked at how we implemented executeFontSizeDelta, they will never know that property. Also, CSSParser doesn't parse -webkit-font-size-delta, so they must be using CSSWebKitFontSizeDelta directly with px CSS primitive value. Should we really worry about people digging into WebCore that deep? I feel like any other changes we make to WebCore will cause a similar compatibility problems.

(In reply to comment #4) > But we've never exposed font-size-delta to the Web. True, but not completely relevant. This is about applications making direct use of WebKit, not the web. > Also, CSSParser doesn't parse -webkit-font-size-delta, so they must be using CSSWebKitFontSizeDelta directly with px CSS primitive value. CSSParser is not relevant. To use -[WebView applyStyle:], programmers use the Objective-C DOM API to create a CSSStyleDeclaration object, not the CSS parser. > Unless people have looked at how we implemented executeFontSizeDelta, they will never know that property. You may be overlooking some Mac-specific things. This is also used to make text smaller in response to the Smaller menu item in the Style submenu in the Format menu in the Mail application, for example, but not by using -[WebView applyStyle:] directly. > Should we really worry about people digging into WebCore that deep? You may be right. We can probably cross our fingers and hope nobody does this. It’s not really about “digging deep” into WebCore, but it’s true that discovering this does require knowing about an internal CSS property name. > I feel like any other changes we make to WebCore will cause a similar compatibility problems. I don’t agree. Most other changes don’t remove a publicly accessible feature as this one does.

Attachment 75836 [details] did not build on win: Build output: http://queues.webkit.org/results/6824084

(In reply to comment #5) > (In reply to comment #4) > > Unless people have looked at how we implemented executeFontSizeDelta, they will never know that property. > > You may be overlooking some Mac-specific things. This is also used to make text smaller in response to the Smaller menu item in the Style submenu in the Format menu in the Mail application, for example, but not by using -[WebView applyStyle:] directly. Oh, I didn't know that. I was wondering what kind of web pages / App uses this feature. > > Should we really worry about people digging into WebCore that deep? > > You may be right. We can probably cross our fingers and hope nobody does this. It’s not really about “digging deep” into WebCore, but it’s true that discovering this does require knowing about an internal CSS property name. We could leave it there if you want. I wanted to get rid of it in order to avoid web apps start depending on this feature in the future because I really think this is an implementation detail that we shouldn't have exposed in the first place. If there's a way to prevent new apps from using it without breaking the backward compatibility, I'm more than happy to do it. IMHO moving this property into EditingStyle is a good idea regardless of whether we're getting rid of this property or not because I want to isolate all editing code that touches CSSMutableStyleDeclaration into EditingStyle eventually.

Attachment 75836 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 76709 [details] Don't initialize float in class declaration

Attachment 76709 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7091051

Attachment 76709 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7110057

(In reply to comment #11) > Attachment 76709 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/7110057 I'm going to separate the #include change.

Created attachment 76800 [details] Removed #include changes

Could someone review my patch?

Comment on attachment 76800 [details] Removed #include changes LGTM. Thanks.

(In reply to comment #15) > (From update of attachment 76800 [details]) > LGTM. Thanks. Thanks for the review, Eric. Landing it now.

Committed r75080: <http://trac.webkit.org/changeset/75080>