Bug 50641 – ApplyStyleCommand::applyRelativeFontStyleChange should take EditingStyle*

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This is a part of the effort to deploy EditingStyle in ApplyStyleCommand.

[Ryosuke Niwa]

Created attachment 75836 [details]
refactoring

[Ryosuke Niwa]

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

[Darin Adler]

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

[Ryosuke Niwa]

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

[Darin Adler]

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

[Build Bot]

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

[Ryosuke Niwa]

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

[WebKit Review Bot]

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.

[Ryosuke Niwa]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 76800 [details]
Removed #include changes

[Ryosuke Niwa]

Could someone review my patch?

[Eric Seidel (no email)]

Comment on attachment 76800 [details]
Removed #include changes

LGTM. Thanks.

[Ryosuke Niwa]

(In reply to comment #15)
> (From update of attachment 76800 [details])
> LGTM. Thanks.

Thanks for the review, Eric.  Landing it now.

[Ryosuke Niwa]

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