Remove kDafsaNotFound in favor of using std::optional [chromium/src : main]
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Auto-Submit | +1 |
1 comment
+mmenke for //net/;
+cthomp for //components/url_formatter/;
+theestig for //chrome/browser/media/.
Related details
- Chris Thompson
- Lei Zhang
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted and added 7 comments![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
7 comments
Just my take, but I defer to //net owners.
std::optional<int> result = net::LookupStringInFixedSet(dafsa_, input);Can this be `return net::LookupStringInFixedSet(dafsa_, input).value_or(DafsaResult::kNotFound);`?
// value is std::nullopt, kDafsaFound, or a bitmap consisting of one or moreWould you mind wrapping these in backticks while this is changing anyway?
enum {Can the enum have a name, or LookupStringInFixedSet() can return that? I don't know the historical context, so if it is this way for a reason, then it's fine to leave it be.
} else {No need for `else` after the return now.
CHECK(type.has_value());Not sure if this makes sense, as line 227 already called value(). If that didn't crash, then this always evaluates to true.
Related details
- Chris Fredrickson
- Chris Thompson
- mmenke
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. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 6 comments![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Auto-Submit | +1 |
6 comments
std::optional<int> result = net::LookupStringInFixedSet(dafsa_, input);Can this be `return net::LookupStringInFixedSet(dafsa_, input).value_or(DafsaResult::kNotFound);`?
Ah nice, I forgot about `value_or`. Done.
// value is std::nullopt, kDafsaFound, or a bitmap consisting of one or moreWould you mind wrapping these in backticks while this is changing anyway?
Done
Can the enum have a name, or LookupStringInFixedSet() can return that? I don't know the historical context, so if it is this way for a reason, then it's fine to leave it be.
I plan to give it a name (and convert to `enum class`) and make LookupStringInFixedSet return a `base::EnumSet<...>`, yeah. But that will have some downstream logic changes (e.g. that will expose a bug in chrome/browser/media/media_engagement_preloaded_list.cc, where that code doesn't handle the bitset properly and currently assumes it is just a single tag value), so I'm going to make that change in a followup CL.
No need for `else` after the return now.
Done
if (value) {Chris FredricksonConsistently use has_value().
Done
Not sure if this makes sense, as line 227 already called value(). If that didn't crash, then this always evaluates to true.
- Chris Thompson
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 4 comments![]( "Open in Gerrit")
MediaEngagementPreloadedList::CheckStringIsPresent(BUG: DafsaResult::kNotFound is now wrong.
// bitmask of `kDafsaExceptionRule`, `kDafsaWildcardRule`, and
// `kDafsaPrivateRule`.Again, this documentation seems confusing to me.
// byte array which should be supplied via the |graph| parameter. The return
// value is `std::nullopt`, `kDafsaFound`, or a bitmap consisting of one or more
// of `kDafsaExceptionRule`, `kDafsaWildcardRule` and `kDafsaPrivateRule` ORed
// together.This is confusing, since the comment above implies those values are only for use by GetDomainAndRegistry(), but this is specced to return them. Also, MediaEngagementPreloadedList::CheckStringIsPresent() is clearly not using these to get bitmaps of the listed values.
I think we should clean up all the documentation here, removing mention of these values, when we also move out that enum above.
// The following return values are used by the implementation of
// GetDomainAndRegistry() and are probably not generally useful.Can we move this into a GetDomainAndRegistry()-specific file? Fine to do in a followup. Even kDafsaFound seems not generally useful (And it's only used in a single test, with one use of it looking scary to me - https://source.chromium.org/chromium/chromium/src/+/main:net/tools/tld_cleanup/tld_cleanup_util_unittest.cc;l=184?q=kDafsaFound%20&ss=chromium%2Fchromium%2Fsrc)
Related details
- Chris Fredrickson
- Chris Thompson
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. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 4 comments![]( "Open in Gerrit")
MediaEngagementPreloadedList::CheckStringIsPresent(BUG: DafsaResult::kNotFound is now wrong.
Can you elaborate? IIUC `DafsaResult::kNotFound` is still correct (it's only used when `LookupStringInFixedSet` returns `std::nullopt`), but the other two values are definitely wrong. However, that is an existing issue for the past 8 years or so, and I'll try to address it in a followup CL.
// bitmask of `kDafsaExceptionRule`, `kDafsaWildcardRule`, and
// `kDafsaPrivateRule`.Again, this documentation seems confusing to me.
Acknowledged
// byte array which should be supplied via the |graph| parameter. The return
// value is `std::nullopt`, `kDafsaFound`, or a bitmap consisting of one or more
// of `kDafsaExceptionRule`, `kDafsaWildcardRule` and `kDafsaPrivateRule` ORed
// together.This is confusing, since the comment above implies those values are only for use by GetDomainAndRegistry(), but this is specced to return them. Also, MediaEngagementPreloadedList::CheckStringIsPresent() is clearly not using these to get bitmaps of the listed values.
I think we should clean up all the documentation here, removing mention of these values, when we also move out that enum above.
Agreed, I'll try to do that in followups, but I'd like to keep this CL focused on just one step, i.e. removing `kDafsaNotFound`.
// The following return values are used by the implementation of
// GetDomainAndRegistry() and are probably not generally useful.Can we move this into a GetDomainAndRegistry()-specific file? Fine to do in a followup. Even kDafsaFound seems not generally useful (And it's only used in a single test, with one use of it looking scary to me - https://source.chromium.org/chromium/chromium/src/+/main:net/tools/tld_cleanup/tld_cleanup_util_unittest.cc;l=184?q=kDafsaFound%20&ss=chromium%2Fchromium%2Fsrc)
I actually think that the comment is wrong - these values are useful outside of GetDomainAndRegistry (e.g. media_engagement_preloaded_list.cc relies on them), but I will move them to a separate file in a followup. I also agree that `kDafsaFound` isn't very useful; I'm planning on removing that in a followup as well in favor of using a `base::EnumSet<...>`, but that will force me to figure out how to fix media_engagement_preloaded_list.cc so I wanted to keep that part separate for ease of review. (Also agree about that test usage - I wrote that but now it looks wrong to me.)
All in all I agree with you, but I'd like to handle these issues in separate followups for the sake of keeping CLs small and easy to review.
Related details
- Chris Thompson
- mmenke
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. |
Chris Thompson (Gerrit)
Chris Thompson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Thompson
| Code-Review | +1 |
1 comment
components/url_formatter/ change LGTM
Related details
- Chris Fredrickson
- mmenke
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. |
mmenke (Gerrit)
mmenke added 2 comments![]( "Open in Gerrit")
MediaEngagementPreloadedList::CheckStringIsPresent(Chris FredricksonBUG: DafsaResult::kNotFound is now wrong.
Can you elaborate? IIUC `DafsaResult::kNotFound` is still correct (it's only used when `LookupStringInFixedSet` returns `std::nullopt`), but the other two values are definitely wrong. However, that is an existing issue for the past 8 years or so, and I'll try to address it in a followup CL.
Sorry, I missed that this didn't return an optional for anoptional.
// The following return values are used by the implementation of
// GetDomainAndRegistry() and are probably not generally useful.Chris FredricksonCan we move this into a GetDomainAndRegistry()-specific file? Fine to do in a followup. Even kDafsaFound seems not generally useful (And it's only used in a single test, with one use of it looking scary to me - https://source.chromium.org/chromium/chromium/src/+/main:net/tools/tld_cleanup/tld_cleanup_util_unittest.cc;l=184?q=kDafsaFound%20&ss=chromium%2Fchromium%2Fsrc)
I actually think that the comment is wrong - these values are useful outside of GetDomainAndRegistry (e.g. media_engagement_preloaded_list.cc relies on them), but I will move them to a separate file in a followup. I also agree that `kDafsaFound` isn't very useful; I'm planning on removing that in a followup as well in favor of using a `base::EnumSet<...>`, but that will force me to figure out how to fix media_engagement_preloaded_list.cc so I wanted to keep that part separate for ease of review. (Also agree about that test usage - I wrote that but now it looks wrong to me.)
All in all I agree with you, but I'd like to handle these issues in separate followups for the sake of keeping CLs small and easy to review.
media_engagement_preloaded_list.cc doesn't seem to rely on them at all? It relies on getting a 0 or 1 out of this API, but it doesn't use this enum at all, and uses entirely different meanings for 0 and 1. 0 means "kFoundHttpsOnly" and 1 means "kFoundHttpOrHttps", which in no way map to "kDafsaFound" and "kDafsaExceptionRule".
The code to use them certainly seems to think they mean other things: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/media/media_engagement_preloaded_list.cc;l=99;drc=d209eefee6037bd0905e43f88570fda8edab89a1?q=kFoundHttpOrHttps&ss=chromium%2Fchromium%2Fsrc
They seem to be using this as a way to store a bit (+not found) per match, rather than having a notion of "exception rules" that matches whatever these are intended for.
Related details
- Chris Fredrickson
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. |
mmenke (Gerrit)
mmenke voted and added 1 comment![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
1 comment
Oh, and this LGTM
Related details
- Chris Fredrickson
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. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Commit-Queue | +2 |
1 comment
// The following return values are used by the implementation of
// GetDomainAndRegistry() and are probably not generally useful.Chris FredricksonCan we move this into a GetDomainAndRegistry()-specific file? Fine to do in a followup. Even kDafsaFound seems not generally useful (And it's only used in a single test, with one use of it looking scary to me - https://source.chromium.org/chromium/chromium/src/+/main:net/tools/tld_cleanup/tld_cleanup_util_unittest.cc;l=184?q=kDafsaFound%20&ss=chromium%2Fchromium%2Fsrc)
mmenkeI actually think that the comment is wrong - these values are useful outside of GetDomainAndRegistry (e.g. media_engagement_preloaded_list.cc relies on them), but I will move them to a separate file in a followup. I also agree that `kDafsaFound` isn't very useful; I'm planning on removing that in a followup as well in favor of using a `base::EnumSet<...>`, but that will force me to figure out how to fix media_engagement_preloaded_list.cc so I wanted to keep that part separate for ease of review. (Also agree about that test usage - I wrote that but now it looks wrong to me.)
All in all I agree with you, but I'd like to handle these issues in separate followups for the sake of keeping CLs small and easy to review.
media_engagement_preloaded_list.cc doesn't seem to rely on them at all? It relies on getting a 0 or 1 out of this API, but it doesn't use this enum at all, and uses entirely different meanings for 0 and 1. 0 means "kFoundHttpsOnly" and 1 means "kFoundHttpOrHttps", which in no way map to "kDafsaFound" and "kDafsaExceptionRule".
The code to use them certainly seems to think they mean other things: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/media/media_engagement_preloaded_list.cc;l=99;drc=d209eefee6037bd0905e43f88570fda8edab89a1?q=kFoundHttpOrHttps&ss=chromium%2Fchromium%2Fsrc
They seem to be using this as a way to store a bit (+not found) per match, rather than having a notion of "exception rules" that matches whatever these are intended for.
Ack - right - what I meant by "relies on" is that that code makes assumptions about the values that come out of `LookupStringInFixedSet`, i.e. that it will produce values that map cleanly to `DafsaResult` values (and won't produce any _other_ values, either).
But I see your point that that code is ascribing entirely different semantics to these values, so it doesn't really rely on the GetDomainAndRegistry-related semantics. Maybe a future followup can templatize the `LookupStringInFixedSet` machinery so that different clients can supply the sets of valid tags that they want to use.
Related details
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![]( "Open in Gerrit")
Change information
Remove kDafsaNotFound in favor of using std::optional
This gives us a more modern and less error-prone API, and lets us remove
an unnecessary constant.
- M chrome/browser/media/media_engagement_preloaded_list.cc
- M components/url_formatter/spoof_checks/common_words/common_words_util.cc
- M net/base/lookup_string_in_fixed_set.cc
- M net/base/lookup_string_in_fixed_set.h
- M net/base/lookup_string_in_fixed_set_unittest.cc
- M net/base/registry_controlled_domains/registry_controlled_domain.cc
Code-Review: +1 by mmenke, +1 by Lei Zhang, +1 by Chris Thompson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |