[APC] Trigger content extraction auto-build via DOMContentLoaded [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
WebViewImpl* web_view = document.GetPage()->GetChromeClient().GetWebView();Implicit downcast from 'WebView*' to 'WebViewImpl*' is invalid C++. Use 'static_cast<WebViewImpl*>' if the instance is known to be a WebViewImpl.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
WrapWeakPersistent(this)));Verify namespace for BindOnce. 'PostTask' expects a 'base::OnceClosure', so 'base::BindOnce' is likely intended. 'blink::BindOnce' may not exist or is non-standard.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Commit-Queue | +1 |
2 comments
WebViewImpl* web_view = document.GetPage()->GetChromeClient().GetWebView();Implicit downcast from 'WebView*' to 'WebViewImpl*' is invalid C++. Use 'static_cast<WebViewImpl*>' if the instance is known to be a WebViewImpl.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Invalid: ChromeClient::GetWebView() already returns WebViewImpl* (see third_party/blink/renderer/core/page/chrome_client.h), so
there’s no downcast at third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc.
WrapWeakPersistent(this)));Verify namespace for BindOnce. 'PostTask' expects a 'base::OnceClosure', so 'base::BindOnce' is likely intended. 'blink::BindOnce' may not exist or is non-standard.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
blink::BindOnce is defined in third_party/blink/renderer/platform/wtf/functional.h Invalid: and returns a base::OnceCallback, which satisfies PostTask, so no namespace change needed.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Commit-Queue | +1 |
1 comment
See also the followup CL in https://chromium-review.googlesource.com/c/chromium/src/+/7416700 that fixes the remaining DCHECKs caused by throttling and display locking.
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 7 comments![]( "Open in Gerrit")
#if DCHECK_IS_ON()
// Called by the DOMContentLoaded listener to kick off auto-build, which
// is used by test infrastructure to ensure we run cleanly, without triggering
// crashes or checks/dchecks.
void RunAutoBuildAfterDOMContentLoaded();
#endifCould we put this stuff in another class. Make it a friend class to this one if needed but just to isolate code which is test only.
#if DCHECK_IS_ON()why is this needed? the whole function is already in a DCHECK block.
// tests and browser tests. Blink unit tests assert specific lifecycle
// invariants and are not a good target for this feature. We use a binary-nameI'd like to understand what invariants we're talking about. Maybe there's test only APIs to suppress ones which don't make sense.
I'm also curious to see if this is an issue with the approach of using the lifecycle observer which ensures the lifecycle is in clean state. We shouldn't be hitting invariants in that case.
// invariants and are not a good target for this feature. We use a binary-name
// check (see WebTestSupport) to avoid broad unit-test fallout while keeping
// the coverage benefits for web tests.why are unit-tests hitting this? This is an opt-in triggered by EnableAutomaticActionableExtractionOnPageLoadForTesting. Can't we prevent unit-tests from calling this opt-in?
// When tracing is active (e.g. inspector-protocol tracing tests), auto-build
// can emit list-based HitTest trace events that change the expected trace
// payload shape. Skip auto-build in that case to keep tracing output stable.Sorry what tracing are we talking about here? I'm not following how that impacts the web tests.
if (!AreAllDocumentsReadyForAutoBuild(*document)) {
// Retry after a task turn so we don't re-enter from within lifecycle-
// sensitive scopes. This keeps the auto-build loop simple while waiting
// for lifecycle state to settle.
const bool was_registered = is_lifecycle_observer_registered_;
EnsureLifecycleObserverRegistered();
if (!was_registered) {
LocalFrameView* view = document->View();
Page* page = document->GetPage();
LocalFrame* frame = document->GetFrame();
if (view && page && frame && !page->Animator().IsServicingAnimations()) {
page->Animator().ScheduleVisualUpdate(frame);
}
document->GetTaskRunner(TaskType::kInternalUserInteraction)
->PostTask(FROM_HERE,
blink::BindOnce(
&AIPageContentAgent::RunAutoBuildAfterDOMContentLoaded,
WrapWeakPersistent(this)));
}
return;
}Could we reuse the [LifecycleNotificationObserver](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.h;l=156;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3). It's purpose-built for work that needs to happen when the lifecycle is clean. Seems more robust than the approach here which triggers an animation frame and then posts a task hoping the document state will permit running the lifecycle using `IsDocumentReadyForAutoBuild`.
cc-ing szager who's familiar with the lifecycle stuff and can advise on the right pattern.
// Enable the auto-build path and then simulate DOMContentLoaded dispatch
// still being in progress by forcing the parsing state to "in DCL".
AIPageContentAgent::EnableAutomaticActionableExtractionOnPageLoadForTesting(
*helper_.LocalMainFrame()->GetFrame());This shouldn't be necessary since you're invoking `RunAutoBuildAfterDOMContentLoaded` after this explicitly. In fact the code has a check to ignore this call in unit-test builds?
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 1 comment![]( "Open in Gerrit")
if (!AreAllDocumentsReadyForAutoBuild(*document)) {
// Retry after a task turn so we don't re-enter from within lifecycle-
// sensitive scopes. This keeps the auto-build loop simple while waiting
// for lifecycle state to settle.
const bool was_registered = is_lifecycle_observer_registered_;
EnsureLifecycleObserverRegistered();
if (!was_registered) {
LocalFrameView* view = document->View();
Page* page = document->GetPage();
LocalFrame* frame = document->GetFrame();
if (view && page && frame && !page->Animator().IsServicingAnimations()) {
page->Animator().ScheduleVisualUpdate(frame);
}
document->GetTaskRunner(TaskType::kInternalUserInteraction)
->PostTask(FROM_HERE,
blink::BindOnce(
&AIPageContentAgent::RunAutoBuildAfterDOMContentLoaded,
WrapWeakPersistent(this)));
}
return;
}Could we reuse the [LifecycleNotificationObserver](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.h;l=156;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3). It's purpose-built for work that needs to happen when the lifecycle is clean. Seems more robust than the approach here which triggers an animation frame and then posts a task hoping the document state will permit running the lifecycle using `IsDocumentReadyForAutoBuild`.
cc-ing szager who's familiar with the lifecycle stuff and can advise on the right pattern.
Actually I realized while reviewing [here](https://chromium-review.googlesource.com/c/chromium/src/+/7416700/comment/4cb037eb_29711c34/) why we can't use the lifecycle observer. It's because we trigger invalidation by force unlocking display lock subtrees so we have to do it outside the regular lifecycle.
Then i'm confused why we need the `AreAllDocumentsReadyForAutoBuild` check at all. Assuming we wait for dom content loaded and then post a task, we should always be able to run the extraction in that task. That's the same behaviour as production where the browser's IPC to trigger the extraction can be sent anytime [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=1024;drc=f3b299d131a2d94c2bc1cae88e703f62880cf60d).
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal removed a vote from this change![]( "Open in Gerrit")
Removed Commit-Queue+1 by Aaron Leventhal <aleve...@google.com>
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 7 comments![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Commit-Queue | +1 |
7 comments
#if DCHECK_IS_ON()
// Called by the DOMContentLoaded listener to kick off auto-build, which
// is used by test infrastructure to ensure we run cleanly, without triggering
// crashes or checks/dchecks.
void RunAutoBuildAfterDOMContentLoaded();
#endifCould we put this stuff in another class. Make it a friend class to this one if needed but just to isolate code which is test only.
Done
why is this needed? the whole function is already in a DCHECK block.
Done
// tests and browser tests. Blink unit tests assert specific lifecycle
// invariants and are not a good target for this feature. We use a binary-nameI'd like to understand what invariants we're talking about. Maybe there's test only APIs to suppress ones which don't make sense.
I'm also curious to see if this is an issue with the approach of using the lifecycle observer which ensures the lifecycle is in clean state. We shouldn't be hitting invariants in that case.
Added detailed comment.
// invariants and are not a good target for this feature. We use a binary-name
// check (see WebTestSupport) to avoid broad unit-test fallout while keeping
// the coverage benefits for web tests.why are unit-tests hitting this? This is an opt-in triggered by EnableAutomaticActionableExtractionOnPageLoadForTesting. Can't we prevent unit-tests from calling this opt-in?
Added detailed comment.
Here's why we turn it on carte blanch for all tests via runtime_enabled_features.json5 instead of from an opt in for specific tests: we don't know which tests should opt in, and new tests are added all the time that could break when combined with APC. Eventually the idea is to do what we have for
accessibility, which is to have an FYI bot that turns on AutomaticActionableExtractionOnPageLoadForTesting for all web tests and browser tests. By turning it on carte blanche it casts a very wide net and finds almost all the inconsistencies in the tree building code. Actually, accessibility has a
try bot that is triggered by changes to specific accessibility paths.
However, there didn't seem to be any value in turning autoload on in unit tests, and trying to debug the failing ones didn't seem to have a lot of value. Sometimes the unit tests were specifically working with the document lifecycle. More details
on why some unit tests fail in the comment.
// When tracing is active (e.g. inspector-protocol tracing tests), auto-build
// can emit list-based HitTest trace events that change the expected trace
// payload shape. Skip auto-build in that case to keep tracing output stable.Sorry what tracing are we talking about here? I'm not following how that impacts the web tests.
Added detauked cinnebt,
if (!AreAllDocumentsReadyForAutoBuild(*document)) {
// Retry after a task turn so we don't re-enter from within lifecycle-
// sensitive scopes. This keeps the auto-build loop simple while waiting
// for lifecycle state to settle.
const bool was_registered = is_lifecycle_observer_registered_;
EnsureLifecycleObserverRegistered();
if (!was_registered) {
LocalFrameView* view = document->View();
Page* page = document->GetPage();
LocalFrame* frame = document->GetFrame();
if (view && page && frame && !page->Animator().IsServicingAnimations()) {
page->Animator().ScheduleVisualUpdate(frame);
}
document->GetTaskRunner(TaskType::kInternalUserInteraction)
->PostTask(FROM_HERE,
blink::BindOnce(
&AIPageContentAgent::RunAutoBuildAfterDOMContentLoaded,
WrapWeakPersistent(this)));
}
return;
}Khushal SagarCould we reuse the [LifecycleNotificationObserver](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.h;l=156;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3). It's purpose-built for work that needs to happen when the lifecycle is clean. Seems more robust than the approach here which triggers an animation frame and then posts a task hoping the document state will permit running the lifecycle using `IsDocumentReadyForAutoBuild`.
cc-ing szager who's familiar with the lifecycle stuff and can advise on the right pattern.
Actually I realized while reviewing [here](https://chromium-review.googlesource.com/c/chromium/src/+/7416700/comment/4cb037eb_29711c34/) why we can't use the lifecycle observer. It's because we trigger invalidation by force unlocking display lock subtrees so we have to do it outside the regular lifecycle.
Then i'm confused why we need the `AreAllDocumentsReadyForAutoBuild` check at all. Assuming we wait for dom content loaded and then post a task, we should always be able to run the extraction in that task. That's the same behaviour as production where the browser's IPC to trigger the extraction can be sent anytime [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=1024;drc=f3b299d131a2d94c2bc1cae88e703f62880cf60d).
Fixed
// Enable the auto-build path and then simulate DOMContentLoaded dispatch
// still being in progress by forcing the parsing state to "in DCL".
AIPageContentAgent::EnableAutomaticActionableExtractionOnPageLoadForTesting(
*helper_.LocalMainFrame()->GetFrame());This shouldn't be necessary since you're invoking `RunAutoBuildAfterDOMContentLoaded` after this explicitly. In fact the code has a check to ignore this call in unit-test builds?
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 6 comments![]( "Open in Gerrit")
void AutoBuildHelper::RunAfterDOMContentLoaded() {nit: Define it where the class is declared above?
// invariants and are not a good target for this feature. We use a binary-name
// check (see WebTestSupport) to avoid broad unit-test fallout while keeping
// the coverage benefits for web tests.Aaron Leventhalwhy are unit-tests hitting this? This is an opt-in triggered by EnableAutomaticActionableExtractionOnPageLoadForTesting. Can't we prevent unit-tests from calling this opt-in?
Added detailed comment.
Here's why we turn it on carte blanch for all tests via runtime_enabled_features.json5 instead of from an opt in for specific tests: we don't know which tests should opt in, and new tests are added all the time that could break when combined with APC. Eventually the idea is to do what we have for
accessibility, which is to have an FYI bot that turns on AutomaticActionableExtractionOnPageLoadForTesting for all web tests and browser tests. By turning it on carte blanche it casts a very wide net and finds almost all the inconsistencies in the tree building code. Actually, accessibility has a
try bot that is triggered by changes to specific accessibility paths.However, there didn't seem to be any value in turning autoload on in unit tests, and trying to debug the failing ones didn't seem to have a lot of value. Sometimes the unit tests were specifically working with the document lifecycle. More details
on why some unit tests fail in the comment.
Thanks for the detailed comment. I agree with you about not running it for unit-tests. Do you mind pulling out the unit-test suppression into a separate CL? I'm not a fan of using the test name (we also have blink_platform_unittests not covered right now) for that detection but also don't have a better suggestion. Probably one of the VIRTUAL_OWNERS would know a better way to enable a runtime_enabled_feature only for web tests.
It should also not be an immediate concern. It'll only be when you have to turn it on for all web tests in an FYI bot. Until then it'll be turned on for the content_extration VirtualTestSuite only.
// Confirm the document satisfies the auto-build readiness gates before
// running the deferred task.
EXPECT_TRUE(document->HasFinishedParsing());
EXPECT_FALSE(document->InStyleRecalc());
EXPECT_FALSE(document->InvalidationDisallowed());
EXPECT_FALSE(document->Lifecycle().LifecyclePostponed());
EXPECT_GE(document->Lifecycle().GetState(), DocumentLifecycle::kLayoutClean);
EXPECT_FALSE(document->NeedsLayoutTreeUpdate());
// Ensure there are no child frames or popups that could block readiness.
int child_view_count = 0;
document->View()->ForAllChildLocalFrameViews(
[&](LocalFrameView&) { ++child_view_count; });
EXPECT_EQ(child_view_count, 0);Not necessary anymore
if (base::TrackEvent::IsEnabled()) {
GTEST_SKIP()
<< "Auto-build is disabled while tracing is active in this harness.";
}Is this conditionally true on some bots?
// Re-run the auto-build entry point now that parsing is finished. This should
// synchronously execute the auto-build path without crashing.
agent->RunAutoBuildAfterDOMContentLoadedForTesting();
test::RunPendingTasks();I'm not following how are we validating that this ran the auto-build but the one on l2030 didn't.
TEST_F(AIPageContentAgentTest, AutoBuildDefersDuringStyleRecalc) {This test is no longer necessary.
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 6 comments![]( "Open in Gerrit")
nit: Define it where the class is declared above?
Done
// invariants and are not a good target for this feature. We use a binary-name
// check (see WebTestSupport) to avoid broad unit-test fallout while keeping
// the coverage benefits for web tests.Aaron Leventhalwhy are unit-tests hitting this? This is an opt-in triggered by EnableAutomaticActionableExtractionOnPageLoadForTesting. Can't we prevent unit-tests from calling this opt-in?
Khushal SagarAdded detailed comment.
Here's why we turn it on carte blanch for all tests via runtime_enabled_features.json5 instead of from an opt in for specific tests: we don't know which tests should opt in, and new tests are added all the time that could break when combined with APC. Eventually the idea is to do what we have for
accessibility, which is to have an FYI bot that turns on AutomaticActionableExtractionOnPageLoadForTesting for all web tests and browser tests. By turning it on carte blanche it casts a very wide net and finds almost all the inconsistencies in the tree building code. Actually, accessibility has a
try bot that is triggered by changes to specific accessibility paths.However, there didn't seem to be any value in turning autoload on in unit tests, and trying to debug the failing ones didn't seem to have a lot of value. Sometimes the unit tests were specifically working with the document lifecycle. More details
on why some unit tests fail in the comment.
Thanks for the detailed comment. I agree with you about not running it for unit-tests. Do you mind pulling out the unit-test suppression into a separate CL? I'm not a fan of using the test name (we also have blink_platform_unittests not covered right now) for that detection but also don't have a better suggestion. Probably one of the VIRTUAL_OWNERS would know a better way to enable a runtime_enabled_feature only for web tests.
It should also not be an immediate concern. It'll only be when you have to turn it on for all web tests in an FYI bot. Until then it'll be turned on for the content_extration VirtualTestSuite only.
Done
// Confirm the document satisfies the auto-build readiness gates before
// running the deferred task.
EXPECT_TRUE(document->HasFinishedParsing());
EXPECT_FALSE(document->InStyleRecalc());
EXPECT_FALSE(document->InvalidationDisallowed());
EXPECT_FALSE(document->Lifecycle().LifecyclePostponed());
EXPECT_GE(document->Lifecycle().GetState(), DocumentLifecycle::kLayoutClean);
EXPECT_FALSE(document->NeedsLayoutTreeUpdate());
// Ensure there are no child frames or popups that could block readiness.
int child_view_count = 0;
document->View()->ForAllChildLocalFrameViews(
[&](LocalFrameView&) { ++child_view_count; });
EXPECT_EQ(child_view_count, 0);Aaron LeventhalNot necessary anymore
Done
if (base::TrackEvent::IsEnabled()) {
GTEST_SKIP()
<< "Auto-build is disabled while tracing is active in this harness.";
}Is this conditionally true on some bots?
Removed
// Re-run the auto-build entry point now that parsing is finished. This should
// synchronously execute the auto-build path without crashing.
agent->RunAutoBuildAfterDOMContentLoadedForTesting();
test::RunPendingTasks();I'm not following how are we validating that this ran the auto-build but the one on l2030 didn't.
Ok I've added some assertions that I'll look to improve in the the CL that restores the unit test guard. Added a TODO for that improvement.
TEST_F(AIPageContentAgentTest, AutoBuildDefersDuringStyleRecalc) {This test is no longer necessary.
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar voted Code-Review+1![]( "Open in Gerrit")
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Auto-Submit | +1 |
1 comment
Simplified for now. Looks like we won't turn on auto extraction for browser tests.
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar voted![]( "Open in Gerrit")
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Auto-Submit | +1 |
2 comments
// tests and browser tests. Blink unit tests assert specific lifecycle
// invariants and are not a good target for this feature. We use a binary-nameI'd like to understand what invariants we're talking about. Maybe there's test only APIs to suppress ones which don't make sense.
I'm also curious to see if this is an issue with the approach of using the lifecycle observer which ensures the lifecycle is in clean state. We shouldn't be hitting invariants in that case.
Aaron LeventhalAdded detailed comment.
Done
// When tracing is active (e.g. inspector-protocol tracing tests), auto-build
// can emit list-based HitTest trace events that change the expected trace
// payload shape. Skip auto-build in that case to keep tracing output stable.Aaron LeventhalSorry what tracing are we talking about here? I'm not following how that impacts the web tests.
Added detauked cinnebt,
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[APC] Trigger content extraction auto-build via DOMContentLoaded
Refactor test-only APC auto-build to run after DOMContentLoaded and
only when documents are lifecycle-safe. The new readiness gate checks
parsing finished, style recalc, invalidation, and layout-clean states
across the main document, local child frames, and popups. Auto-build
also skips the initial empty document and avoids running inside blink
unit test binaries.
Add DCHECK-only unit tests that verify auto-build defers while parsing
or style recalc is in progress, and remove the update-data crash
expectations for the auto-build web test flag.
This change fixes at least one lifecycle-related issue in web tests
and gets us closer to being able to turn on
AIPageContentBuildOnLoadForTesting for an FYI bot and stay green.
More fixes are necessary via future CLs as there continue to be flakes
with geometry checks when auto build is turned on, likely caused by the
difference between the layout viewport and visual viewport.
Test: AIPageContentAgentTest.AutoBuildDefersUntilParsingFinished:
AIPageContentAgentTest.AutoBuildDefersDuringStyleRecalc
fast/dom/HTMLObjectElement/update-data.html
- M third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc
- M third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.h
- M third_party/blink/renderer/modules/content_extraction/ai_page_content_agent_unittest.cc
- M third_party/blink/web_tests/FlagExpectations/content-extraction
- M third_party/blink/web_tests/TestExpectations
Code-Review: +1 by Khushal Sagar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
#include "third_party/blink/public/mojom/content_extraction/ai_page_content.mojom-data-view.h"this change was not necessary as you are not using any dataview types directly - can you let me know which editors or iwyu steps you are using as this should not be happening? (I'll upload a fix - but I'd like to know why these are creeping in!)
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |