[CoreText] Remove the Foundation dependency from CTFrame #2427

msft-Jeyaram · 2017-04-05T19:36:00Z

Implemented CTFrame via Runtimebase
Removed associated Foundation usage
Improvements to code
Performance gain

Develop:

With The Changes:

msft-Jeyaram · 2017-04-05T19:40:07Z

Frameworks/include/CoreTextInternal.h

-@public
+#pragma region CTFrame
+struct __CTFrame : CoreFoundation::CppBase<__CTFrame> {
+    __CTFrame() : _lines(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks)) {


sadly due to the extensive usage of the internal vars outside the actual class, these had to go internal-public 😢 #ByDesign

DHowett-MSFT · 2017-04-05T19:50:10Z

Please reword this title to be concise and imperative. 😄 #Resolved

DHowett-MSFT · 2017-04-05T19:52:46Z

Frameworks/CoreText/CTTypesetter.mm

-                                      CGRectMake(offset, 0, width, FLT_MAX));
-    return ([frame->_lines count] > 0) ? static_cast<_CTLine*>([frame->_lines firstObject])->_strRange.length : 0;
+    CTFrameRef frame = _DWriteGetFrame(static_cast<CFAttributedStringRef>(typesetter->_attributedString.get()),
+                                       CFRangeMake(index, [typesetter->_string length] - index),


this still has an objective-C message dispatch. #ByDesign

These are being moved with their associated class being RuntimeBase.
Now i can go a huge change that does all this, but it makes it easier for bugs to go unnoticed.

I'm just breaking out the main classes and it follows through.
#ByDesign

DHowett-MSFT · 2017-04-05T19:53:53Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

-    if (range.length <= 0) {
-        return frame;
-    }
+    CTFrameRef frame = __CTFrame::CreateInstance(kCFAllocatorDefault);


I'm vaguely worried about the implications of two different TUs containing CreateInstance calls. Do they share static storage? Do they call the same instance of GetTypeID? #Resolved

This is the only one occurrence.

In reply to: 110012166 [](ancestors = 110012166)

DHowett-MSFT · 2017-04-05T19:54:25Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

@@ -446,7 +446,8 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _

    while (i < numOfGlyphRuns) {
        _CTLine* line = [[_CTLine new] autorelease];
-        NSMutableArray<_CTRun*>* runs = [NSMutableArray array];
+
+        woc::StrongCF<CFMutableArrayRef> runs{ CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks) };


This will leak. Please use woc::MakeStrongCF. #Resolved

aballway

I think this would be better suited to a larger, more comprehensive PR that converts all of the CT classes to CppBase and removes the dependency on foundation in one fell swoop. That way, we'd be guaranteed to catch all of the changes in one squashed commit rather than several which will have overlapping changes.

aballway · 2017-04-05T19:45:01Z

tests/Benchmark/TextBenchmarkTests.mm

@@ -233,9 +233,9 @@ size_t GetRunCount() const {

 BENCHMARK_F(CoreText, CTFrameDrawComplete);

-class CTFrameDrawYuge : public TextBenchmarkBase {
+class CTFrameDrawHuge : public TextBenchmarkBase {


leave this #ByDesign

behold this, the death of character #ByDesign

😢 #ByDesign

This was originally done by special request. 😿 #ByDesign

aballway · 2017-04-05T19:49:21Z

Frameworks/CoreText/CTFrame.mm

-                }
-                runs.emplace_back(GlyphRunData{ &curRun->_dwriteGlyphRun, relativePosition, (CFDictionaryRef)curRun->_attributes.get() });
+void CTFrameDraw(CTFrameRef frame, CGContextRef ctx) {
+    RETURN_IF(!ctx);


I don't think it's necessary to churn these files to add early returns #ByDesign

Code Clarity, it makes much easier to read + follow the standards of other frameworks. #ByDesign

aballway · 2017-04-05T19:51:41Z

include/CoreText/CTFrame.h

@@ -25,7 +25,7 @@
 #import <CoreGraphics/CGGeometry.h>
 #import <CoreGraphics/CGPath.h>

-typedef const struct __CTFrame* CTFrameRef;
+typedef struct __CTFrame* CTFrameRef;


this should remain const #Resolved

DHowett-MSFT · 2017-04-05T19:59:14Z

tests/Benchmark/TextBenchmarkTests.mm

@@ -233,9 +233,9 @@ size_t GetRunCount() const {

 BENCHMARK_F(CoreText, CTFrameDrawComplete);

-class CTFrameDrawYuge : public TextBenchmarkBase {
+class CTFrameDrawHuge : public TextBenchmarkBase {


behold this, the death of character #ByDesign

ms-jihua · 2017-04-06T00:10:28Z

Frameworks/CoreText/CTFrame.mm

+
+        CGPoint relativePosition = frame->_lineOrigins[i];
+
+        for (size_t j = 0; j < [line->_runs count]; ++j) {


nit: might want to leave a TODO here as a reminder for when you get to CTLine #ByDesign

the compiler will handily totally fall over for this line when that transition is made :) #ByDesign

o, tru #ByDesign

also when you make it private, to track down all these places. well it really goes off ;) #ByDesign

ms-jihua · 2017-04-06T00:12:03Z

Frameworks/CoreText/CTFrame.mm

    }
 }

 /**
- @Status Stub
- @Notes
+ @Status Interoperable


remove the annotation in the .h also #Resolved

msft-Jeyaram · 2017-04-06T18:24:10Z

@DHowett-MSFT @aballway @ms-jihua Ping! #ByDesign

rajsesh · 2017-04-06T21:40:14Z

Frameworks/CoreText/CTFramesetter.mm

-    StrongId<_CTFrame> ret = _DWriteGetFrame(static_cast<CFAttributedStringRef>(typesetter->_attributedString.get()), range, frameRect);
-    ret->_path.reset(CGPathRetain(path));
+    woc::StrongCF<__CTFrame*> ret;
+    ret =


ret [](start = 4, length = 3)

collapse the declaration and assignment into initialization? there is no need to declare and then assign. #ByDesign

This is due to the minor shortfall of StrongCF, assignment of a return value will cause it to invoke a +0 recount and since we don't own the object, it will end up with over releases. #ByDesign

woc::MakeStrongCF it 😸 #ByDesign

LOL yes was about to ;) #ByDesign

aballway · 2017-04-07T17:53:42Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

-        return frame;
-    }
+    __CTFrame* frame = __CTFrame::CreateInstance(kCFAllocatorDefault);
+    CFAutorelease(frame);


nit: use AutoCF vs. CFAutorelease #ByDesign

Since it's returning and auto released object, i'd rather not AutoCF it it then detach it and autorelease it.

In reply to: 110446169 [](ancestors = 110446169)

I hate that this method returns an "un-owned" CTFrame. It should be a Create method or it should not exist. #Resolved

The way this method does stuff does makes me sad. The frame is created in parts :(
Sadly it's not the scope of this review to fix this up real good.
#ByDesign

- Implemented CTFrame via Runtimebase - Removed associated Foundation dependency - Improvements to the code - Performance gain Fixes microsoft#2359

…ypesetter

msft-Jeyaram requested review from aballway, rajsesh and ms-jihua April 5, 2017 19:36

msftclas added the cla-not-required label Apr 5, 2017

msft-Jeyaram changed the title ~~Implementation of CTFrame via Runtimebase + Removal of Foundation usage + Some Perf~~ [CoreText] Implementation of CTFrame via Runtimebase + Removal of Foundation usage + Some Perf Apr 5, 2017

msft-Jeyaram commented Apr 5, 2017

View reviewed changes

msft-Jeyaram changed the title ~~[CoreText] Implementation of CTFrame via Runtimebase + Removal of Foundation usage + Some Perf~~ [CoreText] Implementation of CTFrame via Runtimebase + removal of associated Foundation usage Apr 5, 2017

msft-Jeyaram changed the title ~~[CoreText] Implementation of CTFrame via Runtimebase + removal of associated Foundation usage~~ [CoreText] Implementation of CTFrame via Runtimebase with removal of associated Foundation usage Apr 5, 2017

DHowett-MSFT suggested changes Apr 5, 2017

View reviewed changes

aballway suggested changes Apr 5, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 5, 2017

View reviewed changes

msft-Jeyaram changed the title ~~[CoreText] Implementation of CTFrame via Runtimebase with removal of associated Foundation usage~~ [CoreText] Remove the Foundation dependency from CTFrame Apr 5, 2017

ms-jihua reviewed Apr 6, 2017

View reviewed changes

rajsesh reviewed Apr 6, 2017

View reviewed changes

rajsesh approved these changes Apr 6, 2017

View reviewed changes

msft-Jeyaram force-pushed the CTFrame_4_ branch from 4a24b58 to 554ce83 Compare April 6, 2017 23:16

DHowett-MSFT approved these changes Apr 6, 2017

View reviewed changes

ms-jihua approved these changes Apr 7, 2017

View reviewed changes

aballway reviewed Apr 7, 2017

View reviewed changes

msft-Jeyaram added 4 commits April 11, 2017 15:32

Change the TextBenchMarkTest name to be proper

abf6990

Implementation of CTFrame via CFRuntimebase

6438ffc

- Implemented CTFrame via Runtimebase - Removed associated Foundation dependency - Improvements to the code - Performance gain Fixes microsoft#2359

Implementation of CTFrame CTFrameGetTypeID

1bf6f2a

Remove Foundation usage for CTline and CTFrame associated code in CTT…

fb28d71

…ypesetter

msft-Jeyaram force-pushed the CTFrame_4_ branch from 554ce83 to fb28d71 Compare April 11, 2017 23:39

msft-Jeyaram merged commit 564655d into microsoft:develop Apr 12, 2017

msft-Jeyaram mentioned this pull request Apr 27, 2017

Break CoreGraphics and CoreText's dependencies on Foundation + UIKit #2026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CoreText] Remove the Foundation dependency from CTFrame #2427

[CoreText] Remove the Foundation dependency from CTFrame #2427

msft-Jeyaram commented Apr 5, 2017 •

edited

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

DHowett-MSFT commented Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

aballway left a comment

aballway Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

rajsesh Apr 6, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

aballway Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

ms-jihua Apr 6, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 6, 2017 •

edited by msft-Jeyaram

Loading

ms-jihua Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 6, 2017 •

edited

Loading

ms-jihua Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram commented Apr 6, 2017 •

edited

Loading

rajsesh Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 6, 2017 •

edited

Loading

aballway Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 6, 2017 •

edited

Loading

aballway Apr 7, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 7, 2017

DHowett-MSFT Apr 7, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 7, 2017 •

edited

Loading


		CGPoint relativePosition = frame->_lineOrigins[i];

		for (size_t j = 0; j < [line->_runs count]; ++j) {

[CoreText] Remove the Foundation dependency from CTFrame #2427

[CoreText] Remove the Foundation dependency from CTFrame #2427

Conversation

msft-Jeyaram commented Apr 5, 2017 • edited Loading

msft-Jeyaram Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT commented Apr 5, 2017 • edited by msft-Jeyaram Loading

DHowett-MSFT Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 5, 2017

Choose a reason for hiding this comment

DHowett-MSFT Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway left a comment

Choose a reason for hiding this comment

aballway Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

rajsesh Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 5, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

ms-jihua Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

ms-jihua Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

ms-jihua Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 6, 2017 • edited Loading

rajsesh Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 7, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 7, 2017

Choose a reason for hiding this comment

DHowett-MSFT Apr 7, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 5, 2017 •

edited

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

DHowett-MSFT commented Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

rajsesh Apr 6, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 5, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 5, 2017 •

edited

Loading

aballway Apr 5, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 5, 2017 •

edited by msft-Jeyaram

Loading

ms-jihua Apr 6, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 6, 2017 •

edited by msft-Jeyaram

Loading

ms-jihua Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 6, 2017 •

edited

Loading

ms-jihua Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram commented Apr 6, 2017 •

edited

Loading

rajsesh Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 6, 2017 •

edited

Loading

aballway Apr 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 6, 2017 •

edited

Loading

aballway Apr 7, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 7, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 7, 2017 •

edited

Loading