Implement GeometryUtil mixin and plug it in for CSSPseudoElement [chromium/src : main]
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
Hey Kent, could you, please, have a look at the geometry part of CL?
And Mason, please, for the DOM and IDL review.
Related details
- Kent Tamura
- 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. |
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
// GeometryUtils mixinDoes this CL implement `GeometryUtils` as methods, not a real `mixin`, intentionally?
Related details
- Daniil Sakhapov
- Mason Freed
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
1 comment
// GeometryUtils mixinDoes this CL implement `GeometryUtils` as methods, not a real `mixin`, intentionally?
It's just me not knowing IDL, sorry 😊 fixed now, ptal!
Related details
- Kent Tamura
- 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 10 comments![]( "Open in Gerrit")
enum CSSBoxType { "margin", "border", "padding", "content" };This could be part of `convert_coordinate_options.idl` if you want.
// https://drafts.csswg.org/cssom-view/#extension-to-the-geometryutils-interfaceIs this link valid? Seems like maybe you want this instead?
https://drafts.csswg.org/cssom-view/#the-geometryutils-interface
// Descend from the originating Element through the collected pseudo-ids in
// reverse order to resolve nested pseudo-elements.I read this chunk of code a few times. I think it's correct. But I think this comment could be expanded to explain what's going on in this loop.
return GeometryUtils::ConvertQuadFromNode(quad, from, options,Can you add this to all of these methods?
```
CHECK(RuntimeEnabledFeatures::GeometryUtilsForCSSPseudoElementEnabled());
```
typedef (Text or Element or CSSPseudoElement or Document) GeometryNode;nit: maybe this could just be defined in `geometry_utils.idl`? Also ok to leave it here.
const ConvertCoordinateOptions* options,I guess the same is true for `ConvertCoordinateOptions`.
const V8UnionCSSPseudoElementOrDocumentOrElementOrText* from,It feels really odd to be passing these around. It makes this "utils" class seem like it only works for one very specific case - the one you're adding here. Other than CSSPseudoElement, this could just be `Node *`. Perhaps there's a way to make it work? Like have an overload for the `CSSPseudoElement` case? Or something?
LayoutObject* GetLayoutObjectFromGeometryNode(Perhaps the fix is to move this up into the caller, and pass around layout objects instead of the V8 union?
if (options && options->hasBox()) {
box_type = options->box().AsEnum();
}
LayoutObject* relative_to = nullptr;
if (options && options->hasRelativeTo()) {
relative_to = GetLayoutObjectFromGeometryNode(options->relativeTo());
}And options is only used here, so maybe just pass in these two variables?
// GeometryUtils mixin.
// https://drafts.csswg.org/cssom-view/#the-geometryutils-interface
name: "GeometryUtils",
status: "experimental",
},
{
// GeometryUtils for CSSPseudoElement interface.
// https://www.w3.org/TR/cssom-view-1/#the-geometryutils-interface
name: "GeometryUtilsForCSSPseudoElement",
depends_on: ["CSSPseudoElementInterface"],
status: "experimental",
},Why two flags for this?
Related details
- Daniil Sakhapov
- Kent Tamura
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. |
Kent Tamura (Gerrit)
Kent Tamura added 2 comments![]( "Open in Gerrit")
// GeometryUtils mixinDaniil SakhapovDoes this CL implement `GeometryUtils` as methods, not a real `mixin`, intentionally?
It's just me not knowing IDL, sorry 😊 fixed now, ptal!
Ack
namespace GeometryUtils {https://google.github.io/styleguide/cppguide.html#Namespace_Names
> Namespace names are snake_case (all lowercase, with underscores between words).
Related details
- Daniil Sakhapov
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. |
Kent Tamura (Gerrit)
Kent Tamura added 3 comments![]( "Open in Gerrit")
double p1_x = quad->hasP1() ? quad->p1()->x() : 0;
double p1_y = quad->hasP1() ? quad->p1()->y() : 0;
double p2_x = quad->hasP2() ? quad->p2()->x() : 0;nit:
```c++
auto p1 = quad->hasP1() ? gfx::PointF(quad->p1()->x(), quad->p1()->y()) : gfx::PointF();
```
might be cleaner.
// Convert from source to absolute, then from absolute to target.How about adding a function to convert a `gfx::PointF` to a `DOMPointInit`?
```c++
auto convert = [&](const gfx::PointF p) {
gfx::PointF absolute = source_layout->LocalToAbsolutePoint(p);
gfx::PointF shifted = absolute - target_origin.OffsetFromOrigin();
DOMPointInit* dom_point = DOMPointInit::Create();
dom_point->setX(shifted.x());
dom_point->setY(shifted.y());
return dom_point;
};
DOMPointInit* dom_p1 = convert(p1);
```
gfx::PointF abs_p1 = source_layout->LocalToAbsolutePoint(gfx::PointF(x, y));Ditto.
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 15 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
Hey Kent, could you, please, have a look at the geometry part of CL?
And Mason, please, for the DOM and IDL review.
Done
enum CSSBoxType { "margin", "border", "padding", "content" };This could be part of `convert_coordinate_options.idl` if you want.
Done
// https://drafts.csswg.org/cssom-view/#extension-to-the-geometryutils-interfaceIs this link valid? Seems like maybe you want this instead?
https://drafts.csswg.org/cssom-view/#the-geometryutils-interface
Done
// Descend from the originating Element through the collected pseudo-ids in
// reverse order to resolve nested pseudo-elements.I read this chunk of code a few times. I think it's correct. But I think this comment could be expanded to explain what's going on in this loop.
Done
return GeometryUtils::ConvertQuadFromNode(quad, from, options,Can you add this to all of these methods?
```
CHECK(RuntimeEnabledFeatures::GeometryUtilsForCSSPseudoElementEnabled());
```
Done
typedef (Text or Element or CSSPseudoElement or Document) GeometryNode;nit: maybe this could just be defined in `geometry_utils.idl`? Also ok to leave it here.
Done
I guess the same is true for `ConvertCoordinateOptions`.
Done
const V8UnionCSSPseudoElementOrDocumentOrElementOrText* from,It feels really odd to be passing these around. It makes this "utils" class seem like it only works for one very specific case - the one you're adding here. Other than CSSPseudoElement, this could just be `Node *`. Perhaps there's a way to make it work? Like have an overload for the `CSSPseudoElement` case? Or something?
Done
https://google.github.io/styleguide/cppguide.html#Namespace_Names
> Namespace names are snake_case (all lowercase, with underscores between words).
Done
Perhaps the fix is to move this up into the caller, and pass around layout objects instead of the V8 union?
Done
if (options && options->hasBox()) {
box_type = options->box().AsEnum();
}
LayoutObject* relative_to = nullptr;
if (options && options->hasRelativeTo()) {
relative_to = GetLayoutObjectFromGeometryNode(options->relativeTo());
}And options is only used here, so maybe just pass in these two variables?
Done
double p1_x = quad->hasP1() ? quad->p1()->x() : 0;
double p1_y = quad->hasP1() ? quad->p1()->y() : 0;
double p2_x = quad->hasP2() ? quad->p2()->x() : 0;nit:
```c++
auto p1 = quad->hasP1() ? gfx::PointF(quad->p1()->x(), quad->p1()->y()) : gfx::PointF();
```
might be cleaner.
Done
// Convert from source to absolute, then from absolute to target.How about adding a function to convert a `gfx::PointF` to a `DOMPointInit`?
```c++
auto convert = [&](const gfx::PointF p) {
gfx::PointF absolute = source_layout->LocalToAbsolutePoint(p);
gfx::PointF shifted = absolute - target_origin.OffsetFromOrigin();
DOMPointInit* dom_point = DOMPointInit::Create();
dom_point->setX(shifted.x());
dom_point->setY(shifted.y());
return dom_point;
};
DOMPointInit* dom_p1 = convert(p1);
```
Done
gfx::PointF abs_p1 = source_layout->LocalToAbsolutePoint(gfx::PointF(x, y));Daniil SakhapovDitto.
Done
// GeometryUtils mixin.
// https://drafts.csswg.org/cssom-view/#the-geometryutils-interface
name: "GeometryUtils",
status: "experimental",
},
{
// GeometryUtils for CSSPseudoElement interface.
// https://www.w3.org/TR/cssom-view-1/#the-geometryutils-interface
name: "GeometryUtilsForCSSPseudoElement",
depends_on: ["CSSPseudoElementInterface"],
status: "experimental",
},Why two flags for this?
One for GeometryUtils interface, one for using it on CSSPseudoElement
Related details
- Kent Tamura
- 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 4 comments![]( "Open in Gerrit")
// Descend from the originating Element through the collected pseudo-ids in
// reverse order to resolve nested pseudo-elements.Daniil SakhapovI read this chunk of code a few times. I think it's correct. But I think this comment could be expanded to explain what's going on in this loop.
Done
Thanks... but. I still don't quite understand. In the `owner::before::marker` example, if `parent` starts at the `::marker`, then you'll build `pseudo_chain` as [`::marker','::before']. Then the second loop (the one in question here) will walk first `::before` and then `::marker`, and will return `::marker`. Why walk up and back just to get where you started?
CORE_EXPORT DOMQuad* ConvertQuadFromNode(DOMQuadInit* quad,Along the same lines, it feels like you should be passing around `DOMQuad` (and `DOMPoint` below) rather than the IDL `Init` types, right?
LayoutObject* GetLayoutObjectFromGeometryNode(Daniil SakhapovPerhaps the fix is to move this up into the caller, and pass around layout objects instead of the V8 union?
Done
I feel like this utility should also be sitting in css_pseudo_element.cc rather than here. I have a hard time believing there will ever be another `V8UnionCSSPseudoElementOrDocumentOrElementOrText` somewhere else in our codebase. Perhaps, but maybe we can refactor it back to somewhere general when that second case shows up?
// GeometryUtils mixin.
// https://drafts.csswg.org/cssom-view/#the-geometryutils-interface
name: "GeometryUtils",
status: "experimental",
},
{
// GeometryUtils for CSSPseudoElement interface.
// https://www.w3.org/TR/cssom-view-1/#the-geometryutils-interface
name: "GeometryUtilsForCSSPseudoElement",
depends_on: ["CSSPseudoElementInterface"],
status: "experimental",
},Daniil SakhapovWhy two flags for this?
One for GeometryUtils interface, one for using it on CSSPseudoElement
- Daniil Sakhapov
- Kent Tamura
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. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
No additional comments from me. It's ok to merge this after resolving Mason's comments.
Related details
- Daniil Sakhapov
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. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
3 comments
// Descend from the originating Element through the collected pseudo-ids in
// reverse order to resolve nested pseudo-elements.Daniil SakhapovI read this chunk of code a few times. I think it's correct. But I think this comment could be expanded to explain what's going on in this loop.
Mason FreedDone
Thanks... but. I still don't quite understand. In the `owner::before::marker` example, if `parent` starts at the `::marker`, then you'll build `pseudo_chain` as [`::marker','::before']. Then the second loop (the one in question here) will walk first `::before` and then `::marker`, and will return `::marker`. Why walk up and back just to get where you started?
So CSSPseudoElement is a proxy representation of PseudoElement, which always exist.
Getting from CSSPseudoElement to PseudoElement is only possible (in Blink) via (Element/PseudoElement).GetCSSPseudoElement(pseudo_id).
Here we recieve CSSPseudoElement from JS, so to get a nested PseudoElement we need to go to the ultimate originating element (Element), while keeping pseudo_ids from the nested chain (of CSSPseudoElements, as `.parent` of CSSPseudoElement returns either Element or CSSPseudoElement).
Once we know that chain, we just descend back, but getting PseudoElements on our way back with those pseudo_ids we got from CSSPseudoElement.
CORE_EXPORT DOMQuad* ConvertQuadFromNode(DOMQuadInit* quad,Along the same lines, it feels like you should be passing around `DOMQuad` (and `DOMPoint` below) rather than the IDL `Init` types, right?
Done
LayoutObject* GetLayoutObjectFromGeometryNode(Daniil SakhapovPerhaps the fix is to move this up into the caller, and pass around layout objects instead of the V8 union?
Mason FreedDone
I feel like this utility should also be sitting in css_pseudo_element.cc rather than here. I have a hard time believing there will ever be another `V8UnionCSSPseudoElementOrDocumentOrElementOrText` somewhere else in our codebase. Perhaps, but maybe we can refactor it back to somewhere general when that second case shows up?
But GeometryUtils is included also for Element, Text and Document, right?
Meaning those would need to transform V8UnionCSSPseudoElementOrDocumentOrElementOrText into LayoutObject before calling the main functions as well?
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 2 comments![]( "Open in Gerrit")
// Descend from the originating Element through the collected pseudo-ids in
// reverse order to resolve nested pseudo-elements.Daniil SakhapovI read this chunk of code a few times. I think it's correct. But I think this comment could be expanded to explain what's going on in this loop.
Mason FreedDone
Daniil SakhapovThanks... but. I still don't quite understand. In the `owner::before::marker` example, if `parent` starts at the `::marker`, then you'll build `pseudo_chain` as [`::marker','::before']. Then the second loop (the one in question here) will walk first `::before` and then `::marker`, and will return `::marker`. Why walk up and back just to get where you started?
So CSSPseudoElement is a proxy representation of PseudoElement, which always exist.
Getting from CSSPseudoElement to PseudoElement is only possible (in Blink) via (Element/PseudoElement).GetCSSPseudoElement(pseudo_id).
Here we recieve CSSPseudoElement from JS, so to get a nested PseudoElement we need to go to the ultimate originating element (Element), while keeping pseudo_ids from the nested chain (of CSSPseudoElements, as `.parent` of CSSPseudoElement returns either Element or CSSPseudoElement).
Once we know that chain, we just descend back, but getting PseudoElements on our way back with those pseudo_ids we got from CSSPseudoElement.
Thank you! Great explanation. (It's easy to forget the distinction between "CSSPseudoElement" and "a CSS PseudoElement".) Can you include some of that in the comment here? And then I think I'll be happy with this.
LayoutObject* GetLayoutObjectFromGeometryNode(Daniil SakhapovPerhaps the fix is to move this up into the caller, and pass around layout objects instead of the V8 union?
Mason FreedDone
Daniil SakhapovI feel like this utility should also be sitting in css_pseudo_element.cc rather than here. I have a hard time believing there will ever be another `V8UnionCSSPseudoElementOrDocumentOrElementOrText` somewhere else in our codebase. Perhaps, but maybe we can refactor it back to somewhere general when that second case shows up?
But GeometryUtils is included also for Element, Text and Document, right?
Meaning those would need to transform V8UnionCSSPseudoElementOrDocumentOrElementOrText into LayoutObject before calling the main functions as well?
It feels ok to have a helper somewhere that does the conversion. It just feels very strange to have a "utils" class that only works for a **very** specific union of IDL types. Like what if a new caller later has a `Node*` and they want to call one of these. What do they do, construct the `V8UnionCSSPseudoElementOrDocumentOrElementOrText` and pass that? Feels odd.
Related details
- Daniil Sakhapov
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. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
LayoutObject* GetLayoutObjectFromGeometryNode(Daniil SakhapovPerhaps the fix is to move this up into the caller, and pass around layout objects instead of the V8 union?
Mason FreedDone
Daniil SakhapovI feel like this utility should also be sitting in css_pseudo_element.cc rather than here. I have a hard time believing there will ever be another `V8UnionCSSPseudoElementOrDocumentOrElementOrText` somewhere else in our codebase. Perhaps, but maybe we can refactor it back to somewhere general when that second case shows up?
Mason FreedBut GeometryUtils is included also for Element, Text and Document, right?
Meaning those would need to transform V8UnionCSSPseudoElementOrDocumentOrElementOrText into LayoutObject before calling the main functions as well?
It feels ok to have a helper somewhere that does the conversion. It just feels very strange to have a "utils" class that only works for a **very** specific union of IDL types. Like what if a new caller later has a `Node*` and they want to call one of these. What do they do, construct the `V8UnionCSSPseudoElementOrDocumentOrElementOrText` and pass that? Feels odd.
But that is actually a helper to go from the V8Union* to LayoutObject*.
And all the real GeomUtil API take LayoutObject*, not the V8Union*?
So new clients would provide LayoutObject* and there would be no need for this helper for them?
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. |