Restrict open-on-interest for <menuitem>s in <menubar>. [chromium/src : main]
David Baron (Gerrit)
Message from David Baron![]( "Open in Gerrit")
Set Ready For Review
Related details
- Mason Freed
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. |
David Baron (Gerrit)
David Baron added 2 comments![]( "Open in Gerrit")
David BaronI'm curious if you think this is too weird -- especially in the context of what we're going to need to standandize. (I'm not quite sure how many of the details of the interest stuff that this is modifying are standardized versus how many are not.)
Done
auto is_open_menuitem = [](Node& n) -> bool {David Baronnit: Inconsistent variable naming. The variable 'menuitem' should be named 'menu_item' to match the convention used in the surrounding code (e.g., line 12373) and the file name 'html_menu_item_element.h'. Consider renaming the lambda 'is_open_menuitem' to 'is_menu_item_open' as well for consistency.
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)_
David BaronThis is at least partly invalid (reordering open versus menuitem), since the lambda is definitely about "is it a menuitem and is it open" and not just "is the menuitem open". Given that the menuitem part is about whether it's a `<menuitem>` element I'm inclined to not insert the underscore.
code in question is now gone
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
The single unittest failure here suggests that maybe this isn't a good idea. Though I'm pretty surprised that it's just *one* unittest that's affected. I'm curious if using `:has(> ...)` like this is something we need to avoid, or whether you think there's a reasonable path to making it allowable.
Related details
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 5 comments![]( "Open in Gerrit")
Looks good! I like this patch better than the prior approach, for sure.
menubar:has(> menuitem:open) > menuitem {Did we decide that the menuitem has to be a direct child?
test needs to wait.nit: perhaps `s/wait/use explicit delays` or something like that? This sounds like we need to wait to write some part of the test or something.
await t.step_wait(() => element.matches(":interest-source"), 2000)These are very long delays - any way to shorten them?
Also, I know step_wait multiplies by the overall test timeout, but should this test be marked with `<meta name="timeout" content="long" />`?
await wait_for_no_interest(t);Perhaps just inline this, to avoid the temptation to use it again somewhere else?
Related details
- David Baron
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. |