[Gap Decorations]: Add UseCounter for Disentanglement change [chromium/src : main]
Alison Maher (Gerrit)
Alison Maher added 12 comments![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));We may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
// "hidden". This is to assess web compatibility risks, as thewill be used
// is triggered when the computed value for a width property isThis could get confused with "width". I would specify it is related to border-width, outline-width, and column-rule-width
// Records use counters for width/style properties. The use counterI'd remove this sentence since it isn't a use counter for the property, but under specific circumstances, which is covered by the rest of the paragraph
// Records use counters for width/style properties. The use counterThere is more than one use counter, right?
const CSSValue* ComputedStylePropertyMap::GetProperty(Why do we only for this on a Get instead of if it is set at all? Is this because we could only cause web compat issues if the author is querying the value since the rendering would be equivalent either way?
if (property.PropertyID() == CSSPropertyID::kBorderWidth) {Likely worth adding a comment here, as well, for extra context at this callsite
return false;Will we ever hit this? If not, should it be a NOT_REACHED instead and just be a void method?
auto ShouldCount = [](int width_value, EBorderStyle style_value) {nit: ShouldRecordUseCounter
int width_value = 0;
EBorderStyle style_value = EBorderStyle::kNone;I'd move these earlier since they can be used here and below
// Record once for the border-width shorthandMissing punctuation. But it's also likely worth noting why we only record once in this case
friend class ComputedStylePropertyMap;Why was this needed?
Related details
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 13 comments![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));We may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
SO essentially having 7 different .html pages for each property? That seems a bit much no?
// "hidden". This is to assess web compatibility risks, as theSam Davis Omekarawill be used
Done
// is triggered when the computed value for a width property isThis could get confused with "width". I would specify it is related to border-width, outline-width, and column-rule-width
Done
// Records use counters for width/style properties. The use counterThere is more than one use counter, right?
Single one for each property so I used "property-specific". Let me know if you have better suggestions.
// Records use counters for width/style properties. The use counterI'd remove this sentence since it isn't a use counter for the property, but under specific circumstances, which is covered by the rest of the paragraph
Done
Why do we only for this on a Get instead of if it is set at all? Is this because we could only cause web compat issues if the author is querying the value since the rendering would be equivalent either way?
Yes, It'll have to be queried via `ComputedStylePropertyMap` for us to notice observable change in behavior.
if (property.PropertyID() == CSSPropertyID::kBorderWidth) {Likely worth adding a comment here, as well, for extra context at this callsite
Done
Will we ever hit this? If not, should it be a NOT_REACHED instead and just be a void method?
Done
auto ShouldCount = [](int width_value, EBorderStyle style_value) {Sam Davis Omekaranit: ShouldRecordUseCounter
Done
int width_value = 0;
EBorderStyle style_value = EBorderStyle::kNone;I'd move these earlier since they can be used here and below
Done
Missing punctuation. But it's also likely worth noting why we only record once in this case
I mean for all properties we record once, I took out "once" and added more context. The diff here is that because there multiple longhands associated, we could record 4 times instead of once.
friend class ComputedStylePropertyMap;Sam Davis OmekaraWhy was this needed?
Needed to access the *Internal methods. The comment above applies.
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 3 comments![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));Sam Davis OmekaraWe may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
SO essentially having 7 different .html pages for each property? That seems a bit much no?
I can't think of a better way to do it, but as it stands, we aren't really testing that these are firing in different scenarios
// Records use counters for width/style properties. The use counterSam Davis OmekaraThere is more than one use counter, right?
Single one for each property so I used "property-specific". Let me know if you have better suggestions.
Acknowledged
const CSSValue* ComputedStylePropertyMap::GetProperty(Sam Davis OmekaraWhy do we only for this on a Get instead of if it is set at all? Is this because we could only cause web compat issues if the author is querying the value since the rendering would be equivalent either way?
Yes, It'll have to be queried via `ComputedStylePropertyMap` for us to notice observable change in behavior.
Will this go down the same path if you do window.getComputedStyle(element).borderWidth? Or element.style.borderWidth?
Related details
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 2 comments![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));Sam Davis OmekaraWe may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
Alison MaherSO essentially having 7 different .html pages for each property? That seems a bit much no?
I can't think of a better way to do it, but as it stands, we aren't really testing that these are firing in different scenarios
Help me understand:
In the existing test, we set the seven different properties and query each of them and expect a hit count for the seven separate properties use counters.
So in my opinion, it's testing firing for different scenarios.
Is your worry that we might fire for border-left-width when we set and query border-width or vice versa?
@kbab...@microsoft.com any thoughts here?
const CSSValue* ComputedStylePropertyMap::GetProperty(Sam Davis OmekaraWhy do we only for this on a Get instead of if it is set at all? Is this because we could only cause web compat issues if the author is querying the value since the rendering would be equivalent either way?
Alison MaherYes, It'll have to be queried via `ComputedStylePropertyMap` for us to notice observable change in behavior.
Will this go down the same path if you do window.getComputedStyle(element).borderWidth? Or element.style.borderWidth?
No because that's querying the resolved value and not computed value.
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 2 comments![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));Sam Davis OmekaraWe may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
Alison MaherSO essentially having 7 different .html pages for each property? That seems a bit much no?
Sam Davis OmekaraI can't think of a better way to do it, but as it stands, we aren't really testing that these are firing in different scenarios
Help me understand:
In the existing test, we set the seven different properties and query each of them and expect a hit count for the seven separate properties use counters.
So in my opinion, it's testing firing for different scenarios.
Is your worry that we might fire for border-left-width when we set and query border-width or vice versa?
@kbab...@microsoft.com any thoughts here?
Yeah, essentially if your code fired all of them whenever one of these were queried, this test would still pass, which isn't what we want
const CSSValue* ComputedStylePropertyMap::GetProperty(Sam Davis OmekaraWhy do we only for this on a Get instead of if it is set at all? Is this because we could only cause web compat issues if the author is querying the value since the rendering would be equivalent either way?
Alison MaherYes, It'll have to be queried via `ComputedStylePropertyMap` for us to notice observable change in behavior.
Sam Davis OmekaraWill this go down the same path if you do window.getComputedStyle(element).borderWidth? Or element.style.borderWidth?
No because that's querying the resolved value and not computed value.
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt added 1 comment![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));Sam Davis OmekaraWe may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
Alison MaherSO essentially having 7 different .html pages for each property? That seems a bit much no?
Sam Davis OmekaraI can't think of a better way to do it, but as it stands, we aren't really testing that these are firing in different scenarios
Alison MaherHelp me understand:
In the existing test, we set the seven different properties and query each of them and expect a hit count for the seven separate properties use counters.
So in my opinion, it's testing firing for different scenarios.
Is your worry that we might fire for border-left-width when we set and query border-width or vice versa?
@kbab...@microsoft.com any thoughts here?
Yeah, essentially if your code fired all of them whenever one of these were queried, this test would still pass, which isn't what we want
I agree with Alison that it would be good to exercise each property in its own test. I'm unclear on why that would require 7 different html pages. For example, would it be possible for each test to inject a bit of JavaScript to exercise a specific property? There's examples of this elsewhere in the file; search for `EvalJs`.
I'm also wondering if it would be worth parameterizing these tests to reduce repetition among them.
Related details
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));Sam Davis OmekaraWe may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
Alison MaherSO essentially having 7 different .html pages for each property? That seems a bit much no?
Sam Davis OmekaraI can't think of a better way to do it, but as it stands, we aren't really testing that these are firing in different scenarios
Alison MaherHelp me understand:
In the existing test, we set the seven different properties and query each of them and expect a hit count for the seven separate properties use counters.
So in my opinion, it's testing firing for different scenarios.
Is your worry that we might fire for border-left-width when we set and query border-width or vice versa?
@kbab...@microsoft.com any thoughts here?
Kevin BabbittYeah, essentially if your code fired all of them whenever one of these were queried, this test would still pass, which isn't what we want
I agree with Alison that it would be good to exercise each property in its own test. I'm unclear on why that would require 7 different html pages. For example, would it be possible for each test to inject a bit of JavaScript to exercise a specific property? There's examples of this elsewhere in the file; search for `EvalJs`.
I'm also wondering if it would be worth parameterizing these tests to reduce repetition among them.
Just did something like this. Let me know if this is sufficient.
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
1 comment
R"JS(
document.getElementById('width-no-style')
.computedStyleMap()
.get('border-top-width');
)JS",Can we reduce the repetition by having just the property name be in the test parameters, and having the test function splice that into the remainder of the script?
Related details
- Alison Maher
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
2 comments
LGTM % Kevin's feedback and merge conflict
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddPageExpectation(TimingField::kLoadEvent);
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/use_counter_features.html")));Sam Davis OmekaraWe may want to test this in a way that loads one at a time to ensure that only the relevant use counter is fired when appropriate
Alison MaherSO essentially having 7 different .html pages for each property? That seems a bit much no?
Sam Davis OmekaraI can't think of a better way to do it, but as it stands, we aren't really testing that these are firing in different scenarios
Alison MaherHelp me understand:
In the existing test, we set the seven different properties and query each of them and expect a hit count for the seven separate properties use counters.
So in my opinion, it's testing firing for different scenarios.
Is your worry that we might fire for border-left-width when we set and query border-width or vice versa?
@kbab...@microsoft.com any thoughts here?
Kevin BabbittYeah, essentially if your code fired all of them whenever one of these were queried, this test would still pass, which isn't what we want
Sam Davis OmekaraI agree with Alison that it would be good to exercise each property in its own test. I'm unclear on why that would require 7 different html pages. For example, would it be possible for each test to inject a bit of JavaScript to exercise a specific property? There's examples of this elsewhere in the file; search for `EvalJs`.
I'm also wondering if it would be worth parameterizing these tests to reduce repetition among them.
Just did something like this. Let me know if this is sufficient.
- Sam Davis Omekara
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 2 comments![]( "Open in Gerrit")
@sisid...@chromium.org: for c/b/p/page_load_metrics_browsertests.cc
R"JS(
document.getElementById('width-no-style')
.computedStyleMap()
.get('border-top-width');
)JS",Can we reduce the repetition by having just the property name be in the test parameters, and having the test function splice that into the remainder of the script?
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
2 comments
page_load_metrics LGTM.
namespace {
struct DecoupleComputedWidthCase {
const char* property_name;
WebFeature feature;
};
static const DecoupleComputedWidthCase kDecoupleComputedWidthCases[] = {
{
"border-top-width",
WebFeature::kComputedBorderTopWidthWithNoneOrHiddenStyle,
},
{
"border-right-width",
WebFeature::kComputedBorderRightWidthWithNoneOrHiddenStyle,
},
{
"border-bottom-width",
WebFeature::kComputedBorderBottomWidthWithNoneOrHiddenStyle,
},
{
"border-left-width",
WebFeature::kComputedBorderLeftWidthWithNoneOrHiddenStyle,
},
{
"border-width",
WebFeature::kComputedBorderWidthWithNoneOrHiddenStyle,
},
{
"column-rule-width",
WebFeature::kComputedColumnRuleWidthWithNoneOrHiddenStyle,
},
{
"outline-width",
WebFeature::kComputedOutlineWidthWithNoneOrHiddenStyle,
},
};
} // namespaceCould you avoid having multiple anonymous namespaces? It would be better to move this to the existing namespace started from L133.
Related details
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
namespace {Could you avoid having multiple anonymous namespaces? It would be better to move this to the existing namespace started from L133.
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| 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
12 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/public/mojom/use_counter/metrics/web_feature.mojom
Insertions: 8, Deletions: 7.
The diff is too large to show. Please review the diff.
```
```
The name of the file: tools/metrics/histograms/metadata/blink/enums.xml
Insertions: 8, Deletions: 7.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
Insertions: 36, Deletions: 39.
The diff is too large to show. Please review the diff.
```
Change information
[Gap Decorations]: Add UseCounter for Disentanglement change
This CL introduces granular use counters for the *-width CSS properties
affected by the disentanglement work in crrev.com/c/6489213. These
counters are property-specific to accurately measure usage and assess
potential web compatibility risks resulting from the CSS issue
resolution [1], which led to decoupling the *-width computed values from
their corresponding style values.
[1]:
https://github.com/w3c/csswg-drafts/issues/11494#issuecomment-2675800489
- M chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
- M chrome/test/data/page_load_metrics/use_counter_features.html
- M third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
- M third_party/blink/renderer/core/css/cssom/computed_style_property_map.cc
- M third_party/blink/renderer/core/css/cssom/computed_style_property_map.h
- M third_party/blink/renderer/core/style/computed_style.h
- M tools/metrics/histograms/metadata/blink/enums.xml
Code-Review: +1 by Kevin Babbitt, +1 by Alison Maher, +1 by Shunya Shishido
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |