This is the first time I hear about this new work and it sounds fairly complex. Is there a design doc I can read up on other than the GitHub issue?
Just to set expectations, it will take me more than 24 hours to review this.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
This is the first time I hear about this new work and it sounds fairly complex. Is there a design doc I can read up on other than the GitHub issue?
Just to set expectations, it will take me more than 24 hours to review this.
I've shared the internal design document with you (I find that sharing Google docs here brings a lot of spam). No rush, thanks for taking a look.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam RosenthalThis is the first time I hear about this new work and it sounds fairly complex. Is there a design doc I can read up on other than the GitHub issue?
Just to set expectations, it will take me more than 24 hours to review this.
I've shared the internal design document with you (I find that sharing Google docs here brings a lot of spam). No rush, thanks for taking a look.
PTAL
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
#include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"Some of these #includes look unnecessary (like this one)
ScriptState* script_state = ToScriptStateForMainWorld(DomWindow());Nit: Pass in, since ethe caller has it?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
~NavigationAPICommitDeferringCondition() {}Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
// This is called by the navigation API after author-provided conditions are met.Line exceeds max characters.
WPT should be `tentative`, right?
Done
// Copyright 2025 The Chromium AuthorsNoam Rosenthal2026
Done
// Copyright 2025 The Chromium AuthorsNoam Rosenthal2026
Done
Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Acknowledged
// This is called by the navigation API after author-provided conditions are met.Noam RosenthalLine exceeds max characters.
Done
#include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"Some of these #includes look unnecessary (like this one)
Done
ScriptState* script_state = ToScriptStateForMainWorld(DomWindow());Nit: Pass in, since ethe caller has it?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam RosenthalWPT should be `tentative`, right?
Done
I don't think the WPT got marked tentative?
ScriptState*);Micro-nit: I believe it's convention to pass the `ScriptState*` as the first parameter (though it's only strictly enforced at the bindings layer).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam RosenthalWPT should be `tentative`, right?
Nate ChapinDone
I don't think the WPT got marked tentative?
Oops, marked this done prematurely. Done now.
Micro-nit: I believe it's convention to pass the `ScriptState*` as the first parameter (though it's only strictly enforced at the bindings layer).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hey Noam, we spent quite a bit of time yesterday discussing this work and it created a lot more questions than answers. Someone from the CSA team will respond to the email thread shortly where we can discuss and answer some of the open questions.
This CL itself is in good shape, but since the overall design has open questions, I would defer from approving it until we get a bit more clarity.