Bug 168657 - Update flexbox to Blink's tip of tree
Summary: Update flexbox to Blink's tip of tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
: 116484 118638 150445 (view as bug list)
Depends on:
Blocks: 170764
  Show dependency treegraph
 
Reported: 2017-02-21 08:36 PST by Dave Hyatt
Modified: 2024-01-02 11:02 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.09 MB, patch)
2017-02-21 09:05 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (1.34 MB, patch)
2017-02-21 09:22 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (1.09 MB, patch)
2017-02-21 12:26 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.45 MB, application/zip)
2017-02-21 13:17 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.42 MB, application/zip)
2017-02-21 13:39 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (2.00 MB, application/zip)
2017-02-21 13:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.51 MB, application/zip)
2017-02-21 13:50 PST, Build Bot
no flags Details
Patch (1.13 MB, patch)
2017-02-23 15:37 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.05 MB, application/zip)
2017-02-23 16:45 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.69 MB, application/zip)
2017-02-23 16:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.82 MB, application/zip)
2017-02-23 17:17 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1.42 MB, application/zip)
2017-02-24 03:32 PST, Build Bot
no flags Details
Patch (1.14 MB, patch)
2017-02-24 09:52 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (9.92 MB, application/zip)
2017-02-24 13:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.04 MB, application/zip)
2017-02-24 13:56 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (908.49 KB, application/zip)
2017-02-24 14:28 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.79 MB, application/zip)
2017-02-24 18:07 PST, Build Bot
no flags Details
Patch (1.14 MB, patch)
2017-02-27 09:56 PST, Dave Hyatt
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2017-02-21 08:36:03 PST
Update flexbox to Blink's tip of tree
Comment 1 Dave Hyatt 2017-02-21 09:05:16 PST
Created attachment 302269 [details]
Patch
Comment 2 Dave Hyatt 2017-02-21 09:22:47 PST
Created attachment 302270 [details]
Patch
Comment 3 WebKit Commit Bot 2017-02-21 09:26:12 PST
Attachment 302270 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderFlexibleBox.cpp:1265:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 330 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 zalan 2017-02-21 10:09:57 PST
Comment on attachment 302269 [details]
Patch

- use modern for loops
- WTFMove vectors if possible
- use default member initializers if possible  
- use 'use' instead of typdef
- use SetForScope instead of manually saving and restoring values (oldInLayout)
- use is<RendererType>(renderer) instead of renderer.isRendererType()
- remove extra lines/spaces/comments like // MERGEPOINT
- not sure about the WebKit style on this, but you can use operator* instead of std::optional::value()
- we tend to not use const auto& -unless there's a good reason.
- use enum class instead of enum 
- add SPECIALIZE_TYPE_TRAITS_RENDER_OBJECT(RenderType, isRenderType()) for new isRendererType()
I can't possibly review the logic in here (I will try though in a second pass)
Comment 5 Dave Hyatt 2017-02-21 12:26:22 PST
Created attachment 302294 [details]
Patch
Comment 6 Build Bot 2017-02-21 13:17:43 PST
Comment on attachment 302294 [details]
Patch

Attachment 302294 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3167705

New failing tests:
css3/flexbox/flexbox-overflow-auto.html
fullscreen/video-cursor-auto-hide.html
css3/flexbox/flex-align-vertical-writing-mode.html
css3/flexbox/flex-order.html
fast/css-grid-layout/relayout-justify-items-changed.html
css3/flexbox/flex-flow-auto-margins-no-available-space-assert.html
css3/flexbox/columns-auto-size.html
fast/repaint/justify-items-change.html
css3/flexbox/flex-align.html
fast/css-grid-layout/relayout-align-items-changed.html
fast/table/colspanMinWidth-vertical.html
Comment 7 Build Bot 2017-02-21 13:17:46 PST
Created attachment 302297 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-02-21 13:39:43 PST
Comment on attachment 302294 [details]
Patch

Attachment 302294 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3167815

New failing tests:
css3/flexbox/flexbox-overflow-auto.html
fullscreen/video-cursor-auto-hide.html
css3/flexbox/flex-align-vertical-writing-mode.html
css3/flexbox/flex-order.html
fast/css-grid-layout/relayout-justify-items-changed.html
fast/repaint/justify-items-change.html
css3/flexbox/columns-auto-size.html
css3/flexbox/flex-flow-auto-margins-no-available-space-assert.html
css3/flexbox/flex-align.html
fast/css-grid-layout/relayout-align-items-changed.html
fast/table/colspanMinWidth-vertical.html
Comment 9 Build Bot 2017-02-21 13:39:46 PST
Created attachment 302300 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-02-21 13:46:08 PST
Comment on attachment 302294 [details]
Patch

Attachment 302294 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3167800

New failing tests:
css3/flexbox/flexbox-overflow-auto.html
fullscreen/video-cursor-auto-hide.html
css3/flexbox/flex-align-vertical-writing-mode.html
css3/flexbox/flex-order.html
fast/css-grid-layout/relayout-justify-items-changed.html
fast/repaint/justify-items-change.html
css3/flexbox/columns-auto-size.html
css3/flexbox/flex-flow-auto-margins-no-available-space-assert.html
css3/flexbox/flex-align.html
fast/css-grid-layout/relayout-align-items-changed.html
fast/table/colspanMinWidth-vertical.html
Comment 11 Build Bot 2017-02-21 13:46:11 PST
Created attachment 302302 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-02-21 13:50:46 PST
Comment on attachment 302294 [details]
Patch

Attachment 302294 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3167807

New failing tests:
css3/flexbox/flexbox-overflow-auto.html
css3/flexbox/flex-align-vertical-writing-mode.html
css3/flexbox/flex-order.html
fast/css-grid-layout/relayout-justify-items-changed.html
css3/flexbox/flexbox-height-with-overflow-auto.html
media/video-controls-rendering.html
media/controls-without-preload.html
css3/flexbox/flex-flow-auto-margins-no-available-space-assert.html
css3/flexbox/columns-auto-size.html
media/controls-strict.html
css3/flexbox/flex-align.html
fast/css-grid-layout/relayout-align-items-changed.html
fast/table/colspanMinWidth-vertical.html
css3/flexbox/overflow-auto-resizes-correctly.html
css3/flexbox/flex-one-sets-flex-basis-to-zero-px.html
Comment 13 Build Bot 2017-02-21 13:50:49 PST
Created attachment 302303 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 14 Jon Lee 2017-02-23 00:36:54 PST
rdar://problem/30351511
Comment 15 Dave Hyatt 2017-02-23 15:37:32 PST
Created attachment 302592 [details]
Patch
Comment 16 Build Bot 2017-02-23 16:45:48 PST
Comment on attachment 302592 [details]
Patch

Attachment 302592 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3181757

New failing tests:
fast/table/003.html
css3/flexbox/overflow-auto-resizes-correctly.html
fast/replaced/table-percent-height.html
css3/flexbox/flexbox-height-with-overflow-auto.html
css3/flexbox/flex-flow-auto-margins-no-available-space-assert.html
fast/table/colspanMinWidth-vertical.html
css3/flexbox/flex-one-sets-flex-basis-to-zero-px.html
Comment 17 Build Bot 2017-02-23 16:45:52 PST
Created attachment 302608 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 18 Build Bot 2017-02-23 16:55:32 PST
Comment on attachment 302592 [details]
Patch

Attachment 302592 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3181834

New failing tests:
css2.1/20110323/height-percentage-005.htm
fullscreen/video-cursor-auto-hide.html
fast/table/colspanMinWidth-vertical.html
Comment 19 Build Bot 2017-02-23 16:55:37 PST
Created attachment 302609 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 20 Build Bot 2017-02-23 17:17:20 PST
Comment on attachment 302592 [details]
Patch

Attachment 302592 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3181907

New failing tests:
fullscreen/video-cursor-auto-hide.html
fast/table/colspanMinWidth-vertical.html
Comment 21 Build Bot 2017-02-23 17:17:24 PST
Created attachment 302615 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 22 Build Bot 2017-02-24 03:32:37 PST
Comment on attachment 302592 [details]
Patch

Attachment 302592 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3184376

New failing tests:
css2.1/20110323/height-percentage-005.htm
fullscreen/video-cursor-auto-hide.html
fast/table/colspanMinWidth-vertical.html
Comment 23 Build Bot 2017-02-24 03:32:42 PST
Created attachment 302664 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 24 Dave Hyatt 2017-02-24 08:18:26 PST
Only two actual failures here, the css2.1 test case and the fullscreen. Rest just need to be skipped or rebaselined. I have a fix for the fullscreen issue and am rebuilding release now in order to see what's up with the css2.1 test.
Comment 25 Dave Hyatt 2017-02-24 09:52:53 PST
Created attachment 302671 [details]
Patch
Comment 26 Build Bot 2017-02-24 13:53:12 PST
Comment on attachment 302671 [details]
Patch

Attachment 302671 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3187207

New failing tests:
fast/table/003.html
fast/table/colspanMinWidth-vertical.html
Comment 27 Build Bot 2017-02-24 13:53:17 PST
Created attachment 302684 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 28 Build Bot 2017-02-24 13:56:13 PST
Comment on attachment 302671 [details]
Patch

Attachment 302671 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3187277

New failing tests:
fast/table/colspanMinWidth-vertical.html
Comment 29 Build Bot 2017-02-24 13:56:17 PST
Created attachment 302686 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 30 Build Bot 2017-02-24 14:28:14 PST
Comment on attachment 302671 [details]
Patch

Attachment 302671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3187400

New failing tests:
fast/table/colspanMinWidth-vertical.html
Comment 31 Build Bot 2017-02-24 14:28:18 PST
Created attachment 302690 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 32 Build Bot 2017-02-24 18:06:34 PST
Comment on attachment 302671 [details]
Patch

Attachment 302671 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3188118

New failing tests:
fast/table/colspanMinWidth-vertical.html
Comment 33 Build Bot 2017-02-24 18:07:08 PST
Created attachment 302720 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 34 Dave Hyatt 2017-02-24 21:50:09 PST
This is ready to go. The remaining failures just need rebaselines once landed.
Comment 35 Alexey Proskuryakov 2017-02-25 23:43:10 PST
Comment on attachment 302671 [details]
Patch

Please update test results before landing. After all the infrastructure troubles, the tree is not in a state that would make it easy to iterate live.
Comment 36 Dave Hyatt 2017-02-27 09:56:10 PST
Created attachment 302846 [details]
Patch
Comment 37 zalan 2017-02-27 14:19:35 PST
Comment on attachment 302846 [details]
Patch

As we discussed, follow-up patches should address the review comments ^^
Comment 38 Dave Hyatt 2017-02-28 08:26:02 PST
Landed in r213149.
Comment 39 Jon Lee 2017-02-28 10:51:23 PST
*** Bug 150445 has been marked as a duplicate of this bug. ***
Comment 40 Michael Catanzaro 2017-02-28 18:19:05 PST
Looks like it regressed one layout test on GTK port, bug #169010.
Comment 41 Manuel Rego Casasnovas 2017-03-02 03:57:26 PST
Some comments about this patch and the bits related to Grid Layout.
Please, next time ping any of us (svillar, jfernadez or me) to take a look
to the Grid Layout stuff. I don't know why we didn't get notified
as in theory we're in the "CSSGridLayout" watchlist.

* I think that the new hasDefiniteLogicalHeight() implementation could be split
  in a separated patch.
  That change how percentage heights are resolved for any element
  (not only flexbox or grid), so it'd be nice to have it isolated
  on a different patch; as it might introduce some regressions and
  it'd be easier to identify or rollout if needed.
  The related patches on Blink are:
  * https://chromium.googlesource.com/chromium/src/+/9a12b00b915eccd82d4444ecba101f27e2761769
  * https://chromium.googlesource.com/chromium/src/+/c4dc5e50ffa3ca4bef23a105ae8dc8304926b312
  It'd be nice to also import the Grid Layout tests from them.

* Also the changes related to percentage gaps support (RenderGrid::gridGapForDirection())
  were imported as part of this. I don't see a clear reason to import them
  as part of this patch, and I believe this could be done on a separated patch.
  Note that right now we have some code that is never executed,
  as the parser was not modified and percentage gaps are not allowed yet
  (at parsing level, layout code seems ready for them).
  We should enable percentage gaps on the parser and import the tests
  from Blink too:
  * https://chromium.googlesource.com/chromium/src/+/78579c71b9013a88936d31b6ac4ce4b18b0ac339
Comment 42 Ryan Reno 2022-08-22 20:27:26 PDT
*** Bug 116484 has been marked as a duplicate of this bug. ***
Comment 43 Sammy Gill 2024-01-02 11:02:45 PST
*** Bug 118638 has been marked as a duplicate of this bug. ***