[iOS] Compile page tools non eligibility reasons [chromium/src : main]
Francis Beauchamp (Gerrit)
Francis Beauchamp added 2 comments![]( "Open in Gerrit")
- (BOOL)isLensAvailableForTraitCollection:(UITraitCollection*)traitCollection {Note 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.
// Handles toggle switch changes for permission-based features.Note to the reviewer: I have moved these functions to regroup all of the handlers together.
Related details
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 1 comment![]( "Open in Gerrit")
Sending this your way so you can have a look.
Related details
- 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 1 comment![]( "Open in Gerrit")
Updated with an action identifier instead of an url to follow what was done in the gemini consent screen.
Related details
- 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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 12 comments![]( "Open in Gerrit")
thanks Francis! see a few comments
This change introduces the various reasons for features being made non
available within the page tools menu. The UI that supports the feedbackcan you elaborate a little more on the changes in the commit message please?
return _geminiTabHelper->IsGeminiAvailableForWebState() &&is it intended that we no longer check the WebState eligibility?
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]];
}instead, can we add a TODO? I prefer to have landed code in a state that is not broken on its own (no string). thanks!
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:@""]];
}same TODO comment for these
// Disabled without disclaimernit: period at the end of comment.
- (instancetype)initWithEnabled:(BOOL)enabled;here and for above interface: set as designated initializer and `NS_UNAVAILABLE` the normal `init` (assuming we only want the user to use these)
@property(nonatomic, copy) NSString* text;
@property(nonatomic, strong) UIImage* icon;
@property(nonatomic, copy) NSString* actionIdentifier;here and below, will you need them to be public? if not, they can be ivars instead
also, comments for these please
@interface ContentEntryPointUnavailabilityItem : NSObjectnit: for this and interface below, comments
- (void)pageLoadStatusChanged {in the current state of the patch, this no longer checks webstate and only profile level checks, so does this need to stay?
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 4 comments![]( "Open in Gerrit")
Answering a few comments before I leave for the weekend. I will be addressing most of them on Monday. Let me know what you prefer for the string thing; I've proposed a way forward that mitigate your concerns, although I thing it won't make an impact in the end.
return _geminiTabHelper->IsGeminiAvailableForWebState() &&is it intended that we no longer check the WebState eligibility?
I need to reassess; I've made this CL a while ago and this is possible that it was an oversight on my end.
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]];
}instead, can we add a TODO? I prefer to have landed code in a state that is not broken on its own (no string). thanks!
I 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.
@class CrURL;still needed?
Should go in the garbage, I moved from a URL to a simple string identifier since the URLs opened link to internal pages.
Related details
- Ibrahim Kanouche
- Nicolas MacBeth
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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 4 comments![]( "Open in Gerrit")
ack! thanks
return _geminiTabHelper->IsGeminiAvailableForWebState() &&Francis Beauchampis it intended that we no longer check the WebState eligibility?
I need to reassess; I've made this CL a while ago and this is possible that it was an oversight on my end.
Ack
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!
I 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.
Acknowledged, ok either way then for me
@class CrURL;Francis Beauchampstill needed?
Should go in the garbage, I moved from a URL to a simple string identifier since the URLs opened link to internal pages.
- 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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 5 comments![]( "Open in Gerrit")
IneligibilityReasons ineligibilityReasons_;nit: `ineligibility_reasons_`
const IneligibilityReasons& ineligibilityReasons() const {nit: ineligibility_reasons()
bool isEligible() const { return is_eligible_; }if you do keep this class, `IsEligibile()`
instead 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
_actionIdentifier = actionIdentifier;`copy` as well?
Francis Beauchamp (Gerrit)
Francis Beauchamp added 4 comments![]( "Open in Gerrit")
bool isEligible() const { return is_eligible_; }if you do keep this class, `IsEligibile()`
I 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.
instead 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
So 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.
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.
Acknowledged, ok either way then for me
I'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.
@interface ContentEntryPointUnavailabilityItem : NSObjectFrancis Beauchampnit: for this and interface below, comments
Kind 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.
Related details
- Ibrahim Kanouche
- Nicolas MacBeth
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. |
Ibrahim Kanouche (Gerrit)
Ibrahim Kanouche added 5 comments![]( "Open in Gerrit")
// need to know the reason for non eligibility of the profile.Nit: "non eligibility" -> "ineligibility".
if (!_geminiService) {This method is missing the check for `_geminiTabHelper`. The previous implementation `isGeminiAvailable` checked for both `_geminiService` and `_geminiTabHelper`. Is this intentional?
if (result.isEligible()) {This 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.
- (instancetype)initWithText:(NSString*)textThe `and` prefix for subsequent parameters is usually omitted: `initWithText:(NSString*)text icon:(UIImage*)icon actionIdentifier:(NSString*)actionIdentifier;`
self = [self initWithEnabled:enabled footerItem:nil];This can be simplified to:
`return [self initWithEnabled:enabled footerItem:nil];`
Related details
- Francis Beauchamp
- Nicolas MacBeth
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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 2 comments![]( "Open in Gerrit")
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
So 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.
std::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!
@interface ContentEntryPointUnavailabilityItem : NSObjectFrancis Beauchampnit: for this and interface below, comments
Kind 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.
some people use it, but you'll see it quite rarely indeed!
Related details
- Francis Beauchamp
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. |