[iOS] Compile page tools feature ineligibility reasons [chromium/src : main]
Francis Beauchamp (Gerrit)
Francis Beauchamp added 26 comments![]( "Open in Gerrit")
Updated according to your comments. I'll double check the whole thing to make sure everything was addressed properly
This change introduces the various reasons for features being made non
available within the page tools menu. The UI that supports the feedbackFrancis Beauchampcan you elaborate a little more on the changes in the commit message please?
Done
// need to know the reason for non eligibility of the profile.Francis BeauchampNit: "non eligibility" -> "ineligibility".
Done
IneligibilityReasons ineligibilityReasons_;Francis Beauchampnit: `ineligibility_reasons_`
Done
const IneligibilityReasons& ineligibilityReasons() const {Francis Beauchampnit: ineligibility_reasons()
Probably a nitwish, but it would be nice if `git cl format` would allow formatting names so they align with language specific conventions. I find it to be quite distracting to remember if this is objc or c++ and should use snake, camel case or whatnot. Are we allowed to make changes to those scripts? I'd be willing to investigate the possibility of making that change.
bool isEligible() const { return is_eligible_; }Francis Beauchampif you do keep this class, `IsEligibile()`
Francis BeauchampI think I want all of this object to be immutable. I'll probably make the properties constants. The idea is that the static "builder" functions should be the only way to create this.
Done
Francis Beauchampinstead of this whole file/class, what about wrapping ineligibility reasons in a `std::optional`? would be quite idiomatic and also no need for this whole and its maintenance. no a strong opinion, food for though
Nicolas MacBethSo my naive long term goal would be to delegate more of the computation of eligibility in here if possible since it has grown a lot over the last few weeks (i.e. remove the cognitive burden of needing ~8 booleans to evaluate eligibility in the service). I'm not certain an `std::optional` gracefully allows for that, but maybe I'me foreseeing something for which we have no need? Would be curious to know your thoughts on this.
Francis Beauchampstd::optional could wrap the presence or not of ineligibility reasons, instead of the class in its current state, but if you prefer to move the actual computation of it down to this class, then that makes more sense to me!
I'll try your approach but for more insight, what I had in mind somewhat aligns with the [expected](https://en.cppreference.com/w/cpp/utility/expected.html) type.
- (BOOL)isLensAvailableForTraitCollection:(UITraitCollection*)traitCollection {Francis BeauchampNote to the reviewer: these functions have been replaced with their "entry point" counterparts, which adds a new abstraction layer for supporting the footer items linked to ineligibility reasons.
Done
if (!_geminiService) {Francis BeauchampThis method is missing the check for `_geminiTabHelper`. The previous implementation `isGeminiAvailable` checked for both `_geminiService` and `_geminiTabHelper`. Is this intentional?
Not intentional; it seems like a regression as Nic suggested in the comment above. The code should now align with the desired behaviour.
return _geminiTabHelper->IsGeminiAvailableForWebState() &&Francis Beauchampis it intended that we no longer check the WebState eligibility?
Nicolas MacBethI need to reassess; I've made this CL a while ago and this is possible that it was an oversight on my end.
Francis BeauchampAck
I've updated the code as you ad Ibrahim suggested; this should no longer be an issue but feel free to double check again.
if (result.isEligible()) {Francis BeauchampThis no longer checks `_geminiTabHelper->IsGeminiAvailableForWebState()`. This means Gemini could be enabled on pages where it is meant to be disabled (like internal chrome:// pages).
You should probably return `initWithEnabled:YES` only if `result.isEligible()` AND `_geminiTabHelper->IsGeminiAvailableForWebState()` are both true.
I've updated the checks here if you want to double check.
If unwrapping the optional yields a value, the profile is ineligible. We look at the ineligibility reasons that support footer items to populate the "view model".
Otherwise, the profile is eligible and we simply check if Gemini is eligible for the webState in order to set the entry point availability.
if (result.ineligibilityReasons().chrome_enterprise) {
// TODO(crbug.com/485297147): Add string when the footer UI gets
// implemented.
return [[PageActionMenuContentEntryPoint alloc]
initWithEnabled:NO
footerItem:
[[ContentEntryPointUnavailabilityItem alloc]
initWithText:@""
andIcon:
[CustomSymbolWithPointSize(
kEnterpriseSymbol, kFeatureRowIconSize)
imageWithRenderingMode:
UIImageRenderingModeAlwaysTemplate]
andActionIdentifier:nil]];
}Francis Beauchampinstead, can we add a TODO? I prefer to have landed code in a state that is not broken on its own (no string). thanks!
Nicolas MacBethI would be tempted to agree in spirit, but the UI doesn't exist yet here; none of the parameters in the `footerIem` are going to be reflected anywhere in the page tools. The next cl in the chain will use the footer item to showcase the unavailability, while this CL only relies on the `enabled`. A possible solution to this would be to temporarily make the `footerItem` parameter `nil`, although the end result is exactly the same for all code paths and behaviour.
Francis BeauchampAcknowledged, ok either way then for me
Francis BeauchampI'll do the change, I'll just need to rebase a few CLs down the line and I'm more than happy if it mitigates your concerns.
Done
if (!featureAvailable) {
// TODO(crbug.com/485297147): Add string when the footer UI gets
// implemented.
return [[PageActionMenuContentEntryPoint alloc]
initWithEnabled:NO
footerItem:
[[ContentEntryPointUnavailabilityItem alloc]
initWithText:@""
andIcon:
[CustomSymbolWithPointSize(
kEnterpriseSymbol, kFeatureRowIconSize)
imageWithRenderingMode:
UIImageRenderingModeAlwaysTemplate]
andActionIdentifier:nil]];
}
if (!hasDefaultSearchEngine) {
// TODO(crbug.com/485297147): Add string when the footer UI gets
// implemented.
return [[PageActionMenuContentEntryPoint alloc]
initWithEnabled:NO
footerItem:[[ContentEntryPointUnavailabilityItem alloc]
initWithText:@""]];
}Francis Beauchampsame TODO comment for these
Done
// Disabled without disclaimerFrancis Beauchampnit: period at the end of comment.
Done
- (instancetype)initWithEnabled:(BOOL)enabled;Francis Beauchamphere and for above interface: set as designated initializer and `NS_UNAVAILABLE` the normal `init` (assuming we only want the user to use these)
Done
- (instancetype)initWithText:(NSString*)textFrancis BeauchampThe `and` prefix for subsequent parameters is usually omitted: `initWithText:(NSString*)text icon:(UIImage*)icon actionIdentifier:(NSString*)actionIdentifier;`
I'll update accordingly; wasn't aware we had that in our style guide.
- (instancetype)initWithText:(NSString*)textFrancis BeauchampThe `and` prefix for subsequent parameters is usually omitted: `initWithText:(NSString*)text icon:(UIImage*)icon actionIdentifier:(NSString*)actionIdentifier;`
Is that always the case? That goes against usual objc
@property(nonatomic, copy) NSString* text;
@property(nonatomic, strong) UIImage* icon;
@property(nonatomic, copy) NSString* actionIdentifier;Francis Beauchamphere and below, will you need them to be public? if not, they can be ivars instead
also, comments for these please
I think the confusion comes from the fact that these are used in the next CL by the view controller. They cannot be ivars since the UI will need to access these properties.
@interface ContentEntryPointUnavailabilityItem : NSObjectFrancis Beauchampnit: for this and interface below, comments
Nicolas MacBethKind of a parallel question; do we generate documentation with our comments? I don't see us using the `///` format which allows for that in iOS code. It comes with the additional drawback of `alt + click` never giving descriptions for any of the commented declarations.
Francis Beauchampsome people use it, but you'll see it quite rarely indeed!
Acknowledged
@class CrURL;Francis Beauchampstill needed?
Nicolas MacBethShould go in the garbage, I moved from a URL to a simple string identifier since the URLs opened link to internal pages.
Francis BeauchampAck
Done
@class CrURL;Francis Beauchampstill needed?
Nicolas MacBethShould go in the garbage, I moved from a URL to a simple string identifier since the URLs opened link to internal pages.
Francis BeauchampAck
Done
self = [self initWithEnabled:enabled footerItem:nil];Francis BeauchampThis can be simplified to:
`return [self initWithEnabled:enabled footerItem:nil];`
Done
_actionIdentifier = actionIdentifier;Francis Beauchamp`copy` as well?
Done
- (void)pageLoadStatusChanged {Francis Beauchampin the current state of the patch, this no longer checks webstate and only profile level checks, so does this need to stay?
Should be fixed by checking the web state now.
// Handles toggle switch changes for permission-based features.Francis BeauchampNote to the reviewer: I have moved these functions to regroup all of the handlers together.
Done
Related details
- Ibrahim Kanouche
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 6 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
6 comments
lgtm with a few nits, thanks Francis!
// are deemed eligible for Gemini will result in std::nulopt.nit: nullopt
return is_eligible ? std::optional<gemini::IneligibilityReasons>()`std::nullopt`?
return [[PageActionMenuContentEntryPoint alloc]
initWithEnabled:_geminiTabHelper->IsGeminiAvailableForWebState()];might be a larger UX question so could definitely be a follow-up CL, but this would mean that the user would see some eligibility issue string, fix it, and then can come back and still not be able to use Gemini due to the webstate not being eligibile. should we instead only surface the eligibility issue if the webstate is valid to use gemini on?
- (instancetype)initWithText:(NSString*)textnit: comment for this initializer too
@property(nonatomic, copy) NSString* actionIdentifier;not sure what this is for in upcoming CLs, but consider using `GURL`. OK to proceed without though for now
Related details
- Francis Beauchamp
- Ibrahim Kanouche
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. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 5 comments![]( "Open in Gerrit")
// are deemed eligible for Gemini will result in std::nulopt.Francis Beauchampnit: nullopt
Done
return is_eligible ? std::optional<gemini::IneligibilityReasons>()Francis Beauchamp`std::nullopt`?
Compiler was not happy with it because of the ternary, so I had to be extra specific about the associated type of that optional (using the default constructor). I tend to prefer single return points to having multiple return paths like this:
```
if (is_eligible) {
return std::nullopt;
}
return ineligibility_reasons;
```
return [[PageActionMenuContentEntryPoint alloc]
initWithEnabled:_geminiTabHelper->IsGeminiAvailableForWebState()];might be a larger UX question so could definitely be a follow-up CL, but this would mean that the user would see some eligibility issue string, fix it, and then can come back and still not be able to use Gemini due to the webstate not being eligibile. should we instead only surface the eligibility issue if the webstate is valid to use gemini on?
Like we discussed offline, I believe this might be something to be investigated at the UX level.
nit: comment for this initializer too
Done
@property(nonatomic, copy) NSString* actionIdentifier;not sure what this is for in upcoming CLs, but consider using `GURL`. OK to proceed without though for now
The way the link is handled here is following the pattern we have in the consent screen. We associate an identifier to the link, and as the link gets tapped in the text view, we do not open an URL. Instead, we simply `switch` on the string and trigger the specific logic associated with that action identifier.
See [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/bwg/ui/gemini_consent_view_controller.mm;l=533) for a similar pattern.
Related details
- Ibrahim Kanouche
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |