Xiyuan Xia (Gerrit)

unread,

Nov 11, 2024, 5:12:01 PMNov 11

to Dave Tapuska, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Dave Tapuska

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Xiyuan Xia . resolved

Dave, a simple follow-up CL to fix the CL you reviewd for Zhengzheng. Hope it is okay. Thanks.

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska

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

Dave Tapuska (Gerrit)

unread,

Nov 11, 2024, 5:21:28 PMNov 11

to Xiyuan Xia, Andrey Kosyakov, Chromium LUCI CQ, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov and Xiyuan Xia

Dave Tapuska added 1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Dave Tapuska . unresolved

I don't think this is the right fix. I think there is something wrong with the probe DEPs.. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=47?q=instrumentation_probes%20BUILD.gn&ss=chromium

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov
Xiyuan Xia

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

Xiyuan Xia (Gerrit)

unread,

Nov 11, 2024, 6:12:28 PMNov 11

to Andrey Kosyakov, Chromium LUCI CQ, Dave Tapuska, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov and Dave Tapuska

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3:

Dave Tapuska . unresolved

I don't think this is the right fix. I think there is something wrong with the probe DEPs.. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=47?q=instrumentation_probes%20BUILD.gn&ss=chromium

Xiyuan Xia

Thanks for the link.

Please check wheter PS2 make sense. It also fixes the compile error locally for me.

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov
Dave Tapuska

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

Andrey Kosyakov (Gerrit)

unread,

Nov 11, 2024, 6:34:49 PMNov 11

to Xiyuan Xia, Chromium LUCI CQ, Dave Tapuska, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Dave Tapuska and Xiyuan Xia

Andrey Kosyakov added 2 comments

Patchset-level comments

File-level comment, Patchset 3:

Dave Tapuska . unresolved

I don't think this is the right fix. I think there is something wrong with the probe DEPs.. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=47?q=instrumentation_probes%20BUILD.gn&ss=chromium

Xiyuan Xia

Thanks for the link.
Please check wheter PS2 make sense. It also fixes the compile error locally for me.

Andrey Kosyakov

I'm not sure the deps Dave is pointing to are wrong. First, at the template level we have no idea what actual probes would be dependent on (this would be determined by the actual `probe_set` argument and thus would be more appropriate at the invoker level ([here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=80) in this case), and second, if you look at the actual stuff core_probes_impl includes, it's [all within core](https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/blink/renderer/core/core_probes_impl.cc), i.e. should not require an additional dependency. My reading of this error log:
```
In file included from gen/third_party/blink/renderer/core/core_probes_impl.cc:16:
In file included from ../../third_party/blink/renderer/core/inspector/inspector_animation_agent.h:12:
In file included from ../../third_party/blink/renderer/core/animation/scroll_snapshot_timeline.h:15:
In file included from ../../third_party/blink/renderer/core/layout/layout_box.h:46:
In file included from ../../third_party/blink/renderer/core/scroll/scrollbar_theme.h:32:
In file included from ../../third_party/blink/renderer/core/exported/web_view_impl.h:67:
In file included from ../../third_party/blink/renderer/core/frame/resize_viewport_anchor.h:9:
In file included from ../../third_party/blink/renderer/core/page/page.h:50:
../../third_party/blink/renderer/bindings/core/v8/v8_compile_hints_producer.h:8:10: fatal error: 'third_party/blink/renderer/bindings/buildflags.h' file not found
    8 | #include "third_party/blink/renderer/bindings/buildflags.h"
```
 is that the missing dependency is somewhere within the bottom couple of entries.

File third_party/blink/renderer/core/probe/BUILD.gn

Line 48, Patchset 5 (Latest): "//third_party/blink/renderer/bindings:buildflags",

Andrey Kosyakov . unresolved

So where do we actually depend on bindings in the generated code?

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Xiyuan Xia

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

Dave Tapuska (Gerrit)

unread,

Nov 11, 2024, 6:44:28 PMNov 11

to Xiyuan Xia, Andrey Kosyakov, Chromium LUCI CQ, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Xiyuan Xia

Dave Tapuska added 1 comment

Patchset-level comments

File-level comment, Patchset 3:

Dave Tapuska . unresolved

I don't think this is the right fix. I think there is something wrong with the probe DEPs.. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=47?q=instrumentation_probes%20BUILD.gn&ss=chromium

Xiyuan Xia

Thanks for the link.
Please check wheter PS2 make sense. It also fixes the compile error locally for me.

Andrey Kosyakov

I'm not sure the deps Dave is pointing to are wrong. First, at the template level we have no idea what actual probes would be dependent on (this would be determined by the actual `probe_set` argument and thus would be more appropriate at the invoker level ([here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=80) in this case), and second, if you look at the actual stuff core_probes_impl includes, it's [all within core](https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/blink/renderer/core/core_probes_impl.cc), i.e. should not require an additional dependency. My reading of this error log:
```
In file included from gen/third_party/blink/renderer/core/core_probes_impl.cc:16:
In file included from ../../third_party/blink/renderer/core/inspector/inspector_animation_agent.h:12:
In file included from ../../third_party/blink/renderer/core/animation/scroll_snapshot_timeline.h:15:
In file included from ../../third_party/blink/renderer/core/layout/layout_box.h:46:
In file included from ../../third_party/blink/renderer/core/scroll/scrollbar_theme.h:32:
In file included from ../../third_party/blink/renderer/core/exported/web_view_impl.h:67:
In file included from ../../third_party/blink/renderer/core/frame/resize_viewport_anchor.h:9:
In file included from ../../third_party/blink/renderer/core/page/page.h:50:
../../third_party/blink/renderer/bindings/core/v8/v8_compile_hints_producer.h:8:10: fatal error: 'third_party/blink/renderer/bindings/buildflags.h' file not found
    8 | #include "third_party/blink/renderer/bindings/buildflags.h"
```
 is that the missing dependency is somewhere within the bottom couple of entries.

Dave Tapuska

I think you got to gn describe the core_probes target and see if it has the buildflags as a deps. Liekly it doesn't. We generally don't public_deps the dependencies and that is likely the problem here is that the probes code doesn't explicitly list deps (and incorrectly depends on public_deps) to have those defined to haul things in for the header files it includes.

Is there a way to define the deps per generated probe file? (with the generated code?)

Open in Gerrit

Related details

Attention is currently required from:

Xiyuan Xia

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

Xiyuan Xia (Gerrit)

unread,

Nov 12, 2024, 12:44:33 PMNov 12

to Andrey Kosyakov, Chromium LUCI CQ, Dave Tapuska, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov and Dave Tapuska

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3:

Dave Tapuska . unresolved

I don't think this is the right fix. I think there is something wrong with the probe DEPs.. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=47?q=instrumentation_probes%20BUILD.gn&ss=chromium

Xiyuan Xia

Thanks for the link.
Please check wheter PS2 make sense. It also fixes the compile error locally for me.

Andrey Kosyakov

I'm not sure the deps Dave is pointing to are wrong. First, at the template level we have no idea what actual probes would be dependent on (this would be determined by the actual `probe_set` argument and thus would be more appropriate at the invoker level ([here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/probe/BUILD.gn;l=80) in this case), and second, if you look at the actual stuff core_probes_impl includes, it's [all within core](https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/blink/renderer/core/core_probes_impl.cc), i.e. should not require an additional dependency. My reading of this error log:
```
In file included from gen/third_party/blink/renderer/core/core_probes_impl.cc:16:
In file included from ../../third_party/blink/renderer/core/inspector/inspector_animation_agent.h:12:
In file included from ../../third_party/blink/renderer/core/animation/scroll_snapshot_timeline.h:15:
In file included from ../../third_party/blink/renderer/core/layout/layout_box.h:46:
In file included from ../../third_party/blink/renderer/core/scroll/scrollbar_theme.h:32:
In file included from ../../third_party/blink/renderer/core/exported/web_view_impl.h:67:
In file included from ../../third_party/blink/renderer/core/frame/resize_viewport_anchor.h:9:
In file included from ../../third_party/blink/renderer/core/page/page.h:50:
../../third_party/blink/renderer/bindings/core/v8/v8_compile_hints_producer.h:8:10: fatal error: 'third_party/blink/renderer/bindings/buildflags.h' file not found
    8 | #include "third_party/blink/renderer/bindings/buildflags.h"
```
 is that the missing dependency is somewhere within the bottom couple of entries.

Dave Tapuska

I think you got to gn describe the core_probes target and see if it has the buildflags as a deps. Liekly it doesn't. We generally don't public_deps the dependencies and that is likely the problem here is that the probes code doesn't explicitly list deps (and incorrectly depends on public_deps) to have those defined to haul things in for the header files it includes.
Is there a way to define the deps per generated probe file? (with the generated code?)

Xiyuan Xia

The missing deps is for build target `//third_party/blink/renderer/core/probe:instrumentation_probes`. As mentioned in the bug, we could repro the problem with a clean build.

`gn path` shows no deps between `instrumentation_probes` and `renderer/bindings:buildflags`.

```
$ gn path $OUT //third_party/blink/renderer/core/probe:instrumentation_probes //third_party/blink/renderer/bindings:buildflags
No non-data paths found between these two targets.
```

I revised the CL to pass in the extra deps to `probe_generator` template separately at the invoker side. Would that look better?

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov
Dave Tapuska

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

Dave Tapuska (Gerrit)

unread,

Nov 12, 2024, 12:54:09 PMNov 12

to Xiyuan Xia, Tricium, Andrey Kosyakov, Chromium LUCI CQ, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov and Xiyuan Xia

Dave Tapuska added 1 comment

Patchset-level comments

File-level comment, Patchset 3:

Dave Tapuska

Thanks this makes sense to me. caseq@ thoughts?

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov
Xiyuan Xia

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

Andrey Kosyakov (Gerrit)

unread,

Nov 12, 2024, 1:43:27 PMNov 12

to Xiyuan Xia, Tricium, Chromium LUCI CQ, Dave Tapuska, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Xiyuan Xia

Andrey Kosyakov voted and added 2 comments

Votes added by Andrey Kosyakov

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 3:

Andrey Kosyakov

Ok, on a closer look we do depend on bindings on core_probes, so this lgtm. I would also vote for landing the original change as a matter of preventing accidental circular header dependencies in the future, including the class definition, especially such a high-level one, just for the sake of a `friend` declaration looks wasteful to me.

File third_party/blink/renderer/core/probe/BUILD.gn

Line 50, Patchset 8 (Latest): deps += invoker.extra_deps

Andrey Kosyakov . unresolved

I think we commonly use just the usual `deps` for that: https://source.chromium.org/search?q=invoker.deps

Open in Gerrit

Related details

Attention is currently required from:

Xiyuan Xia

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

Xiyuan Xia (Gerrit)

unread,

Nov 12, 2024, 2:07:33 PMNov 12

to Andrey Kosyakov, Tricium, Chromium LUCI CQ, Dave Tapuska, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov and Dave Tapuska

Xiyuan Xia voted and added 4 comments

Votes added by Xiyuan Xia

Commit-Queue

+1

4 comments

Patchset-level comments

File-level comment, Patchset 3:

Dave Tapuska . resolved

Xiyuan Xia

Bring back PS1 change to use forward declaration for `friend`.

File-level comment, Patchset 11 (Latest):

Xiyuan Xia . resolved

Lost stamps after uploading new PS. And I need stamps from both you guys.

File third_party/blink/renderer/core/probe/BUILD.gn

Line 48, Patchset 5: "//third_party/blink/renderer/bindings:buildflags",

Andrey Kosyakov . resolved

So where do we actually depend on bindings in the generated code?

Xiyuan Xia

Acknowledged. Discussed in other comments.

Line 50, Patchset 8: deps += invoker.extra_deps

Andrey Kosyakov . resolved

I think we commonly use just the usual `deps` for that: https://source.chromium.org/search?q=invoker.deps

Xiyuan Xia

Done

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov
Dave Tapuska

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

Dave Tapuska (Gerrit)

unread,

Nov 12, 2024, 2:09:02 PMNov 12

to Xiyuan Xia, Andrey Kosyakov, Tricium, Chromium LUCI CQ, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov and Xiyuan Xia

Dave Tapuska voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov
Xiyuan Xia

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

Xiyuan Xia (Gerrit)

unread,

Nov 12, 2024, 4:07:59 PMNov 12

to Dave Tapuska, Andrey Kosyakov, Tricium, Chromium LUCI CQ, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Andrey Kosyakov

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 11 (Latest):

Xiyuan Xia . resolved

Thanks for reviewing. Landing...

Open in Gerrit

Related details

Attention is currently required from:

Andrey Kosyakov

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,

Nov 12, 2024, 4:11:46 PMNov 12

to Xiyuan Xia, Dave Tapuska, Andrey Kosyakov, Tricium, Zhengzheng Liu, Fumitoshi Ukai, chromium...@chromium.org, blink-...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

blink: Fix v8_compile_hints_producer.h compile error

Follow up of http://crrev.com/c/5977022.

Including "web_view_impl.h" in "scrollbar_theme.h" brings in new
deps. The new deps seem to be missing in build gn files. As a
result, compilation occasionally fails with
```


  v8_compile_hints_producer.h:8:10: fatal error:
  'third_party/blink/renderer/bindings/buildflags.h' file

```

This CL adds the missing deps exposed by the above CL.
`instrumentation_probes` -> `blink/renderer/bindings:buildflags`
And also uses forward declaration for `friend`.

Bug: b:367789464, 378381144

Change-Id: I7ed116f98231d02865ee03527edcaf1611233a39

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

Reviewed-by: Dave Tapuska <dtap...@chromium.org>

Commit-Queue: Xiyuan Xia <xiy...@chromium.org>

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

Files:

M third_party/blink/renderer/core/probe/BUILD.gn
M third_party/blink/renderer/core/scroll/scrollbar_theme.h

Change size: XS

Delta: 2 files changed, 7 insertions(+), 1 deletion(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Dave Tapuska

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

blink: Fix v8_compile_hints_producer.h compile error [chromium/src : main]

Xiyuan Xia (Gerrit)

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

1 comment

Related details

Dave Tapuska (Gerrit)

Dave Tapuska added 1 comment

Related details

Xiyuan Xia (Gerrit)

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

1 comment

Related details

Andrey Kosyakov (Gerrit)

Andrey Kosyakov added 2 comments

Related details

Dave Tapuska (Gerrit)

Dave Tapuska added 1 comment

Related details

Xiyuan Xia (Gerrit)

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

1 comment

Related details

Dave Tapuska (Gerrit)

Dave Tapuska added 1 comment

Related details

Andrey Kosyakov (Gerrit)

Andrey Kosyakov voted and added 2 comments

Votes added by Andrey Kosyakov

2 comments

Related details

Xiyuan Xia (Gerrit)

Xiyuan Xia voted and added 4 comments

Votes added by Xiyuan Xia

4 comments

Related details

Dave Tapuska (Gerrit)

Dave Tapuska voted Code-Review+1

Related details

Xiyuan Xia (Gerrit)

Xiyuan Xia voted and added 1 comment

Votes added by Xiyuan Xia

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information