Arthur Sonzogni (Gerrit)

unread,

11:05 AM (2 hours ago) 11:05 AM

to Avi Drissman, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org, kinuko...@chromium.org, blink-...@chromium.org, mac-r...@chromium.org

Attention needed from Avi Drissman

Arthur Sonzogni voted and added 3 comments

Votes added by Arthur Sonzogni

Auto-Submit

+1

3 comments

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Arthur Sonzogni . resolved

Hi Avi, could you please take a look?
I need the

File third_party/blink/renderer/platform/mac/color_mac.mm

Line 64, Patchset 13 (Latest): static base::NoDestructor<std::array<NSColor*, kCacheSize>> cached_colors;

Arthur Sonzogni . resolved

I need this due to ARC.
See patchset N-1
```
[1502/11642] OBJCXX obj/third_party/blink/renderer/platform/platform/color_mac.o
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/third_party/blink/renderer/platform/platform/color_mac.o.d -D__STDC_CONS...(too long)
../../third_party/blink/renderer/platform/mac/color_mac.mm:62:45: error: declaration requires an exit-time destructor [-Werror,-Wexit-time-destructors]
   62 |     static std::array<NSColor*, kCacheSize> cached_colors;
      |                                             ^
```

Line 63, Patchset 6:

  } else {
    constexpr int kCacheSize = 32;
    static std::array<unsigned, kCacheSize> cached_rgba_values;
    static base::NoDestructor<std::array<NSColor*, kCacheSize>> cached_colors;

    auto values_span = base::span(cached_rgba_values);
    auto colors_span = base::span(*cached_colors);

    for (int i = 0; i != kCacheSize; ++i) {
      if (values_span[i] == c) {
        return colors_span[i];
      }
    }

    NSColor* result = [NSColor
        colorWithDeviceRed:static_cast<CGFloat>(color.Red()) / 255
                     green:static_cast<CGFloat>(color.Green()) / 255
                      blue:static_cast<CGFloat>(color.Blue()) / 255
                     alpha:static_cast<CGFloat>(color.AlphaAsInteger()) / 255];

    static int cursor;
    values_span[cursor] = c;
    colors_span[cursor] = result;
    if (++cursor == kCacheSize) {
      cursor = 0;
    }
    return result;

Arthur Sonzogni . resolved

Let's avoid using base::NoDestructor (that's useless for pointers)
Let rename all of the xxx_span into xxx

Arthur Sonzogni

Done

Open in Gerrit

Related details

Attention is currently required from:

Avi Drissman

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Avi Drissman (Gerrit)

unread,

11:42 AM (2 hours ago) 11:42 AM

to Arthur Sonzogni, Avi Drissman, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org, kinuko...@chromium.org, blink-...@chromium.org, mac-r...@chromium.org

Attention needed from Arthur Sonzogni

Avi Drissman voted and added 1 comment

Votes added by Avi Drissman

Code-Review

+1

1 comment

File third_party/blink/renderer/platform/mac/color_mac.mm

Line 64, Patchset 13 (Latest): static base::NoDestructor<std::array<NSColor*, kCacheSize>> cached_colors;

Arthur Sonzogni . resolved

I need this due to ARC.
See patchset N-1
```
[1502/11642] OBJCXX obj/third_party/blink/renderer/platform/platform/color_mac.o
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/third_party/blink/renderer/platform/platform/color_mac.o.d -D__STDC_CONS...(too long)
../../third_party/blink/renderer/platform/mac/color_mac.mm:62:45: error: declaration requires an exit-time destructor [-Werror,-Wexit-time-destructors]
   62 |     static std::array<NSColor*, kCacheSize> cached_colors;
      |                                             ^
```

Avi Drissman

yep! Objective-C objects with ARC are effectively smart pointers that have destructors.

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

11:45 AM (2 hours ago) 11:45 AM

to Avi Drissman, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org, kinuko...@chromium.org, blink-...@chromium.org, mac-r...@chromium.org

Arthur Sonzogni voted and added 1 comment

Votes added by Arthur Sonzogni

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Arthur Sonzogni . resolved

Thanks!

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

David Bokan (Gerrit)

unread,

12:07 PM (1 hour ago) 12:07 PM

to Arthur Sonzogni, Avi Drissman, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org, kinuko...@chromium.org, blink-...@chromium.org, mac-r...@chromium.org

Attention needed from Arthur Sonzogni

David Bokan voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

12:14 PM (1 hour ago) 12:14 PM

to Arthur Sonzogni, David Bokan, Avi Drissman, Akihiro Ota, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, android-bu...@system.gserviceaccount.com, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org, kinuko...@chromium.org, blink-...@chromium.org, mac-r...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[unsafe-buffers] Remove allow_unsafe_buffers from Blink color_mac.mm

This is a mechanical change transitioning Blink color_mac.mm from
coarse, file-level pragmas to fine-grained, instruction-level macros. It
converts raw static arrays to std::array and base::NoDestructor.

Bug: 351564777, 501112808

Doc: go/complete_unsafe_todo_migration

Change-Id: I4734f42808ff2e479ddbae64efa9f8d5002a5c9d

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8045191

Reviewed-by: Avi Drissman <a...@chromium.org>

Commit-Queue: David Bokan <bo...@chromium.org>

Auto-Submit: Arthur Sonzogni <arthurs...@chromium.org>

Reviewed-by: David Bokan <bo...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1659609}

Files:

M third_party/blink/renderer/platform/mac/color_mac.mm

Change size: S

Delta: 1 file changed, 11 insertions(+), 11 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by David Bokan, +1 by Avi Drissman

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[unsafe-buffers] Remove allow_unsafe_buffers from Blink color_mac.mm [chromium/src : main]

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 3 comments

Votes added by Arthur Sonzogni

3 comments

Related details

Avi Drissman (Gerrit)

Avi Drissman voted and added 1 comment

Votes added by Avi Drissman

1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 1 comment

Votes added by Arthur Sonzogni

1 comment

Related details

David Bokan (Gerrit)

David Bokan voted

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information