Miguel López López (Gerrit)

unread,

Jul 2, 2024, 10:29:57 AMJul 2

to Daniel Cheng, AyeAye, max+watc...@igalia.com, ozone-...@chromium.org, nickdiego+wa...@igalia.com

Miguel López López abandoned this change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Max Ihlenfeldt (Gerrit)

unread,

Jul 8, 2024, 10:08:20 AMJul 8

to Miguel López López, Nick Yamane, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Miguel López López and Nick Yamane

Max Ihlenfeldt voted and added 3 comments

Votes added by Max Ihlenfeldt

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Max Ihlenfeldt . resolved

lgtm % nits!

File ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc

Line 1291, Patchset 1 (Latest): false);

Max Ihlenfeldt . unresolved

nit: Add a comment to investigate why this is the only test that fails without nested runloops, starting with `// TODO(crbug.com/328783999): `.

Also, it'd be great of course if you could find the reason why this test needs the nested runloops! But fixing that can be done in a follow up.

File ui/ozone/platform/wayland/test/wayland_test.h

Line 57, Patchset 1 (Latest): // TODO(https://crbug.com/328783999): Avoid nested runloops by default.

Max Ihlenfeldt . unresolved

nit: Remove this line.

Open in Gerrit

Related details

Attention is currently required from:

Miguel López López
Nick Yamane

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Miguel López López (Gerrit)

unread,

Jul 9, 2024, 5:11:57 AMJul 9

to Nick Yamane, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Max Ihlenfeldt and Nick Yamane

Miguel López López added 2 comments

File ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc

Line 1291, Patchset 1: false);

Max Ihlenfeldt . unresolved

nit: Add a comment to investigate why this is the only test that fails without nested runloops, starting with `// TODO(crbug.com/328783999): `.
Also, it'd be great of course if you could find the reason why this test needs the nested runloops! But fixing that can be done in a follow up.

Miguel López López

I suspect this test needs nested run loops because it runs asynchronous code, as explained in the comment in line 1266. For reference test `DropSeveralMimeTypes` in this same file uses a run loop in it's body and has a similar explanation.

File ui/ozone/platform/wayland/test/wayland_test.h

Line 57, Patchset 1: // TODO(https://crbug.com/328783999): Avoid nested runloops by default.

Max Ihlenfeldt . resolved

nit: Remove this line.

Miguel López López

Done

Open in Gerrit

Related details

Attention is currently required from:

Max Ihlenfeldt
Nick Yamane

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Max Ihlenfeldt (Gerrit)

unread,

Jul 9, 2024, 7:52:02 AMJul 9

to Miguel López López, Nick Yamane, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Miguel López López and Nick Yamane

Max Ihlenfeldt voted and added 2 comments

Votes added by Max Ihlenfeldt

Code-Review

+1

2 comments

Commit Message

Line 10, Patchset 1:all tests except for WaylandDataDragControllerTest.PostToServerAndWait

Max Ihlenfeldt . unresolved

nit: WaylandDataDragControllerTest.DropWhileFetchingData

File ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc

Line 1291, Patchset 1: false);

Max Ihlenfeldt . unresolved

nit: Add a comment to investigate why this is the only test that fails without nested runloops, starting with `// TODO(crbug.com/328783999): `.
Also, it'd be great of course if you could find the reason why this test needs the nested runloops! But fixing that can be done in a follow up.

Miguel López López

I suspect this test needs nested run loops because it runs asynchronous code, as explained in the comment in line 1266. For reference test `DropSeveralMimeTypes` in this same file uses a run loop in it's body and has a similar explanation.

Max Ihlenfeldt

I see. I think what `DropSeveralMimeTypes` does, i.e. having a `RunLoop` in the test directly, is the better approach. So please add a comment like this:
```
// TODO(crbug.com/328783999): Use a RunLoop here (like in DropSeveralMimeTypes) so
we can pass `no_nested_runloops=true` to `PostToServerAndWait()`.
```
and make the change in a follow up. The condition when to quit the `RunLoop` in the test might be non-trivial, so I think it's better to decouple it from this CL.

Open in Gerrit

Related details

Attention is currently required from:

Miguel López López
Nick Yamane

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Miguel López López (Gerrit)

unread,

Jul 9, 2024, 8:14:11 AMJul 9

to Nick Yamane, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Nick Yamane

Miguel López López added 1 comment

File ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc

Line 1291, Patchset 1: false);

Max Ihlenfeldt . resolved

nit: Add a comment to investigate why this is the only test that fails without nested runloops, starting with `// TODO(crbug.com/328783999): `.
Also, it'd be great of course if you could find the reason why this test needs the nested runloops! But fixing that can be done in a follow up.

Miguel López López

I suspect this test needs nested run loops because it runs asynchronous code, as explained in the comment in line 1266. For reference test `DropSeveralMimeTypes` in this same file uses a run loop in it's body and has a similar explanation.

Max Ihlenfeldt

I see. I think what `DropSeveralMimeTypes` does, i.e. having a `RunLoop` in the test directly, is the better approach. So please add a comment like this:
```
// TODO(crbug.com/328783999): Use a RunLoop here (like in DropSeveralMimeTypes) so
we can pass `no_nested_runloops=true` to `PostToServerAndWait()`.
```
and make the change in a follow up. The condition when to quit the `RunLoop` in the test might be non-trivial, so I think it's better to decouple it from this CL.

Miguel López López

Done

Open in Gerrit

Related details

Attention is currently required from:

Nick Yamane

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Miguel López López (Gerrit)

unread,

Jul 9, 2024, 8:15:55 AMJul 9

to Nick Yamane, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Nick Yamane

Miguel López López added 1 comment

Commit Message

Line 10, Patchset 1:all tests except for WaylandDataDragControllerTest.PostToServerAndWait

Max Ihlenfeldt . resolved

nit: WaylandDataDragControllerTest.DropWhileFetchingData

Miguel López López

Done

Open in Gerrit

Related details

Attention is currently required from:

Nick Yamane

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nick Yamane (Gerrit)

unread,

Jul 23, 2024, 10:38:40 PM (3 days ago) Jul 23

to Miguel López López, Orko Garai, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Miguel López López and Orko Garai

Nick Yamane added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Nick Yamane . resolved

orko could you PTAL, as you were the one working on nested-loop-less waits for unit tests? thanks

Open in Gerrit

Related details

Attention is currently required from:

Miguel López López
Orko Garai

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Orko Garai (Gerrit)

unread,

Jul 23, 2024, 10:59:17 PM (3 days ago) Jul 23

to Miguel López López, Nick Yamane, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Miguel López López

Orko Garai voted and added 1 comment

Votes added by Orko Garai

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Orko Garai . resolved

Tentative +1.

If lacros bots are still running it maybe a good idea to check none of the ozone_unittests for lacros are failing as a result.

Open in Gerrit

Related details

Attention is currently required from:

Miguel López López

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Nick Yamane (Gerrit)

unread,

Jul 24, 2024, 9:25:43 AM (3 days ago) Jul 24

to Miguel López López, Orko Garai, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Attention needed from Miguel López López

Nick Yamane voted and added 1 comment

Votes added by Nick Yamane

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Nick Yamane . resolved

Thanks Orko.

Assuming ozone unit tests run successfully in both linux/wayland and lacros try bots, it should be ok to land this.

Open in Gerrit

Related details

Attention is currently required from:

Miguel López López

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jul 24, 2024, 9:30:40 AM (3 days ago) Jul 24

to Miguel López López, Nick Yamane, Orko Garai, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Avoid nested runloops by default in PostToServerAndWait()

Tests seem to work just fine after setting no_nested_runloops to true in
all tests except for WaylandDataDragControllerTest.DropWhileFetchingData

Bug: 328783999

Change-Id: I510ed515a5ff9dfeac524c5e85d4505b809e8852

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

Commit-Queue: Nick Yamane <nick...@igalia.com>

Reviewed-by: Orko Garai <or...@igalia.com>

Reviewed-by: Max Ihlenfeldt <m...@igalia.com>

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

Files:

M ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc
M ui/ozone/platform/wayland/test/wayland_test.h

Change size: S

Delta: 2 files changed, 23 insertions(+), 18 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Orko Garai, +1 by Max Ihlenfeldt

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Avi Drissman (Gerrit)

unread,

Jul 24, 2024, 2:58:07 PM (3 days ago) Jul 24

to Miguel López López, Chromium LUCI CQ, Nick Yamane, Orko Garai, chromium...@chromium.org, Daniel Cheng, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org, Avi Drissman

Avi Drissman has created a revert of this change

Open original change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Avoid nested runloops by default in PostToServerAndWait() [chromium/src : main]

Miguel López López (Gerrit)

Miguel López López abandoned this change

Related details

Max Ihlenfeldt (Gerrit)

Max Ihlenfeldt voted and added 3 comments

Votes added by Max Ihlenfeldt

3 comments

Related details

Miguel López López (Gerrit)

Miguel López López added 2 comments

Related details

Max Ihlenfeldt (Gerrit)

Max Ihlenfeldt voted and added 2 comments

Votes added by Max Ihlenfeldt

2 comments

Related details

Miguel López López (Gerrit)

Miguel López López added 1 comment

Related details

Miguel López López (Gerrit)

Miguel López López added 1 comment

Related details

Nick Yamane (Gerrit)

Nick Yamane added 1 comment

Related details

Orko Garai (Gerrit)

Orko Garai voted and added 1 comment

Votes added by Orko Garai

1 comment

Related details

Nick Yamane (Gerrit)

Nick Yamane voted and added 1 comment

Votes added by Nick Yamane

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information

Avi Drissman (Gerrit)

Avi Drissman has created a revert of this change

Related details