Bug 141777 – [WK2] Add support for font panel on OS X

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

We need to add the necessary hooks into WKWebView to interact with the font panel.

[Enrica Casucci]

Created attachment 246859 [details]
Patch

[Tim Horton]

Comment on attachment 246859 [details]
Patch

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

> Source/WebCore/editing/mac/EditorMac.mm:689
> +    style->setProperty(CSSPropertyFontStyle, (fontTraits & NSFontItalicTrait) ? String("italic") : String("normal"));
> +    style->setProperty(CSSPropertyFontWeight, (fontTraits & NSFontBoldTrait) ? String("bold") : String("normal"));

I feel like there's a better way to make these Strings from literals.

> Source/WebKit2/ChangeLog:52
> [phone]  Enrica Casucci  <[e-mail]>

Duplicate changelog!

[Build Bot]

Comment on attachment 246859 [details]
Patch

Attachment 246859 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6609615424847872

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 246863 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

Something bad happening here:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000040

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x000000010eb3939a WebKit::WebPage::platformEditorState(WebCore::Frame&, WebKit::EditorState&) const + 42 (FontPlatformData.h:127)
1   com.apple.WebKit              	0x000000010eb29075 WebKit::WebPage::editorState() const + 219 (WebPage.cpp:768)
2   com.apple.WebKit              	0x000000010eb2fe6d WebKit::WebPage::didChangeSelection() + 29 (tuple:263)
3   com.apple.WebCore             	0x000000011014638d WebCore::Editor::respondToChangedSelection(WebCore::VisibleSelection const&, unsigned int) + 45 (Editor.h:523)
4   com.apple.WebCore             	0x00000001101f2c76 WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 1014 (FrameSelection.cpp:318)
5   com.apple.WebCore             	0x00000001101f17c8 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 24 (FrameSelection.cpp:325)
6   com.apple.WebCore             	0x00000001101f4a6e WebCore::FrameSelection::textWasReplaced(WebCore::CharacterData*, unsigned int, unsigned int, unsigned int) + 1390 (VisibleSelection.h:39)
7   com.apple.WebCore             	0x000000010ff74f88 WebCore::CharacterData::setDataAndUpdate(WTF::String const&, unsigned int, unsigned int, unsigned int) + 168 (Node.h:410)
8   com.apple.WebCore             	0x000000010ff7540e WebCore::CharacterData::deleteData(unsigned int, unsigned int, int&) + 142 (Node.h:410)
9   com.apple.WebCore             	0x000000011008354a WebCore::DeleteFromTextNodeCommand::doApply() + 218 (DeleteFromTextNodeCommand.cpp:64)
10  com.apple.WebCore             	0x000000010ff8b13b

[Enrica Casucci]

Created attachment 246912 [details]
Patch2

Fixes build failures and crashing tests.

[Enrica Casucci]

(In reply to comment #2)
> Comment on attachment 246859 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246859&action=review
> 
> > Source/WebCore/editing/mac/EditorMac.mm:689
> > +    style->setProperty(CSSPropertyFontStyle, (fontTraits & NSFontItalicTrait) ? String("italic") : String("normal"));
> > +    style->setProperty(CSSPropertyFontWeight, (fontTraits & NSFontBoldTrait) ? String("bold") : String("normal"));
> 
> I feel like there's a better way to make these Strings from literals.
> 
> > Source/WebKit2/ChangeLog:52
> > [phone]  Enrica Casucci  <[e-mail]>
> 
> Duplicate changelog!

Actually, I spoke with Chris Dumez and he showed me a better way to do this. I'll post a new patch shortly.

[Enrica Casucci]

Created attachment 246921 [details]
Patch3

Including suggestions from Chris Dumez on how to set the CSSS properties in EditorMac.

[WebKit Commit Bot]

Attachment 246921 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/mac/EditorMac.mm:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 246921 [details]
Patch3

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

> Source/WebCore/editing/mac/EditorMac.mm:688
> +    style->setProperty(CSSPropertyFontFamily, fontFamily);

nit: Could be cssValuePool().createFontFamilyValue(fontFamily) to avoid calling the CSSParser.

> Source/WebCore/editing/mac/EditorMac.mm:691
> +    style->setProperty(CSSPropertyFontSize, CSSPrimitiveValue::create(fontSize, CSSPrimitiveValue::CSS_PX));

nit: Could leverage the CSSValuePool too: cssValuePool().createValue(fontSize, CSSPrimitiveValue::CSS_PX)

[Chris Dumez]

Comment on attachment 246921 [details]
Patch3

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

> Source/WebCore/editing/mac/EditorMac.mm:687
> +    RefPtr<MutableStyleProperties> style = MutableStyleProperties::create();

nit: This is actually a Ref<MutableStyleProperties> nowadays.

> Source/WebCore/editing/mac/EditorMac.mm:692
> +    applyStyleToSelection(style.get(), EditActionSetFont);

style.ptr() if you use a Ref<>.

[Chris Dumez]

Comment on attachment 246921 [details]
Patch3

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

> Source/WebCore/editing/mac/EditorMac.mm:689
> +    style->setProperty(CSSPropertyFontStyle, cssValuePool().createIdentifierValue((fontTraits & NSFontItalicTrait) ? CSSValueItalic : CSSValueNormal));

nit: style->setProperty(fontTraits & NSFontItalicTrait ? CSSValueItalic : CSSValueNormal); would be equivalent.

[Enrica Casucci]

Created attachment 246983 [details]
Patch4

Addresses some more comments.

[Enrica Casucci]

Comment on attachment 246983 [details]
Patch4

Removing review. I think I've figured out why it doesn't build Release.

[Enrica Casucci]

Created attachment 246999 [details]
Patch5

Hopefully.

[Tim Horton]

Comment on attachment 246999 [details]
Patch5

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

> Source/WebKit2/Shared/EditorState.cpp:45
> +#if PLATFORM(MAC)

Sam was saying something this morning about this patch wrt. "consistent encoding of font descriptions" but I don't know what he meant. You should ask him.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:129
> +        result.fontName = [nsFont fontName];

dots?

[Enrica Casucci]

Committed revision 180465.

[Jessie Berlin]

Attempted build fix in http://trac.webkit.org/changeset/180473

[Alexey Proskuryakov]

It is super unfortunate that we extend EditorState. It just has no way of being accurate enough of the time.