Created attachment 354794 [details] Patch

Created attachment 354795 [details] Patch

Comment on attachment 354795 [details] Patch Attachment 354795 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9987268 New failing tests: fast/css-grid-layout/grid-align-baseline-vertical.html fast/css-grid-layout/grid-self-baseline-04.html fast/css-grid-layout/grid-self-baseline-horiz-04.html fast/css-grid-layout/grid-self-baseline-07.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html fast/css-grid-layout/grid-self-baseline-03.html fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html fast/css-grid-layout/grid-self-baseline-06.html

Created attachment 354798 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 354795 [details] Patch Attachment 354795 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9987735 New failing tests: fast/writing-mode/vertical-align-table-baseline.html

Created attachment 354800 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

Comment on attachment 354795 [details] Patch Attachment 354795 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9987697 New failing tests: fast/css-grid-layout/grid-align-baseline-vertical.html fast/css-grid-layout/grid-self-baseline-04.html fast/css-grid-layout/grid-self-baseline-horiz-04.html fast/css-grid-layout/grid-self-baseline-07.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html fast/css-grid-layout/grid-self-baseline-03.html fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html fast/css-grid-layout/grid-self-baseline-06.html

Created attachment 354801 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 354795 [details] Patch Attachment 354795 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9989091 New failing tests: fast/css-grid-layout/grid-align-baseline-vertical.html fast/css-grid-layout/grid-self-baseline-04.html fast/css-grid-layout/grid-self-baseline-horiz-04.html fast/css-grid-layout/grid-self-baseline-07.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html fast/css-grid-layout/grid-self-baseline-03.html fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html fast/css-grid-layout/grid-self-baseline-06.html

Created attachment 354811 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Created attachment 354852 [details] Patch

Created attachment 354855 [details] Patch

Created attachment 354863 [details] Patch

Comment on attachment 354863 [details] Patch Attachment 354863 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9996425 New failing tests: fast/writing-mode/vertical-align-table-baseline.html

Created attachment 354880 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

Comment on attachment 354863 [details] Patch Attachment 354863 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9996214 New failing tests: fast/css-grid-layout/grid-align-baseline-vertical.html fast/css-grid-layout/grid-self-baseline-04.html fast/css-grid-layout/grid-self-baseline-horiz-04.html fast/css-grid-layout/grid-self-baseline-07.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html fast/css-grid-layout/grid-self-baseline-03.html fast/css-grid-layout/grid-self-baseline-vertical-lr-04.html fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html fast/css-grid-layout/grid-self-baseline-two-dimensional.html fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-002.html fast/css-grid-layout/grid-self-baseline-06.html

Created attachment 354882 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Created attachment 354912 [details] Patch

Comment on attachment 354912 [details] Patch Attachment 354912 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10002383 New failing tests: imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html

Created attachment 354916 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 354912 [details] Patch Attachment 354912 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10002394 New failing tests: imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html

Created attachment 354919 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 354912 [details] Patch Attachment 354912 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10002390 New failing tests: fast/css-grid-layout/grid-self-baseline-two-dimensional.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-002.html fast/css-grid-layout/grid-self-baseline-vertical-lr-04.html

Created attachment 354923 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 354912 [details] Patch Attachment 354912 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10002467 New failing tests: imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html

Created attachment 354925 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 355294 [details] Patch

Comment on attachment 355294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review Amazing patch, adding several comments and nits inline. > Source/WebCore/ChangeLog:9 > + the CSS Box Alignment specification [1]. Nit: I cannot find the link to the spec, and I believe it'd be nice to just link it here. :-) > Source/WebCore/ChangeLog:15 > + This feature allows users to align the grid items sharing a Baseline Alignment Context, > + either row or column contexts, to be aligned based on their respective baselines. Nit: Please reword this sencente: "allows users to align ... to be aligned based". > Source/WebCore/ChangeLog:22 > + fast/css-grid-layout/grid-self-baseline-followed-by-item-style-change-should-not-crash.html Don't we have any WPT tests? I guess it'd be nice to add some in the future if that's the case. > Source/WebCore/rendering/GridBaselineAlignment.cpp:39 > +// since in grid we can can 2-dimensional alignment by baseline. In Nit: "can can". Also wrapping in 80-columns is not required in WebKit- > Source/WebCore/rendering/GridBaselineAlignment.cpp:51 > +// since in grid we can can 2-dimensional alignment by baseline. In Nit: "can can" > Source/WebCore/rendering/GridBaselineAlignment.cpp:110 > +const BaselineGroup& GridBaselineAlignment::getBaselineGroupForChild(ItemPosition preference, unsigned sharedContext, const RenderBox& child, GridAxis baselineAxis) const Nit: Remove "get"? > Source/WebCore/rendering/GridBaselineAlignment.cpp:126 > + // Determine Ascent and Descent values of this child with respect to > + // its grid container. Nit: No need for wrapping the lines. > Source/WebCore/rendering/GridBaselineAlignment.cpp:133 > + // Looking up for a shared alignment context perpendicular to the > + // baseline axis. Nit: Ditto regarding wrapping. > Source/WebCore/rendering/GridBaselineAlignment.cpp:171 > +void BaselineGroup::update(const RenderBox& child, LayoutUnit ascent, LayoutUnit descent) Why this is called "update"? SHouldn't be add? It doesn't update anything, it adds the item, and if it already exists it doesn't doo anything. > Source/WebCore/rendering/GridBaselineAlignment.cpp:234 > +// TODO Properly implement baseline-group compatibility Nit: In WebKit it's much more common to put "FIXME: ". > Source/WebCore/rendering/GridBaselineAlignment.cpp:236 > +BaselineGroup& BaselineContext::findCompatibleSharedGroup(const RenderBox& child, ItemPosition preference) The difference between this method and getSharedGroup() is only that getSharedGroup() returns a const. Do we really need this different? If so shouldn't we look for more clearer names so they don't look like different methods. > Source/WebCore/rendering/GridBaselineAlignment.h:35 > + Super nit: I'd add "//" at the beginning of this line so it looks like a big comment. > Source/WebCore/rendering/GridBaselineAlignment.h:51 > +// The 'Update' method is used to store an item (if not already > +// present) and update the max_ascent and max_descent associated to > +// this baseline-sharing group. I'd move this comment to the method. Also the method name is lowercase in WebKit. > Source/WebCore/rendering/GridBaselineAlignment.h:68 > + // block-flow is opposite (LR vs RL) to particular item's Nit: Typo "to particular". Again 80-cols line wrapping is not needed. > Source/WebCore/rendering/GridBaselineAlignment.h:93 > +// * table cells in the same row, along the table's row (inline) axis > +// * table cells in the same column, along the table's column (block) > +// axis > +// * grid items in the same row, along the grid's row (inline) axis > +// * grid items in the same column, along the grid's colum (block) axis > +// * flex items in the same flex line, along the flex container's main > +// axis Do we need to list this here? Maybe this varies at some point in the spec and comment gets outdated. > Source/WebCore/rendering/GridBaselineAlignment.h:107 > + Vector<BaselineGroup>& sharedGroups() { return m_sharedGroups; } This method is not used anywhere. > Source/WebCore/rendering/GridBaselineAlignment.h:108 > + const BaselineGroup& getSharedGroup(const RenderBox& child, ItemPosition preference) const; Why "get"? It's not enough with "sharedGroup()". > Source/WebCore/rendering/GridBaselineAlignment.h:121 > + // TODO Properly implement baseline-group compatibility Nit: Ditto regarding TODO vs FIXME. > Source/WebCore/rendering/GridBaselineAlignment.h:139 > +// baseline- sharing groups will be created for each Baseline Nit: "baseline-" why the hyphen? > Source/WebCore/rendering/RenderBlockFlow.cpp:3024 > + // fontMetrics 'ascent' is the distance above the baseline to the 'over' > + // edge, which is 'top' for horizontal and 'right' for vertical-lr and > + // vertical-rl. However, firstLineBox()->logicalTop() gives the offset from > + // the 'left' edge for vertical-lr, hence we need to use the Font Metrics > + // 'descent' instead. The result should be handled accordingly by the caller > + // as a 'descent' value, in order to compute properly the max baseline. > + if (style().isFlippedLinesWritingMode()) > + return firstRootBox()->logicalTop() + firstLineStyle().fontMetrics().descent(firstRootBox()->baselineType()); > + Doesn't this affect other cases different than grid layout? > Source/WebCore/rendering/RenderGrid.cpp:690 > + // TODO (jfernandez): Can we avoid it ? s/TODO (jfernandez)/FIXME/ > Source/WebCore/rendering/RenderGrid.cpp:1209 > +// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it might be s/TODO(lajava)/FIXME/ > Source/WebCore/rendering/RenderGrid.cpp:1211 > // FIXME: This logic could be refactored somehow and defined in RenderBox. But the FIXME is duplicated here... > LayoutTests/platform/gtk/fast/writing-mode/vertical-align-table-baseline-expected.txt:12 > + text run at (70,-10) width 48: "\x{843D}" Mmm, these changes are not in the ChangeLog. Why we need them as part of this patch? > LayoutTests/platform/win/fast/writing-mode/vertical-align-table-baseline-expected.txt:12 > + text run at (69,-10) width 48: "\x{843D}" Ditto.

Comment on attachment 355294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review >> Source/WebCore/rendering/GridBaselineAlignment.cpp:39 >> +// since in grid we can can 2-dimensional alignment by baseline. In > > Nit: "can can". > > Also wrapping in 80-columns is not required in WebKit- Done >> Source/WebCore/rendering/GridBaselineAlignment.cpp:51 >> +// since in grid we can can 2-dimensional alignment by baseline. In > > Nit: "can can" Fixed. >> Source/WebCore/rendering/GridBaselineAlignment.cpp:126 >> + // its grid container. > > Nit: No need for wrapping the lines. Done >> Source/WebCore/rendering/GridBaselineAlignment.cpp:133 >> + // baseline axis. > > Nit: Ditto regarding wrapping. Done >> Source/WebCore/rendering/GridBaselineAlignment.cpp:171 >> +void BaselineGroup::update(const RenderBox& child, LayoutUnit ascent, LayoutUnit descent) > > Why this is called "update"? SHouldn't be add? > It doesn't update anything, it adds the item, and if it already exists it doesn't doo anything. Every time a new item is added to the group, it updates the max ascent and descent values. >> Source/WebCore/rendering/GridBaselineAlignment.cpp:236 >> +BaselineGroup& BaselineContext::findCompatibleSharedGroup(const RenderBox& child, ItemPosition preference) > > The difference between this method and getSharedGroup() is only that getSharedGroup() returns a const. > Do we really need this different? If so shouldn't we look for more clearer names so they don't look like different methods. Well, getSharedGroup is a public getter, while findCompatibleSharedGroup is a private function also used in the BaselineGroup::update method. I find this later function name more auto-descriptive, which is useful for the internal private logic. However, it's too long for a public getter. I have not a strong opinion on this issue, so I'm ok to use the same name and just a different return value (const vs non-const); we could also add that as a prefix to the function name. >> Source/WebCore/rendering/GridBaselineAlignment.h:35 >> + > > Super nit: I'd add "//" at the beginning of this line so it looks like a big comment. Done. >> Source/WebCore/rendering/GridBaselineAlignment.h:51 >> +// this baseline-sharing group. > > I'd move this comment to the method. Also the method name is lowercase in WebKit. Ok. >> Source/WebCore/rendering/GridBaselineAlignment.h:68 >> + // block-flow is opposite (LR vs RL) to particular item's > > Nit: Typo "to particular". > Again 80-cols line wrapping is not needed. Done. >> Source/WebCore/rendering/GridBaselineAlignment.h:93 >> +// axis > > Do we need to list this here? Maybe this varies at some point in the spec and comment gets outdated. I'll add a link to the spec, so we can realize of any change in the spec affecting this logic, but I think the comment is still useful. >> Source/WebCore/rendering/GridBaselineAlignment.h:107 >> + Vector<BaselineGroup>& sharedGroups() { return m_sharedGroups; } > > This method is not used anywhere. True, I'll remove it. >> Source/WebCore/rendering/GridBaselineAlignment.h:108 >> + const BaselineGroup& getSharedGroup(const RenderBox& child, ItemPosition preference) const; > > Why "get"? It's not enough with "sharedGroup()". Agreed. >> Source/WebCore/rendering/GridBaselineAlignment.h:121 >> + // TODO Properly implement baseline-group compatibility > > Nit: Ditto regarding TODO vs FIXME. Done >> Source/WebCore/rendering/RenderBlockFlow.cpp:3024 >> + > > Doesn't this affect other cases different than grid layout? Yes, it indeed may affect to non-grid cases. >> Source/WebCore/rendering/RenderGrid.cpp:690 >> + // TODO (jfernandez): Can we avoid it ? > > s/TODO (jfernandez)/FIXME/ Done >> Source/WebCore/rendering/RenderGrid.cpp:1209 >> +// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it might be > > s/TODO(lajava)/FIXME/ Sure. >> Source/WebCore/rendering/RenderGrid.cpp:1211 >> // FIXME: This logic could be refactored somehow and defined in RenderBox. > > But the FIXME is duplicated here... true, I'll remove the former one. >> LayoutTests/platform/gtk/fast/writing-mode/vertical-align-table-baseline-expected.txt:12 >> + text run at (70,-10) width 48: "\x{843D}" > > Mmm, these changes are not in the ChangeLog. Why we need them as part of this patch? Those changes are caused by the new logic added in the RenderBlockFlow class mentioned above. I think the new results are correct, so that's why the affected tests need to be rebaselined. >> LayoutTests/platform/win/fast/writing-mode/vertical-align-table-baseline-expected.txt:12 >> + text run at (69,-10) width 48: "\x{843D}" > > Ditto. Already answered above.

Created attachment 355365 [details] Patch

Comment on attachment 355294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review >>> Source/WebCore/rendering/GridBaselineAlignment.cpp:171 >>> +void BaselineGroup::update(const RenderBox& child, LayoutUnit ascent, LayoutUnit descent) >> >> Why this is called "update"? SHouldn't be add? >> It doesn't update anything, it adds the item, and if it already exists it doesn't doo anything. > > Every time a new item is added to the group, it updates the max ascent and descent values. Ok, I got it now, this is related to the BaselineGroup so update() is better. > Source/WebCore/rendering/GridBaselineAlignment.cpp:173 > + if (m_items.add(&child).isNewEntry) { Might be nice to avoid storing the items if it's not actually needed, you can maybe add a TODO. >>> Source/WebCore/rendering/GridBaselineAlignment.cpp:236 >>> +BaselineGroup& BaselineContext::findCompatibleSharedGroup(const RenderBox& child, ItemPosition preference) >> >> The difference between this method and getSharedGroup() is only that getSharedGroup() returns a const. >> Do we really need this different? If so shouldn't we look for more clearer names so they don't look like different methods. > > Well, getSharedGroup is a public getter, while findCompatibleSharedGroup is a private function also used in the BaselineGroup::update method. I find this later function name more auto-descriptive, which is useful for the internal private logic. However, it's too long for a public getter. > > I have not a strong opinion on this issue, so I'm ok to use the same name and just a different return value (const vs non-const); we could also add that as a prefix to the function name. Ok, I think I understand now, I don't have a strong opinion either, so I'm fine with the current proposal. >>> Source/WebCore/rendering/RenderBlockFlow.cpp:3024 >>> + >> >> Doesn't this affect other cases different than grid layout? > > Yes, it indeed may affect to non-grid cases. Yes this is definitevely fixing a separated issue. Please report a bug about it and add a test like https://jsbin.com/pocehoquru/1/edit?html,css,output.

Created attachment 355464 [details] Patch

Comment on attachment 355294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review >>>> Source/WebCore/rendering/RenderBlockFlow.cpp:3024 >>>> + >>> >>> Doesn't this affect other cases different than grid layout? >> >> Yes, it indeed may affect to non-grid cases. > > Yes this is definitevely fixing a separated issue. Please report a bug about it and add a test like https://jsbin.com/pocehoquru/1/edit?html,css,output. Just for the future reference, this was fixed at https://bugs.webkit.org/show_bug.cgi?id=191881.

Comment on attachment 355464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review LGTM. I'm just wondering if svillar wants to take a look too, as the patch is rather big. :-) > Source/WebCore/rendering/GridBaselineAlignment.cpp:169 > + if (m_items.add(&child).isNewEntry) { I'd add a FIXME here about getting rid if this hash set. It'd be nice to verify if it's actually need. > Source/WebCore/rendering/RenderBlockFlow.cpp:3018 > + Nit: This newline is not needed. > Source/WebCore/rendering/RenderGrid.cpp:1218 > + Ditto. > LayoutTests/platform/mac/TestExpectations:600 > > +# Baseline Alignment tests affected by 1px diff failures only on Mac platforms > +webkit.org/b/145566 fast/css-grid-layout/grid-align-baseline-vertical.html [ Failure ] > +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-03.html [ ImageOnlyFailure ] > +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-04.html [ ImageOnlyFailure ] > +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html [ ImageOnlyFailure ] > +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html [ ImageOnlyFailure ] > +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-06.html [ ImageOnlyFailure ] > +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-07.html [ ImageOnlyFailure ] > + It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests. Have you reported a bug about this? If so, please link it from the TestExpectations entries. Isn't enough with defining this here? Or we also need it in ios-simulator? If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms".

Comment on attachment 355464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review >> Source/WebCore/rendering/GridBaselineAlignment.cpp:169 >> + if (m_items.add(&child).isNewEntry) { > > I'd add a FIXME here about getting rid if this hash set. > It'd be nice to verify if it's actually need. ok >> Source/WebCore/rendering/RenderBlockFlow.cpp:3018 >> + > > Nit: This newline is not needed. True >> Source/WebCore/rendering/RenderGrid.cpp:1218 >> + > > Ditto. ok >> LayoutTests/platform/mac/TestExpectations:600 >> + > > It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests. > Have you reported a bug about this? If so, please link it from the TestExpectations entries. > > Isn't enough with defining this here? Or we also need it in ios-simulator? > If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms". The bug webkit.org/b/145566 has been reported precisely for that issue. It's actually in the TestExpectation; what do you mean ? We indeed need additional entries in the ios-simulator TestExpecation. I can update the comment accordingly.

Created attachment 355490 [details] Patch

Comment on attachment 355464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review >>> LayoutTests/platform/mac/TestExpectations:600 >>> + >> >> It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests. >> Have you reported a bug about this? If so, please link it from the TestExpectations entries. >> >> Isn't enough with defining this here? Or we also need it in ios-simulator? >> If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms". > > The bug webkit.org/b/145566 has been reported precisely for that issue. It's actually in the TestExpectation; what do you mean ? > > We indeed need additional entries in the ios-simulator TestExpecation. I can update the comment accordingly. webkit.org/b/145566 is this bug, that's why I think you should link a different one. Maybe you were referring to webkit.org/b/170293 ?

Comment on attachment 355490 [details] Patch Attachment 355490 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10115837 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html

Created attachment 355500 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 355464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review >>>> LayoutTests/platform/mac/TestExpectations:600 >>>> + >>> >>> It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests. >>> Have you reported a bug about this? If so, please link it from the TestExpectations entries. >>> >>> Isn't enough with defining this here? Or we also need it in ios-simulator? >>> If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms". >> >> The bug webkit.org/b/145566 has been reported precisely for that issue. It's actually in the TestExpectation; what do you mean ? >> >> We indeed need additional entries in the ios-simulator TestExpecation. I can update the comment accordingly. > > webkit.org/b/145566 is this bug, that's why I think you should link a different one. > Maybe you were referring to webkit.org/b/170293 ? Oh, yes, I meant webkit.org/b/170293 of course. I'll update the TestExpectations with the correct bug.

Created attachment 355502 [details] Patch

Comment on attachment 355464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review >>> Source/WebCore/rendering/GridBaselineAlignment.cpp:169 >>> + if (m_items.add(&child).isNewEntry) { >> >> I'd add a FIXME here about getting rid if this hash set. >> It'd be nice to verify if it's actually need. > > ok After reviewed again the code, storing the grid items is necessary for several reasons; one of the most important is that we need to know the number of elements on each Baseline Group.

Comment on attachment 355502 [details] Patch Clearing flags on attachment: 355502 Committed r238457: <https://trac.webkit.org/changeset/238457>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46220663>