Move OptionList to template class [chromium/src : main]
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
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 3 comments![]( "Open in Gerrit")
HTMLSelectElement* OwnerElement() const { return nearest_ancestor_select_; }This seems less than ideal -- having `OwnerElement` paired with `OwnerDataListElement` seems a bit confusing. I think it would be nice to still write `OwnerSelectElement` at the bulk of the callers spread around the codebase.
I can see two options for how to do this:
(1) just add a third method like `OwnerElementForList` or maybe just `OwnerElement`, and only use it in the `OptionList` code, or
(2) make the method an additional template parameter to `ElementList`
template <typename OwnerType, typename T>here (and above) does it make sense to use `ItemType` rather than just `T`?
enum class ElementListIteratorStartingPoint {Is there a reason this needs to move outside the `ElementListIterator` class?
(Is it fixing something that could have been fixed by adding the `typename` keyword someplace?)
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 3 comments![]( "Open in Gerrit")
HTMLSelectElement* OwnerElement() const { return nearest_ancestor_select_; }This seems less than ideal -- having `OwnerElement` paired with `OwnerDataListElement` seems a bit confusing. I think it would be nice to still write `OwnerSelectElement` at the bulk of the callers spread around the codebase.
I can see two options for how to do this:
(1) just add a third method like `OwnerElementForList` or maybe just `OwnerElement`, and only use it in the `OptionList` code, or
(2) make the method an additional template parameter to `ElementList`
I renamed OwnerElement to OwnerElementForList, I kind of liked that better than having duplicate methods although maybe its harder to read in other places now
template <typename OwnerType, typename T>here (and above) does it make sense to use `ItemType` rather than just `T`?
Done
enum class ElementListIteratorStartingPoint {Is there a reason this needs to move outside the `ElementListIterator` class?
(Is it fixing something that could have been fixed by adding the `typename` keyword someplace?)
it was because i had to add template arguments to usages of the class namespace in other places so I thought it would be better to just move it outside of the class, but i moved it back in and added templates arguments to the other references to make it work
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 and added 2 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
2 comments
HTMLSelectElement* OwnerElement() const { return nearest_ancestor_select_; }Joey ArharThis seems less than ideal -- having `OwnerElement` paired with `OwnerDataListElement` seems a bit confusing. I think it would be nice to still write `OwnerSelectElement` at the bulk of the callers spread around the codebase.
I can see two options for how to do this:
(1) just add a third method like `OwnerElementForList` or maybe just `OwnerElement`, and only use it in the `OptionList` code, or
(2) make the method an additional template parameter to `ElementList`
I renamed OwnerElement to OwnerElementForList, I kind of liked that better than having duplicate methods although maybe its harder to read in other places now
OK, not quite what I was hoping for but I guess it's okay. (I'd have preferred just having duplicate one-line methods so the bulk of the callers could still say `OwnerSelectElement`.)
Joey ArharIs there a reason this needs to move outside the `ElementListIterator` class?
(Is it fixing something that could have been fixed by adding the `typename` keyword someplace?)
it was because i had to add template arguments to usages of the class namespace in other places so I thought it would be better to just move it outside of the class, but i moved it back in and added templates arguments to the other references to make it work
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
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")
HTMLSelectElement* OwnerElement() const { return nearest_ancestor_select_; }Joey ArharThis seems less than ideal -- having `OwnerElement` paired with `OwnerDataListElement` seems a bit confusing. I think it would be nice to still write `OwnerSelectElement` at the bulk of the callers spread around the codebase.
I can see two options for how to do this:
(1) just add a third method like `OwnerElementForList` or maybe just `OwnerElement`, and only use it in the `OptionList` code, or
(2) make the method an additional template parameter to `ElementList`
David BaronI renamed OwnerElement to OwnerElementForList, I kind of liked that better than having duplicate methods although maybe its harder to read in other places now
OK, not quite what I was hoping for but I guess it's okay. (I'd have preferred just having duplicate one-line methods so the bulk of the callers could still say `OwnerSelectElement`.)
ok, i made duplicate methods and reverted changes which renamed from ownerselectelement to ownerelementforlist
Related details
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 voted Code-Review+1![]( "Open in Gerrit")
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |