Bug 66220 – [chromium] Update WebSettings to support per-script font settings

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Matt Falkenhagen]

Update Chromium WebSettings to support the new per-script font settings (see bug 20797).

[Matt Falkenhagen]

Created attachment 103907 [details]
Patch

[Matt Falkenhagen]

Created attachment 103908 [details]
updated patch

[Matt Falkenhagen]

Tony, can you review this small patch please?

[Tony Chang]

Comment on attachment 103908 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103908&action=review

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:59
> +    m_settings->setStandardFontFamily((String)font, script);

Doesn't the conversion from WebString to String happen implicitly?  If not, we should be using static_cast rather than the c-style cast.

[Matt Falkenhagen]

Created attachment 104643 [details]
removed explicit casts

[WebKit Review Bot]

Comment on attachment 104643 [details]
removed explicit casts

Clearing flags on attachment: 104643

Committed r93575: <http://trac.webkit.org/changeset/93575>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This is not compiling on the canaries on any platform.  Build logs:

windows: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/12753/steps/compile/logs/stdio
mac: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder%20%28CG%29/builds/207/steps/compile/logs/stdio
linux: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29%281%29/builds/9654/steps/compile/logs/stdio

[Matt Falkenhagen]

The build failure was caused by including ICU headers needed for the UScriptCode enum used to identify scripts.

I consulted a little with Kent Tamura and he brought up that it might be bad to rely on ICU types in WebSettings.  Perhaps we can use a new type to hide UScriptCode (like WebString does for strings), or just use int.

Darin, what do you think?

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> The build failure was caused by including ICU headers needed for the UScriptCode enum used to identify scripts.
> 
> I consulted a little with Kent Tamura and he brought up that it might be bad to rely on ICU types in WebSettings.  Perhaps we can use a new type to hide UScriptCode (like WebString does for strings), or just use int.
> 
> Darin, what do you think?

Have you tried adding '<(chromium_src_dir)/third_party/icu/icu.gyp:icuuc' to the export_dependent_settings section of the webkit target?

[Matt Falkenhagen]

Created attachment 104817 [details]
added ICU dependency

[Matt Falkenhagen]

Thanks, I've added the dependency in the latest patch and now it seems to be compiling on the trybots (previously I just tested locally).

I'm also not sure whether the #if WEBKIT_IMPLEMENTATION part was needed (I originally was following similar header files in this directory). I've removed it in the latest patch.

[Tony Chang]

Comment on attachment 104817 [details]
added ICU dependency

This built for me with ToT chrome and the ews bots passed, so let's try this again.

[WebKit Review Bot]

Comment on attachment 104817 [details]
added ICU dependency

Clearing flags on attachment: 104817

Committed r93618: <http://trac.webkit.org/changeset/93618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.