Remove ComplexTextController dependency on core

Remove ComplexTextController dependency on core

This is part of the core/platform/graphics move.

BUG=297477
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161857

[rwlbuis, 2013-11-11 22:57:12]

Works on linux, but not yet tried on Mac.

[rwlbuis, 2013-11-11 22:59:44]

Compiles on linux, not tried it  yet on Mac.

[eseidel, 2013-11-11 23:41:13]

Seems reasonable.  I would like eae and leviw to chime in.

[eae, 2013-11-11 23:49:25]

On 2013/11/11 23:41:13, eseidel wrote:
> Seems reasonable.  I would like eae and leviw to chime in.

Moving platform specific code from core/platform/graphics/mac/ into
core/rendering seems like a step backwards to me.

[Stephen Chennney, 2013-11-12 00:26:22]

On 2013/11/11 23:49:25, eae wrote:
> On 2013/11/11 23:41:13, eseidel wrote:
> > Seems reasonable.  I would like eae and leviw to chime in.
> 
> Moving platform specific code from core/platform/graphics/mac/ into
> core/rendering seems like a step backwards to me.

I'm with Emil. Can you explain why this is needed for Mac but not other
platforms? Can we apply the solution for other platforms to Mac?

[rwlbuis, 2013-11-12 15:48:27]

On 2013/11/11 23:49:25, eae wrote:
> On 2013/11/11 23:41:13, eseidel wrote:
> > Seems reasonable.  I would like eae and leviw to chime in.
> 
> Moving platform specific code from core/platform/graphics/mac/ into
> core/rendering seems like a step backwards to me.

Right, I was just moving stuff and not thinking clearly, sorry :)

[eae, 2013-11-12 15:50:17]

On 2013/11/12 15:48:27, rwlbuis wrote:
> On 2013/11/11 23:49:25, eae wrote:
> > On 2013/11/11 23:41:13, eseidel wrote:
> > > Seems reasonable.  I would like eae and leviw to chime in.
> > 
> > Moving platform specific code from core/platform/graphics/mac/ into
> > core/rendering seems like a step backwards to me.
> 
> Right, I was just moving stuff and not thinking clearly, sorry :)

No worries :)

[rwlbuis, 2013-11-12 15:50:34]

On 2013/11/12 00:26:22, Stephen Chenney wrote:
> On 2013/11/11 23:49:25, eae wrote:
> > On 2013/11/11 23:41:13, eseidel wrote:
> > > Seems reasonable.  I would like eae and leviw to chime in.
> > 
> > Moving platform specific code from core/platform/graphics/mac/ into
> > core/rendering seems like a step backwards to me.
> 
> I'm with Emil. Can you explain why this is needed for Mac but not other
> platforms? Can we apply the solution for other platforms to Mac?

I am no expert with this code path but I assume the Mac specific
ComplexTextController is there for a reason. Even if it is possible to apply the
other platform solution there is a big chance of regression and doing that is a
bit out of scope of bug 297477 IMHO. I'll upload a less intrusive patch.

[rwlbuis, 2013-11-12 15:52:43]

Make this less intrusive, simply rely on the TextRun and textLength values for
constructing TextLayout instead of RenderText.

[rwlbuis, 2013-11-12 15:56:33]

I uploaded the wrong version, this one removes the rendering/ includes.

[eae, 2013-11-12 16:03:21]

https://codereview.chromium.org/68483006/diff/110002/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/68483006/diff/110002/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:2975: m_renderTextInfo.m_layout
= font.createLayout(RenderBlockFlow::constructTextRun(renderText, font,
renderText, renderText->style()), renderText->textLength(),
m_width.currentWidth(), m_collapseWhiteSpace);
This looks almost identical to the non-mac code below. Can we make them
identical and remove this ifdef?

[rwlbuis, 2013-11-12 16:53:38]

On 2013/11/12 16:03:21, eae wrote:
>
https://codereview.chromium.org/68483006/diff/110002/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/68483006/diff/110002/Source/core/rendering/Re...
> Source/core/rendering/RenderBlockLineLayout.cpp:2975:
m_renderTextInfo.m_layout
> = font.createLayout(RenderBlockFlow::constructTextRun(renderText, font,
> renderText, renderText->style()), renderText->textLength(),
> m_width.currentWidth(), m_collapseWhiteSpace);
> This looks almost identical to the non-mac code below. Can we make them
> identical and remove this ifdef?

This was a mistaken attempt at minimizing work in the non Mac case. I'll upload
a new patch where this logic is centralized better.

[rwlbuis, 2013-11-12 16:56:38]

Centralize createLayout logic, with this approach we do minimal work for non Mac
port.

[eae, 2013-11-12 17:00:29]

https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:74: m_layout = nullptr;
This doesn't seem right, why would createLayout be a no-op on platforms other
than mac?

[rwlbuis, 2013-11-12 17:04:38]

On 2013/11/12 17:00:29, eae wrote:
>
https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlockLineLayout.cpp:74: m_layout = nullptr;
> This doesn't seem right, why would createLayout be a no-op on platforms other
> than mac?

That is the situation even before my patch AFAICS. TextLayout is internal to
mac/ComplexTextController.cpp, and Font::createLayout already returns 0 for non
Mac.

[eae, 2013-11-12 17:06:37]

On 2013/11/12 17:04:38, rwlbuis wrote:
> On 2013/11/12 17:00:29, eae wrote:
> >
>
https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> > 
> >
>
https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
> > Source/core/rendering/RenderBlockLineLayout.cpp:74: m_layout = nullptr;
> > This doesn't seem right, why would createLayout be a no-op on platforms
other
> > than mac?
> 
> That is the situation even before my patch AFAICS. TextLayout is internal to
> mac/ComplexTextController.cpp, and Font::createLayout already returns 0 for
non
> Mac.

Ok, how about we rename it to createLayoutForMacComplexText?

[rwlbuis, 2013-11-12 18:03:13]

On 2013/11/12 17:06:37, eae wrote:
> On 2013/11/12 17:04:38, rwlbuis wrote:
> > On 2013/11/12 17:00:29, eae wrote:
> > >
> >
>
https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
> > > File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/68483006/diff/180001/Source/core/rendering/Re...
> > > Source/core/rendering/RenderBlockLineLayout.cpp:74: m_layout = nullptr;
> > > This doesn't seem right, why would createLayout be a no-op on platforms
> other
> > > than mac?
> > 
> > That is the situation even before my patch AFAICS. TextLayout is internal to
> > mac/ComplexTextController.cpp, and Font::createLayout already returns 0 for
> non
> > Mac.
> 
> Ok, how about we rename it to createLayoutForMacComplexText?

Great idea, I did that in my latest patch and added ASSERT_NOT_REACHED in
non-Mac Font::createLayoutForMacComplexText.

[eae, 2013-11-12 18:07:24]

Great, LGTM

Thanks for your patience.

[commit-bot: I haz the power, 2013-11-12 18:31:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/68483006/220001

[commit-bot: I haz the power, 2013-11-12 18:58:58]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-11-12 19:17:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/68483006/350001

[commit-bot: I haz the power, 2013-11-12 19:17:23]

Failed to apply patch for Source/core/rendering/RenderBlockLineLayout.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBlockLineLayout.cpp
  Hunk #1 FAILED at 65.
  Hunk #2 FAILED at 2971.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderBlockLineLayout.cpp.rej

Patch:       Source/core/rendering/RenderBlockLineLayout.cpp
Index: Source/core/rendering/RenderBlockLineLayout.cpp
diff --git a/Source/core/rendering/RenderBlockLineLayout.cpp
b/Source/core/rendering/RenderBlockLineLayout.cpp
index
7e2b2528cdb5817b28f9b5a87ba97b171a76296c..4cbccfc9b5eea2c8edb511e5dd4218b834cf4e10
100644
--- a/Source/core/rendering/RenderBlockLineLayout.cpp
+++ b/Source/core/rendering/RenderBlockLineLayout.cpp
@@ -65,6 +65,15 @@ struct RenderTextInfo {
     OwnPtr<TextLayout> m_layout;
     LazyLineBreakIterator m_lineBreakIterator;
     const Font* m_font;
+
+    void createLayout(RenderText* renderText, float xPos, bool
collapseWhiteSpace)
+    {
+#if OS(MACOSX)
+        m_layout =
m_font->createLayoutForMacComplexText(RenderBlockFlow::constructTextRun(renderText,
*m_font, renderText, renderText->style()), renderText->textLength(), xPos,
collapseWhiteSpace);
+#else
+        m_layout = nullptr;
+#endif
+    }
 };
 
 class TrailingObjects {
@@ -2971,11 +2980,11 @@ inline bool
BreakingContext::handleText(WordMeasurements& wordMeasurements, bool
     if (m_renderTextInfo.m_text != renderText) {
         m_renderTextInfo.m_text = renderText;
         m_renderTextInfo.m_font = &font;
-        m_renderTextInfo.m_layout = font.createLayout(renderText,
m_width.currentWidth(), m_collapseWhiteSpace);
+        m_renderTextInfo.createLayout(renderText, m_width.currentWidth(),
m_collapseWhiteSpace);
        
m_renderTextInfo.m_lineBreakIterator.resetStringAndReleaseIterator(renderText->text(),
style->locale());
     } else if (m_renderTextInfo.m_layout && m_renderTextInfo.m_font != &font) {
         m_renderTextInfo.m_font = &font;
-        m_renderTextInfo.m_layout = font.createLayout(renderText,
m_width.currentWidth(), m_collapseWhiteSpace);
+        m_renderTextInfo.createLayout(renderText, m_width.currentWidth(),
m_collapseWhiteSpace);
     }
 
     TextLayout* textLayout = m_renderTextInfo.m_layout.get();

[commit-bot: I haz the power, 2013-11-12 19:55:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/68483006/220002

[commit-bot: I haz the power, 2013-11-12 20:32:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-12 20:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/68483006/480001

[commit-bot: I haz the power, 2013-11-12 23:10:05]

Change committed as 161857