Move resolveAlignment logic to RenderStyle

Move resolveAlignment logic to RenderStyle

This CL moves resolveAlignment logic to RenderStyle as this seems
more appropriate place for this code. Also, the same logic is being
duplicated in both RenderGrid and RenderFlexible box, so it also
gets rid of duplicated code.

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

[Sunil Ratnu, 2014-09-05 16:16:11]

sunil.ratnu@samsung.com changed reviewers:
+ jchaffraix@chromium.org

[Sunil Ratnu, 2014-09-05 16:16:12]

Please review this. Thanks!

[Sunil Ratnu, 2014-09-05 17:16:30]

sunil.ratnu@samsung.com changed reviewers:
+ cbiesinger@chromium.org

[Sunil Ratnu, 2014-09-06 12:44:27]

On 2014/09/05 16:46:37, cbiesinger wrote:

Gentle ping! :)

[Sunil Ratnu, 2014-09-09 01:49:21]

On 2014/09/06 12:44:27, Sunil Ratnu wrote:
> On 2014/09/05 16:46:37, cbiesinger wrote:
> 
> Gentle ping! :)

Hi all, please review this. Thanks!

[jfernandez, 2014-09-09 08:33:00]

On 2014/09/09 01:49:21, Sunil Ratnu wrote:
> On 2014/09/06 12:44:27, Sunil Ratnu wrote:
> > On 2014/09/05 16:46:37, cbiesinger wrote:
> > 
> > Gentle ping! :)
> 
> Hi all, please review this. Thanks!

Well, I'm not owner of the source code involved, but I've been working on it for
several weeks now and I think I could do an informal review, 

LGTM.

[Sunil Ratnu, 2014-09-09 08:46:16]

On 2014/09/09 08:33:00, jfernandez wrote:
> On 2014/09/09 01:49:21, Sunil Ratnu wrote:
> > On 2014/09/06 12:44:27, Sunil Ratnu wrote:
> > > On 2014/09/05 16:46:37, cbiesinger wrote:
> > > 
> > > Gentle ping! :)
> > 
> > Hi all, please review this. Thanks!
> 
> Well, I'm not owner of the source code involved, but I've been working on it
for
> several weeks now and I think I could do an informal review, 
> 
> LGTM.

Thanks Javier for the review. :)
Owners, please can you also take a look?

[esprehn, 2014-09-09 10:10:50]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org, jfernandez@igalia.com

[esprehn, 2014-09-09 10:10:51]

https://codereview.chromium.org/547673002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/547673002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:160: align =
(parentStyle->alignItems() == ItemPositionAuto) ? ItemPositionStretch :
parentStyle->alignItems();
This is not style adjuster's job, it's not a place to dump static functions.

https://codereview.chromium.org/547673002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/547673002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1291: ItemPosition alignSelf =
StyleAdjuster::resolveAlignment(style(), child->style());
This is backwards, the StyleAdjuster should call some shared function,
RenderGrid should not depend on the StyleAdjuster.

[Sunil Ratnu, 2014-09-09 10:33:09]

https://codereview.chromium.org/547673002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/547673002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1291: ItemPosition alignSelf =
StyleAdjuster::resolveAlignment(style(), child->style());
On 2014/09/09 10:10:51, esprehn wrote:
> This is backwards, the StyleAdjuster should call some shared function,
> RenderGrid should not depend on the StyleAdjuster.

Thanks esprehn for review. Maybe not in StyleAdjuster,but still we can remove
the redundant code. I am not exactly sure of where to have the common code, can
you please provide some suggestion?

[esprehn, 2014-09-09 10:34:52]

RenderStyle.h?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Sunil Ratnu, 2014-09-09 11:06:26]

On 2014/09/09 10:34:52, esprehn wrote:
> RenderStyle.h?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Thanks esprehn for the suggestion. Now, moving to RenderStyle instead of
StyleAdjuster so that the flow is not backwards. Please have a look. Thanks!

[Sunil Ratnu, 2014-09-10 16:44:23]

sunil.ratnu@samsung.com changed reviewers:
+ leviw@chromium.org

[Sunil Ratnu, 2014-09-10 16:44:24]

Hi everyone, please do let me know your opinion on this. Thanks!

[Sunil Ratnu, 2014-09-24 15:31:43]

Gentle ping!

[Sunil Ratnu, 2014-09-30 05:47:24]

On 2014/09/24 15:31:43, Sunil Ratnu wrote:
> Gentle ping!

Gentle ping!

[Julien - ping for review, 2014-10-01 00:39:31]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2014-10-01 00:39:31]

Based on esprehn's earlier review, lgtm

[commit-bot: I haz the power, 2014-10-01 00:41:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/547673002/20001

[commit-bot: I haz the power, 2014-10-01 00:41:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-01 00:41:22]

Failed to apply patch for Source/core/rendering/RenderFlexibleBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderFlexibleBox.cpp
  Hunk #1 succeeded at 36 (offset 1 line).
  Hunk #2 succeeded at 194 with fuzz 1 (offset 1 line).
  Hunk #3 succeeded at 208 (offset 1 line).
  Hunk #4 FAILED at 1006.
  1 out of 4 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderFlexibleBox.cpp.rej

Patch:       Source/core/rendering/RenderFlexibleBox.cpp
Index: Source/core/rendering/RenderFlexibleBox.cpp
diff --git a/Source/core/rendering/RenderFlexibleBox.cpp
b/Source/core/rendering/RenderFlexibleBox.cpp
index
644696facbe1bee7c8cbc791e9ad8c2f4bb10ed7..d893ec3981c7a87123d7ac1922c7816c814a1827
100644
--- a/Source/core/rendering/RenderFlexibleBox.cpp
+++ b/Source/core/rendering/RenderFlexibleBox.cpp
@@ -35,6 +35,7 @@
 #include "core/rendering/RenderLayer.h"
 #include "core/rendering/RenderView.h"
 #include "core/rendering/TextAutosizer.h"
+#include "core/rendering/style/RenderStyle.h"
 #include "platform/LengthFunctions.h"
 #include "wtf/MathExtras.h"
 #include <limits>
@@ -192,14 +193,6 @@ int
RenderFlexibleBox::inlineBlockBaseline(LineDirectionMode direction) const
     return synthesizedBaselineFromContentBox(this, direction) + marginAscent;
 }
 
-static ItemPosition resolveAlignment(const RenderStyle* parentStyle, const
RenderStyle* childStyle)
-{
-    ItemPosition align = childStyle->alignSelf();
-    if (align == ItemPositionAuto)
-        align = (parentStyle->alignItems() == ItemPositionAuto) ?
ItemPositionStretch : parentStyle->alignItems();
-    return align;
-}
-
 void RenderFlexibleBox::removeChild(RenderObject* child)
 {
     RenderBlock::removeChild(child);
@@ -214,8 +207,8 @@ void RenderFlexibleBox::styleDidChange(StyleDifference diff,
const RenderStyle*
         // Flex items that were previously stretching need to be relayed out so
we can compute new available cross axis space.
         // This is only necessary for stretching since other alignment values
don't change the size of the box.
         for (RenderBox* child = firstChildBox(); child; child =
child->nextSiblingBox()) {
-            ItemPosition previousAlignment = resolveAlignment(oldStyle,
child->style());
-            if (previousAlignment == ItemPositionStretch && previousAlignment
!= resolveAlignment(style(), child->style()))
+            ItemPosition previousAlignment =
RenderStyle::resolveAlignment(oldStyle, child->style());
+            if (previousAlignment == ItemPositionStretch && previousAlignment
!= RenderStyle::resolveAlignment(style(), child->style()))
                 child->setChildNeedsLayout(MarkOnlyThis);
         }
     }
@@ -1013,7 +1006,7 @@ void
RenderFlexibleBox::prepareChildForPositionedLayout(RenderBox* child, Layout
 
 ItemPosition RenderFlexibleBox::alignmentForChild(RenderBox* child) const
 {
-    ItemPosition align = resolveAlignment(style(), child->style());
+    ItemPosition align = RenderStyle::resolveAlignment(style(),
child->style());
 
     if (align == ItemPositionBaseline && hasOrthogonalFlow(child))
         align = ItemPositionFlexStart;

[Sunil Ratnu, 2014-10-01 06:21:20]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-10-01 06:22:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/547673002/40001

[commit-bot: I haz the power, 2014-10-01 07:29:02]

Committed patchset #3 (id:40001) as 183039