Add options to fillTextCluster to override some cluster values [chromium/src : main]
Andres Ricardo Perez (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Fernando Serboncini
- Jean-Philippe Gravel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Serboncini (Gerrit)
Fernando Serboncini added 2 comments![]( "Open in Gerrit")
DrawTextInternal(fillTextCluster(text_cluster, x, y, nullptr);
if (cluster_options == nullptr) {TextCluster* whatever = text_cluster;
if (!nullptr) { whatever = MakeGC<TextCluster>(*text_cluster); whatever.updateWithOptions(options); }
drawtextinternal....
Related details
- Andres Ricardo Perez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez added 2 comments![]( "Open in Gerrit")
DrawTextInternal(Andres Ricardo PerezfillTextCluster(text_cluster, x, y, nullptr);
Done
TextCluster* whatever = text_cluster;
if (!nullptr) { whatever = MakeGC<TextCluster>(*text_cluster); whatever.updateWithOptions(options); }drawtextinternal....
- Fernando Serboncini
- Jean-Philippe Gravel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Serboncini (Gerrit)
Fernando Serboncini added 3 comments![]( "Open in Gerrit")
TextCluster::TextCluster(const TextCluster& other)can't you just use the default copy constructor here?
TextCluster::TextCluster(const TextCluster&) = default;
void TextCluster::ApplyFillTextClusterOptions(const FillTextClusterOptions* options) {if you are going to pass a pointer, you need a "if (!options) return" in the beginning. Otherwise, pass a reference.
dictionary GetTextClusterOptions {could we merge those two as TextClusterOptions?
The align/baseline are the same (btw, Are the "?=null" necessary?)
It would be reasonable to either ignore the x,y or add some meaning to it on the getTextCluster)
Related details
- Andres Ricardo Perez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez added 3 comments![]( "Open in Gerrit")
TextCluster::TextCluster(const TextCluster& other)can't you just use the default copy constructor here?
TextCluster::TextCluster(const TextCluster&) = default;
The compiler doesn't allow it as `TextCluster` inherits from `ScriptWrappable`, which deletes the default copy constructor here: https://crsrc.org/c/third_party/blink/renderer/platform/bindings/script_wrappable.h;l=102
Do you know of a workaround or should I leave the constructor as is?
void TextCluster::ApplyFillTextClusterOptions(const FillTextClusterOptions* options) {if you are going to pass a pointer, you need a "if (!options) return" in the beginning. Otherwise, pass a reference.
Made it a reference. Thanks!
dictionary GetTextClusterOptions {could we merge those two as TextClusterOptions?
The align/baseline are the same (btw, Are the "?=null" necessary?)
It would be reasonable to either ignore the x,y or add some meaning to it on the getTextCluster)
If it's okay to just ignore the `x` and `y` params then yes, at least implementation-wise.
In that case, should they still be separate dictionaries in the IDL for the explainer?
Related details
- Fernando Serboncini
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
TextCluster::TextCluster(const TextCluster& other)Andres Ricardo Perezcan't you just use the default copy constructor here?
TextCluster::TextCluster(const TextCluster&) = default;
The compiler doesn't allow it as `TextCluster` inherits from `ScriptWrappable`, which deletes the default copy constructor here: https://crsrc.org/c/third_party/blink/renderer/platform/bindings/script_wrappable.h;l=102
Do you know of a workaround or should I leave the constructor as is?
I'm not sure, but it sounds to me that there must be a reason for ScriptWrappable to be non-copyable. Is it safe to make child classes copyable?
From the [script_wrappable.h](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_wrappable.h;l=48;drc=7c38333ec090336a9a3a8be8deeb23ae76030942):
"ScriptWrappable provides a way to map from/to C++ DOM implementation to/from JavaScript object (platform object)."
What happens if you make a copy of the C++ object without also copying the corresponding JavaScript object? Is it legal to have a ScriptWrappable object without a corresponding JavaScript object?
Instead of copying the `TextCluster`, could you not update `DrawTextInternal` to accept the params to use, either the ones passed by via `TextClusterOptions`, the ones already in the `TextCluster`, or the parameters in the context state?
Related details
- Andres Ricardo Perez
- Fernando Serboncini
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez added 1 comment![]( "Open in Gerrit")
TextCluster::TextCluster(const TextCluster& other)Andres Ricardo Perezcan't you just use the default copy constructor here?
TextCluster::TextCluster(const TextCluster&) = default;
Jean-Philippe GravelThe compiler doesn't allow it as `TextCluster` inherits from `ScriptWrappable`, which deletes the default copy constructor here: https://crsrc.org/c/third_party/blink/renderer/platform/bindings/script_wrappable.h;l=102
Do you know of a workaround or should I leave the constructor as is?
I'm not sure, but it sounds to me that there must be a reason for ScriptWrappable to be non-copyable. Is it safe to make child classes copyable?
From the [script_wrappable.h](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_wrappable.h;l=48;drc=7c38333ec090336a9a3a8be8deeb23ae76030942):
"ScriptWrappable provides a way to map from/to C++ DOM implementation to/from JavaScript object (platform object)."What happens if you make a copy of the C++ object without also copying the corresponding JavaScript object? Is it legal to have a ScriptWrappable object without a corresponding JavaScript object?
Instead of copying the `TextCluster`, could you not update `DrawTextInternal` to accept the params to use, either the ones passed by via `TextClusterOptions`, the ones already in the `TextCluster`, or the parameters in the context state?
My reasoning for passing the whole `TextCluster` was to avoid adding to many optional parameters to `DrawTextInternal()`, but I do feel this will make the logic for which value to use.
I added the modifications to `DrawTextInternal()` in a previous CL in the stack. Let me know what you think!
Related details
- Fernando Serboncini
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Serboncini (Gerrit)
Fernando Serboncini added 2 comments![]( "Open in Gerrit")
dictionary GetTextClusterOptions {Andres Ricardo Perezcould we merge those two as TextClusterOptions?
The align/baseline are the same (btw, Are the "?=null" necessary?)
It would be reasonable to either ignore the x,y or add some meaning to it on the getTextCluster)
If it's okay to just ignore the `x` and `y` params then yes, at least implementation-wise.
In that case, should they still be separate dictionaries in the IDL for the explainer?
I think no, just make them the same?
if (ParseTextAlign(cluster_options->align(), options_align)) {can't we just "ParseTextAlign(cluster_options->align(), align); and below?
Related details
- Andres Ricardo Perez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez added 2 comments![]( "Open in Gerrit")
dictionary GetTextClusterOptions {Andres Ricardo Perezcould we merge those two as TextClusterOptions?
The align/baseline are the same (btw, Are the "?=null" necessary?)
It would be reasonable to either ignore the x,y or add some meaning to it on the getTextCluster)
Fernando SerbonciniIf it's okay to just ignore the `x` and `y` params then yes, at least implementation-wise.
In that case, should they still be separate dictionaries in the IDL for the explainer?
I think no, just make them the same?
Okay, I will use the same options dictionary for both in the explainer too. Thanks!
if (ParseTextAlign(cluster_options->align(), options_align)) {can't we just "ParseTextAlign(cluster_options->align(), align); and below?
- Fernando Serboncini
- Jean-Philippe Gravel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted and added 5 comments![]( "Open in Gerrit")
Votes added by Jean-Philippe Gravel
| Code-Review | +1 |
5 comments
Looks good, appart from a few nits and suggestions.
#include "third_party/blink/renderer/platform/graphics/graphics_types.h"This is still needed, isn't it? `TextAlign` and others are defined in this header.
fillTextCluster(text_cluster, x, y, nullptr);Please name this parameter with a comment.
TextAlign align = text_cluster->GetTextAlign();
TextBaseline baseline = text_cluster->GetTextBaseline();
double cluster_x = text_cluster->x();
double cluster_y = text_cluster->y();You are assigning all of these defaults even if they end up being never used if you have a `text_cluster`. Plus, the non-cluster-options API now needs to run though this more complex function. Why not just move each case of the `if` to the callers and have both `fillTextCluster` overloads call `DrawTextInternal` directly?
let tm = ctx.measureText(text);These two variables are never reassigned. Make them `const`, for consistency with the `text` variable?
Related details
- Andres Ricardo Perez
- Fernando Serboncini
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez added 4 comments![]( "Open in Gerrit")
#include "third_party/blink/renderer/platform/graphics/graphics_types.h"This is still needed, isn't it? `TextAlign` and others are defined in this header.
Done
fillTextCluster(text_cluster, x, y, nullptr);Please name this parameter with a comment.
Done
TextAlign align = text_cluster->GetTextAlign();
TextBaseline baseline = text_cluster->GetTextBaseline();
double cluster_x = text_cluster->x();
double cluster_y = text_cluster->y();You are assigning all of these defaults even if they end up being never used if you have a `text_cluster`. Plus, the non-cluster-options API now needs to run though this more complex function. Why not just move each case of the `if` to the callers and have both `fillTextCluster` overloads call `DrawTextInternal` directly?
@fserb suggested passing the non-options version of `fillTextCluster()` through here [in a previous comment](https://crrev.com/c/6006496/4..11/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc#b3422), although at that point we were copying the `TextCluster` object instead of passing the parameters directly.
I'm also working on a CL that migrates the align and baselines enums to IDL enums, which would come pre-parsed and would allow much cleaner ternary operators. Following our in-person discussion, I will leave this CL as is and do that in a follow-up CL to the IDL migration one.
These two variables are never reassigned. Make them `const`, for consistency with the `text` variable?
- Fernando Serboncini
- Jean-Philippe Gravel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Serboncini (Gerrit)
Fernando Serboncini added 2 comments![]( "Open in Gerrit")
it's looking much much better. :P
small nit.
TextAlign align = text_cluster->GetTextAlign();DCHECK(text_cluster) ?
Related details
- Andres Ricardo Perez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |