New issue
Advanced search Search tips

Issue 893068 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Blink paint code uses the explicitly banned std::function

Project Member Reported by brat...@opera.com, Oct 8

Issue description

I just noticed that we use std::function in some parts of the paint code in Blink, and it is explicitly banned according to https://chromium-cpp.appspot.com/ . Instead base::Callback should be used. There are also a couple of other C++ library uses I'm not sure about like std::priority_queue.

core/paint/image_paint_timing_detector.h:                      std::function<bool(std::shared_ptr<ImageRecord>&,
core/paint/image_paint_timing_detector.h:                      std::function<bool(std::shared_ptr<ImageRecord>&,
core/paint/text_paint_timing_detector.h:                      std::function<bool(std::unique_ptr<TextRecord>&,
core/paint/text_paint_timing_detector.h:                      std::function<bool(std::unique_ptr<TextRecord>&,
platform/graphics/paint/raster_invalidator.h:  using RasterInvalidationFunction = std::function<void(const IntRect&)>;
platform/graphics/paint/raster_invalidator_test.cc:      std::function<void(FloatRect&)> mapper = nullptr) {
platform/testing/fake_graphics_layer_client.h:  using Painter = std::function<void(const GraphicsLayer*,


 
Description: Show this description
I'd advice you to proceed carefully, base::Callback requires the use of WTF::Bind in many cases which is significantly slower than the std counterpart.

In some of the layout code we ended up using classic c-style function pointers instead of base::Callback as a std::function replacement for that reason.

image_paint_timing_detector.cc also uses std::shared_ptr which is also explictly forbidden and std::priority_queue which I don't know the state of.

The functions in image_paint_timing_detector.cc look simple though so all of this might be overkill anyway. Plain C functions should be perfect for this.
Owner: maxlg@chromium.org
Status: Started (was: Untriaged)
I will correct that. Thanks. 
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73f4e3a25ed4d25bba9c98b495e8c4dc39ebb62b

commit 73f4e3a25ed4d25bba9c98b495e8c4dc39ebb62b
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Wed Oct 10 20:50:44 2018

[FCP++] Replace std::function, std::shared_ptr

As std::function and std::shared_ptr are banned from use in blink,
this CL is to
1. change the std::function to function pointer
2. replace shared_ptr with WeakPtr
3. change the ownership of image records to id_record_map_ due to
WeakPtr

Bug:  893068 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3711045d7bae0ba4dac6a9ac87fa5d319d8934d7
Reviewed-on: https://chromium-review.googlesource.com/c/1269796
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598492}
[modify] https://crrev.com/73f4e3a25ed4d25bba9c98b495e8c4dc39ebb62b/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/73f4e3a25ed4d25bba9c98b495e8c4dc39ebb62b/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/73f4e3a25ed4d25bba9c98b495e8c4dc39ebb62b/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/73f4e3a25ed4d25bba9c98b495e8c4dc39ebb62b/third_party/blink/renderer/core/paint/text_paint_timing_detector.h

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e4387c05dee8493ae1411cf5154642b42180854

commit 3e4387c05dee8493ae1411cf5154642b42180854
Author: Daniel Bratell <bratell@opera.com>
Date: Thu Oct 11 12:24:49 2018

[jumbo] Give two kNodeNumberLimit constants unique names

In jumbo builds many cc files compile together in the same
translation unit, and thus share the same anonymous namespace.
That means that constants need to have unique names in the
whole build target, and not just unique in the file.

This renames two constants, one that was just added/copied and
one that already existed to give them more descriptive, and
unique, names.

TBR=maxlg@chromium.org,skobes@chromium.org

Bug:  893068 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I45ec9852370fb5572fd53e21136a21f1dd14e220
Reviewed-on: https://chromium-review.googlesource.com/c/1275891
Reviewed-by: Daniel Bratell <bratell@opera.com>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#598730}
[modify] https://crrev.com/3e4387c05dee8493ae1411cf5154642b42180854/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/3e4387c05dee8493ae1411cf5154642b42180854/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc

Status: Fixed (was: Started)

Sign in to add a comment