[LayoutNG] Introduce LayoutNGMixin in preparation for LayoutNGTableCell. [chromium/src : master]

0 views
Skip to first unread message

Karl Anders Øygard (Gerrit)

unread,
Oct 20, 2017, 4:55:26 PM10/20/17
to atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Emil A Eklund, Ian Kilpatrick, Commit Bot, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

ptal.

Some of the names are now a little misleading (eg. EnclosingNGBlockFlow), suggestions are welcome.

The coding style allows for explicit instantiation of the template classes instead of defining all methods in the header files, which I preferred for LayoutNGMixin, but MSVC throws a warning for this, which fails the build. To prevent this, I disabled the specific warning, but I'm not sure if this is kosher. If moving the definitions to the header file is preferred, please let me know.

View Change

    To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
    Gerrit-Change-Number: 725346
    Gerrit-PatchSet: 12
    Gerrit-Owner: Karl Anders Øygard <ka...@opera.com>
    Gerrit-Reviewer: Emil A Eklund <e...@chromium.org>
    Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>
    Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com>
    Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
    Gerrit-CC: Commit Bot <commi...@chromium.org>
    Gerrit-CC: Dongseong Hwang <dongseo...@intel.com>
    Gerrit-Comment-Date: Fri, 20 Oct 2017 20:55:19 +0000
    Gerrit-HasComments: No
    Gerrit-HasLabels: No

    Emil A Eklund (Gerrit)

    unread,
    Oct 20, 2017, 5:31:14 PM10/20/17
    to Karl Anders Øygard, atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Ian Kilpatrick, Commit Bot, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

    Looks good to me but please allow Ian to comment before landing.

    Thank you!

    Patch set 12:Code-Review +1

    View Change

    1 comment:

    To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
    Gerrit-Change-Number: 725346
    Gerrit-PatchSet: 12
    Gerrit-Owner: Karl Anders Øygard <ka...@opera.com>
    Gerrit-Reviewer: Emil A Eklund <e...@chromium.org>
    Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>
    Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com>
    Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
    Gerrit-CC: Commit Bot <commi...@chromium.org>
    Gerrit-CC: Dongseong Hwang <dongseo...@intel.com>
    Gerrit-Comment-Date: Fri, 20 Oct 2017 21:31:09 +0000
    Gerrit-HasComments: Yes
    Gerrit-HasLabels: Yes

    Aleks Totic (Gerrit)

    unread,
    Oct 20, 2017, 5:35:13 PM10/20/17
    to Karl Anders Øygard, atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Emil A Eklund, Ian Kilpatrick, Commit Bot, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

    The coding style allows for explicit instantiation of the template classes instead of defining all methods in the header files, which I preferred for LayoutNGMixin, but MSVC throws a warning for this, which fails the build. To prevent this, I disabled the specific warning, but I'm not sure if this is kosher. If moving the definitions to the header file is preferred, please let me know.

    ng_inline_items_builder.cc also uses explicit instantiation pattern, and compiles without the #pragma.

    View Change

      To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
      Gerrit-Change-Number: 725346
      Gerrit-PatchSet: 12
      Gerrit-Owner: Karl Anders Øygard <ka...@opera.com>
      Gerrit-Reviewer: Emil A Eklund <e...@chromium.org>
      Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com>
      Gerrit-CC: Aleks Totic <ato...@chromium.org>
      Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-CC: Dongseong Hwang <dongseo...@intel.com>
      Gerrit-Comment-Date: Fri, 20 Oct 2017 21:35:08 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: No

      Ian Kilpatrick (Gerrit)

      unread,
      Oct 20, 2017, 5:52:15 PM10/20/17
      to Karl Anders Øygard, atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Aleks Totic, Emil A Eklund, Commit Bot, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

      Patch set 12:Code-Review +1

      View Change

      1 comment:

      To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
      Gerrit-Change-Number: 725346
      Gerrit-PatchSet: 12
      Gerrit-Owner: Karl Anders Øygard <ka...@opera.com>
      Gerrit-Reviewer: Emil A Eklund <e...@chromium.org>
      Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com>
      Gerrit-CC: Aleks Totic <ato...@chromium.org>
      Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-CC: Dongseong Hwang <dongseo...@intel.com>
      Gerrit-Comment-Date: Fri, 20 Oct 2017 21:52:09 +0000
      Gerrit-HasComments: Yes
      Gerrit-HasLabels: Yes

      Karl Anders Øygard (Gerrit)

      unread,
      Oct 22, 2017, 11:39:37 AM10/22/17
      to atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Ian Kilpatrick, Aleks Totic, Emil A Eklund, Commit Bot, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

      Patch Set 12:

      The coding style allows for explicit instantiation of the template classes instead of defining all methods in the header files, which I preferred for LayoutNGMixin, but MSVC throws a warning for this, which fails the build. To prevent this, I disabled the specific warning, but I'm not sure if this is kosher. If moving the definitions to the header file is preferred, please let me know.

      ng_inline_items_builder.cc also uses explicit instantiation pattern, and compiles without the #pragma.

      Thanks, Aleks, that's exactly what I needed. #pragma removed!

      View Change

      2 comments:

        • will some/most of this also be shared in the mixin for table cells?

          Yes, some of this can be shared with table cells and captions. I skipped over that part for this patch.

      To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
      Gerrit-Change-Number: 725346
      Gerrit-PatchSet: 14
      Gerrit-Owner: Karl Anders Øygard <ka...@opera.com>
      Gerrit-Reviewer: Emil A Eklund <e...@chromium.org>
      Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>
      Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com>
      Gerrit-CC: Aleks Totic <ato...@chromium.org>
      Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-CC: Dongseong Hwang <dongseo...@intel.com>
      Gerrit-Comment-Date: Sun, 22 Oct 2017 15:39:30 +0000
      Gerrit-HasComments: Yes
      Gerrit-HasLabels: No

      Karl Anders Øygard (Gerrit)

      unread,
      Oct 23, 2017, 3:45:31 AM10/23/17
      to Ian Kilpatrick, Emil A Eklund, atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang, Commit Bot, Aleks Totic

      Karl Anders Øygard uploaded patch set #15 to this change.

      View Change

      [LayoutNG] Introduce LayoutNGMixin in preparation for LayoutNGTableCell.

      This class holds code shared between LayoutNG subclasses of
      LayoutBlockFlow.  Inheritance is parameterized, but only valid
      for LayoutBlockFlow and subclasses.

      Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
      Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
      ---
      M third_party/WebKit/Source/core/layout/BUILD.gn
      M third_party/WebKit/Source/core/layout/LayoutBlockFlow.h
      M third_party/WebKit/Source/core/layout/LayoutObject.cpp
      M third_party/WebKit/Source/core/layout/LayoutObject.h
      M third_party/WebKit/Source/core/layout/LayoutText.cpp
      M third_party/WebKit/Source/core/layout/ng/NGInlineLayoutTest.cpp
      M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
      M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc
      M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.h
      M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node_offset_mapping_test.cc
      M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node_test.cc
      M third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc
      M third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.h
      A third_party/WebKit/Source/core/layout/ng/layout_ng_mixin.cc
      A third_party/WebKit/Source/core/layout/ng/layout_ng_mixin.h
      M third_party/WebKit/Source/core/layout/ng/legacy_layout_tree_walking.cc
      M third_party/WebKit/Source/core/layout/ng/legacy_layout_tree_walking.h
      M third_party/WebKit/Source/core/layout/ng/ng_base_layout_algorithm_test.cc
      M third_party/WebKit/Source/core/layout/ng/ng_base_layout_algorithm_test.h
      M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
      M third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
      M third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
      M third_party/WebKit/Source/core/paint/ng/ng_block_flow_painter.h
      23 files changed, 378 insertions(+), 274 deletions(-)

      To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: newpatchset
      Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
      Gerrit-Change-Number: 725346
      Gerrit-PatchSet: 15

      Karl Anders Øygard (Gerrit)

      unread,
      Oct 23, 2017, 3:45:51 AM10/23/17
      to atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Ian Kilpatrick, Aleks Totic, Emil A Eklund, Commit Bot, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

      Patch set 15:Commit-Queue +2

      View Change

        To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: chromium/src
        Gerrit-Branch: master
        Gerrit-MessageType: comment
        Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
        Gerrit-Change-Number: 725346
        Gerrit-PatchSet: 15
        Gerrit-Owner: Karl Anders Øygard <ka...@opera.com>
        Gerrit-Reviewer: Emil A Eklund <e...@chromium.org>
        Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org>
        Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com>
        Gerrit-CC: Aleks Totic <ato...@chromium.org>
        Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
        Gerrit-CC: Commit Bot <commi...@chromium.org>
        Gerrit-CC: Dongseong Hwang <dongseo...@intel.com>
        Gerrit-Comment-Date: Mon, 23 Oct 2017 07:45:46 +0000
        Gerrit-HasComments: No
        Gerrit-HasLabels: Yes

        Commit Bot (Gerrit)

        unread,
        Oct 23, 2017, 3:46:04 AM10/23/17
        to Karl Anders Øygard, atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Ian Kilpatrick, Aleks Totic, Emil A Eklund, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

        CQ is trying da patch.

        Note: The patchset sent to CQ was uploaded after this CL was approved.
        "Edit commit message" https://chromium-review.googlesource.com/c/725346/15

        Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/725346/15

        Bot data: {"action": "start", "triggered_at": "2017-10-23T07:45:46.0Z", "cq_cfg_revision": "573b6c39d3de239a70e8fa672647b63bf0bd1f89", "revision": "4b4811996f17b5a12158abeff681f4a002bc4618"}

        Gerrit-Comment-Date: Mon, 23 Oct 2017 07:45:59 +0000
        Gerrit-HasComments: No
        Gerrit-HasLabels: No

        Commit Bot (Gerrit)

        unread,
        Oct 23, 2017, 6:23:29 AM10/23/17
        to Karl Anders Øygard, atotic+...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dgrog...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, lchoi+...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Ian Kilpatrick, Aleks Totic, Emil A Eklund, Christian Biesinger, chromium...@chromium.org, Dongseong Hwang

        Commit Bot merged this change.

        View Change

        Approvals: Emil A Eklund: Looks good to me Ian Kilpatrick: Looks good to me Karl Anders Øygard: Commit 
        [LayoutNG] Introduce LayoutNGMixin in preparation for LayoutNGTableCell.

        This class holds code shared between LayoutNG subclasses of
        LayoutBlockFlow.  Inheritance is parameterized, but only valid
        for LayoutBlockFlow and subclasses.

        Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
        Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149
        Reviewed-on: https://chromium-review.googlesource.com/725346
        Commit-Queue: Karl Anders Øygard <ka...@opera.com>
        Reviewed-by: Emil A Eklund <e...@chromium.org>
        Reviewed-by: Ian Kilpatrick <ikilp...@chromium.org>
        Cr-Commit-Position: refs/heads/master@{#510749}

        To view, visit change 725346. To unsubscribe, or for help writing mail filters, visit settings.
        


         Gerrit-Project: chromium/src 
        

         Gerrit-Branch: master 
        

         Gerrit-MessageType: merged 
        

         Gerrit-Change-Id: I973193718d046ec6e8aa1a9bf548c5fef45aa149 
        

         Gerrit-Change-Number: 725346 
        

         Gerrit-PatchSet: 16 
        

         Gerrit-Owner: Karl Anders Øygard <ka...@opera.com> 
        

         Gerrit-Reviewer: Commit Bot <commi...@chromium.org> 
        

         Gerrit-Reviewer: Emil A Eklund <e...@chromium.org> 
        

         Gerrit-Reviewer: Ian Kilpatrick <ikilp...@chromium.org> 
        

         Gerrit-Reviewer: Karl Anders Øygard <ka...@opera.com> 
        

         Gerrit-CC: Aleks Totic <ato...@chromium.org> 
        

         Gerrit-CC: Christian Biesinger <cbies...@chromium.org> 
        

        Reply all
        Reply to author
        Forward
        0 new messages