Cache option's ancestor optgroup [chromium/src : main]
David Baron (Gerrit)
David Baron added 6 comments![]( "Open in Gerrit")
This mostly looks good, but a few comments below. I'm pushing a little bit on code simplicity here because this code has tended to accumulate unneeded complexity in the past, so I'd like to try to push to simplifying where it's easy to do so.
This patch also updates some apperance:auto text indentation for optionsPlease fix this WARNING reported by Spellchecker: "apperance" is a possible misspelling of "appearance".
To bypass Spellchecker, ...
"apperance" is a possible misspelling of "appearance".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
nearest_ancestor_select_ = select;I think at this point `SetOwnerSelectElement` makes the code more confusing rather than less. In paricular, it requires that the reader of the code understand that `nearest_ancestor_optgroup_` and `nearest_ancestor_select_` are actually both set when this function is called.
I think at this point it's probably beneficial to just remove the method and inline it into the two callers (`InsertedInto` and `RemovedFrom`, which you're both already modifying). I don't think you need the `DCHECK()` anymore since both callers are just setting `nearest_ancestor_select_` based on the result of the method.
auto select_and_optgroup =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
HTMLSelectElement* new_ancestor_select = select_and_optgroup.first;
SetOwnerSelectElement(new_ancestor_select);
nearest_ancestor_optgroup_ = select_and_optgroup.second;Optional comment, but I think it's perhaps a little more readable if you use `std::tie()`:
```suggestion
HTMLSelectElement* new_ancestor_select;
std::tie(new_ancestor_select, nearest_ancestor_optgroup_) =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
SetOwnerSelectElement(new_ancestor_select);
```
and a little bit more so if you also take my suggestion above about dropping `SetOwnerSelectElement` since then it just becomes:
```suggestion
std::tie(nearest_ancestor_select_, nearest_ancestor_optgroup_) =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
```
auto select_and_optgroup =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
nearest_ancestor_optgroup_ = select_and_optgroup.second;
HTMLSelectElement* new_ancestor_select = select_and_optgroup.first;
HTMLSelectElement* old_ancestor_select = nearest_ancestor_select_;Same advice here about `std::tie()` and dropping `SetOwnerSelectElement`... just make sure to assign to `old_ancestor_select` first!
NearestAncestorSelectNoNesting(const Element& element);Maybe this should be renamed at this point? Maybe something like `AssociatedAncestorSelectAndOptgroup` or just `AssociatedSelectAndOptgroup`?
Related details
- Joey Arhar
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. |
Joey Arhar (Gerrit)
Joey Arhar added 5 comments![]( "Open in Gerrit")
This patch also updates some apperance:auto text indentation for optionsPlease fix this WARNING reported by Spellchecker: "apperance" is a possible misspelling of "appearance".
To bypass Spellchecker, ...
"apperance" is a possible misspelling of "appearance".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
nearest_ancestor_select_ = select;I think at this point `SetOwnerSelectElement` makes the code more confusing rather than less. In paricular, it requires that the reader of the code understand that `nearest_ancestor_optgroup_` and `nearest_ancestor_select_` are actually both set when this function is called.
I think at this point it's probably beneficial to just remove the method and inline it into the two callers (`InsertedInto` and `RemovedFrom`, which you're both already modifying). I don't think you need the `DCHECK()` anymore since both callers are just setting `nearest_ancestor_select_` based on the result of the method.
i agree, i deleted this method
auto select_and_optgroup =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
HTMLSelectElement* new_ancestor_select = select_and_optgroup.first;
SetOwnerSelectElement(new_ancestor_select);
nearest_ancestor_optgroup_ = select_and_optgroup.second;Optional comment, but I think it's perhaps a little more readable if you use `std::tie()`:
```suggestion
HTMLSelectElement* new_ancestor_select;
std::tie(new_ancestor_select, nearest_ancestor_optgroup_) =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
SetOwnerSelectElement(new_ancestor_select);
```
and a little bit more so if you also take my suggestion above about dropping `SetOwnerSelectElement` since then it just becomes:
```suggestion
std::tie(nearest_ancestor_select_, nearest_ancestor_optgroup_) =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
```
Done
auto select_and_optgroup =
HTMLSelectElement::NearestAncestorSelectNoNesting(*this);
nearest_ancestor_optgroup_ = select_and_optgroup.second;
HTMLSelectElement* new_ancestor_select = select_and_optgroup.first;
HTMLSelectElement* old_ancestor_select = nearest_ancestor_select_;Same advice here about `std::tie()` and dropping `SetOwnerSelectElement`... just make sure to assign to `old_ancestor_select` first!
Done
NearestAncestorSelectNoNesting(const Element& element);Maybe this should be renamed at this point? Maybe something like `AssociatedAncestorSelectAndOptgroup` or just `AssociatedSelectAndOptgroup`?
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
AssociatedSelectAndOptgroup(const Element& element);nit: The parameter name 'element' can be omitted from the function declaration in the header file, as its purpose is clear from the type 'Element'. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **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
- 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. |
David Baron (Gerrit)
David Baron voted Code-Review+1![]( "Open in Gerrit")
- Joey Arhar
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. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
AssociatedSelectAndOptgroup(const Element& element);nit: The parameter name 'element' can be omitted from the function declaration in the header file, as its purpose is clear from the type 'Element'. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis 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)_
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/html/forms/html_select_element.h
Insertions: 1, Deletions: 1.
@@ -170,7 +170,7 @@
// there is an <optgroup> in between the provided element and the returned
// <select>.
static std::pair<HTMLSelectElement*, HTMLOptGroupElement*>
- AssociatedSelectAndOptgroup(const Element& element);
+ AssociatedSelectAndOptgroup(const Element&);
void AccessKeyAction(SimulatedClickCreationScope creation_scope) override;
void SelectOptionByAccessKey(HTMLOptionElement*);
```
Change information
Cache option's ancestor optgroup
Now that we are using the nearest ancestor optgroup to change some
behaviors, it makes sense to cache it during the same tree traversal
that we are already doing for the nearest ancestor select element.
This patch also updates some appearance:auto text indentation for
options when they are inside an optgroup to look for an ancestor
optgroup rather than a parent optgroup.
- M third_party/blink/renderer/core/html/forms/html_opt_group_element.cc
- M third_party/blink/renderer/core/html/forms/html_option_element.cc
- M third_party/blink/renderer/core/html/forms/html_option_element.h
- M third_party/blink/renderer/core/html/forms/html_select_element.cc
- M third_party/blink/renderer/core/html/forms/html_select_element.h
- M third_party/blink/renderer/core/html/forms/option_list.cc
- M third_party/blink/renderer/core/html/html_hr_element.cc
- M third_party/blink/web_tests/fast/forms/select/optgroup-rendering.html
Code-Review: +1 by David Baron
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |