[iOS][AppBar] Introduce Menu Factory for Toolbar Buttons [chromium/src : main]
Eric Ekey (Gerrit)
Eric Ekey voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eric Ekey
| Commit-Queue | +1 |
Related details
- Scott Yoder
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. |
Scott Yoder (Gerrit)
Scott Yoder added 7 comments![]( "Open in Gerrit")
_buttonMenuFactory.currentWebStateList = _currentWebStateList;This wont be set at this point, so you could probably skip setting it here.
- (void)setRegularActionFactory:(BrowserActionFactory*)regularActionFactory {
if (regularActionFactory == _regularActionFactory) {
return;
}
_regularActionFactory = regularActionFactory;
_buttonMenuFactory.regularActionFactory = _regularActionFactory;
}
- (void)setIncognitoActionFactory:
(BrowserActionFactory*)incognitoActionFactory {
if (incognitoActionFactory == _incognitoActionFactory) {
return;
}
_incognitoActionFactory = incognitoActionFactory;
_buttonMenuFactory.incognitoActionFactory = _incognitoActionFactory;
}Since it now takes these in the initializer, does it need properties/setters to handle this after initialization?
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
@property(nonatomic, assign) WebStateList* currentWebStateList;Why does this need to become a property? I'm not sure I understand when it would change. As far as I can tell, it is only set in the initializer and in disconnect and I don't think you need it to be a property with a setter for that.
"+ios/chrome/browser/lens/ui_bundled/lens_availability.h",nit: add keep-sorted.
@optionalAre these really optional? If they are, you probably need to check whether the delegate implements them before calling (via `respondsToSelector:`). And then in order to have that work, your delegate protocol probably needs to include NSObject.
Related details
- Eric Ekey
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. |
Scott Yoder (Gerrit)
Scott Yoder added 2 comments![]( "Open in Gerrit")
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
Done
// TODO: commentScott Yodernit: fix comment.
Done
Related details
- Eric Ekey
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. |
Eric Ekey (Gerrit)
Eric Ekey added 7 comments![]( "Open in Gerrit")
@scott...@google.com Please take a first look
Thanks!
_buttonMenuFactory.currentWebStateList = _currentWebStateList;This wont be set at this point, so you could probably skip setting it here.
Done
- (void)setRegularActionFactory:(BrowserActionFactory*)regularActionFactory {
if (regularActionFactory == _regularActionFactory) {
return;
}
_regularActionFactory = regularActionFactory;
_buttonMenuFactory.regularActionFactory = _regularActionFactory;
}
- (void)setIncognitoActionFactory:
(BrowserActionFactory*)incognitoActionFactory {
if (incognitoActionFactory == _incognitoActionFactory) {
return;
}
_incognitoActionFactory = incognitoActionFactory;
_buttonMenuFactory.incognitoActionFactory = _incognitoActionFactory;
}Since it now takes these in the initializer, does it need properties/setters to handle this after initialization?
Not sure I understand what you mean here, could you clarify?. Though, this latest patchset removes the action factories as public properties and adds a public interface to set them.
@property(nonatomic, assign) WebStateList* currentWebStateList;Why does this need to become a property? I'm not sure I understand when it would change. As far as I can tell, it is only set in the initializer and in disconnect and I don't think you need it to be a property with a setter for that.
If we're sure it would never change, then yeah this doesn't need to be a property. This was a measure to ensure any changes would be properly reflected in the Menu Factory's WebStateList. Changed in latest patchset.
"+ios/chrome/browser/lens/ui_bundled/lens_availability.h",Eric Ekeynit: add keep-sorted.
Done
@optionalAre these really optional? If they are, you probably need to check whether the delegate implements them before calling (via `respondsToSelector:`). And then in order to have that work, your delegate protocol probably needs to include NSObject.
Yes, I believe these should all be optional. Not every delegate will have the buttons that require these actions (toolbar doesn't have a new tab button, tab grid won't have navigation arrows, etc.). Updated.
Related details
- Gauthier Ambard
- Scott Yoder
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. |
Scott Yoder (Gerrit)
Scott Yoder added 2 comments![]( "Open in Gerrit")
- (void)setRegularActionFactory:(BrowserActionFactory*)regularActionFactory {
if (regularActionFactory == _regularActionFactory) {
return;
}
_regularActionFactory = regularActionFactory;
_buttonMenuFactory.regularActionFactory = _regularActionFactory;
}
- (void)setIncognitoActionFactory:
(BrowserActionFactory*)incognitoActionFactory {
if (incognitoActionFactory == _incognitoActionFactory) {
return;
}
_incognitoActionFactory = incognitoActionFactory;
_buttonMenuFactory.incognitoActionFactory = _incognitoActionFactory;
}Eric EkeySince it now takes these in the initializer, does it need properties/setters to handle this after initialization?
Not sure I understand what you mean here, could you clarify?. Though, this latest patchset removes the action factories as public properties and adds a public interface to set them.
regularActionFactory and incognitoActionFactory are both parameters of the initializer - they are passed in there. So I'm wondering why we would still need setter methods. Are they used anywhere?
@property(nonatomic, assign) WebStateList* currentWebStateList;Eric EkeyWhy does this need to become a property? I'm not sure I understand when it would change. As far as I can tell, it is only set in the initializer and in disconnect and I don't think you need it to be a property with a setter for that.
If we're sure it would never change, then yeah this doesn't need to be a property. This was a measure to ensure any changes would be properly reflected in the Menu Factory's WebStateList. Changed in latest patchset.
A browser has a WebStateList. Items in the list may change, but the list itself doesn't change. Since the toolbar is browser scoped (unlike the App bar), I don't think this is needed.
Related details
- Eric Ekey
- Gauthier Ambard
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. |
Eric Ekey (Gerrit)
Eric Ekey added 2 comments![]( "Open in Gerrit")
- (void)setRegularActionFactory:(BrowserActionFactory*)regularActionFactory {
if (regularActionFactory == _regularActionFactory) {
return;
}
_regularActionFactory = regularActionFactory;
_buttonMenuFactory.regularActionFactory = _regularActionFactory;
}
- (void)setIncognitoActionFactory:
(BrowserActionFactory*)incognitoActionFactory {
if (incognitoActionFactory == _incognitoActionFactory) {
return;
}
_incognitoActionFactory = incognitoActionFactory;
_buttonMenuFactory.incognitoActionFactory = _incognitoActionFactory;
}Eric EkeySince it now takes these in the initializer, does it need properties/setters to handle this after initialization?
Scott YoderNot sure I understand what you mean here, could you clarify?. Though, this latest patchset removes the action factories as public properties and adds a public interface to set them.
regularActionFactory and incognitoActionFactory are both parameters of the initializer - they are passed in there. So I'm wondering why we would still need setter methods. Are they used anywhere?
FWIU if the incognito browser is updated from the coordinator, the incognito action factory would need to be updated as well to dispatch from the new incognito browser. Strictly speaking, this CL only calls the setter for the incognito factory, but I saw having setters for both as a way to prevent future pitfalls, since the mediator pushes that update to the menu factory. WDYT?
See `-[AppBarCoordinator setIncognitoBrowser:]`
@property(nonatomic, assign) WebStateList* currentWebStateList;Eric EkeyWhy does this need to become a property? I'm not sure I understand when it would change. As far as I can tell, it is only set in the initializer and in disconnect and I don't think you need it to be a property with a setter for that.
Scott YoderIf we're sure it would never change, then yeah this doesn't need to be a property. This was a measure to ensure any changes would be properly reflected in the Menu Factory's WebStateList. Changed in latest patchset.
A browser has a WebStateList. Items in the list may change, but the list itself doesn't change. Since the toolbar is browser scoped (unlike the App bar), I don't think this is needed.
- Gauthier Ambard
- Scott Yoder
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. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 9 comments![]( "Open in Gerrit")
surfaces for the new tab/group button on iPad. A stale TODO is removed.I like the idea!
However, I think the API should probably be revisited. In particular it feels a bit clunky for the Toolbar.
- (void)setRegularActionFactory:(BrowserActionFactory*)regularActionFactory;Remove this one, I don't think it is needed
- (void)setRegularActionFactory:(BrowserActionFactory*)regularActionFactory {
if (regularActionFactory == _regularActionFactory) {
return;
}
_regularActionFactory = regularActionFactory;
_buttonMenuFactory.regularActionFactory = _regularActionFactory;
}
- (void)setIncognitoActionFactory:
(BrowserActionFactory*)incognitoActionFactory {
if (incognitoActionFactory == _incognitoActionFactory) {
return;
}
_incognitoActionFactory = incognitoActionFactory;
_buttonMenuFactory.incognitoActionFactory = _incognitoActionFactory;
}Eric EkeySince it now takes these in the initializer, does it need properties/setters to handle this after initialization?
Scott YoderNot sure I understand what you mean here, could you clarify?. Though, this latest patchset removes the action factories as public properties and adds a public interface to set them.
Eric EkeyregularActionFactory and incognitoActionFactory are both parameters of the initializer - they are passed in there. So I'm wondering why we would still need setter methods. Are they used anywhere?
FWIU if the incognito browser is updated from the coordinator, the incognito action factory would need to be updated as well to dispatch from the new incognito browser. Strictly speaking, this CL only calls the setter for the incognito factory, but I saw having setters for both as a way to prevent future pitfalls, since the mediator pushes that update to the menu factory. WDYT?
See `-[AppBarCoordinator setIncognitoBrowser:]`
I would rather remove the setter for the regular one. It is not supposed to be updated.
_buttonMenuFactory.incognitoActionFactory = _actionFactory;I don't think this one should pass the incognito state nor the incognito action factory (especially as it is not the incognito one here).
Could we make the menu factory work in that case?
- (instancetype)initWithIncognitoState:(IncognitoState*)incognitoStateBased on other comment above, do we want to have the initializer take the ingontio state or do we want to pass maybe the factory to the method?
@property(nonatomic, assign) WebStateList* currentWebStateList;It feels weird that the WSL is the "current one" but the factory are based on the incognito state.
CHECK(self.delegate);You can't CHECK a delegate: delegate can be nil and can not respond to selector if they are optional (otherwise, don't make them optional).
CHECK(_templateURLService);If you CHECK it, should it be in the init instead?
@optionalI wonder if I wouldn't prefer having them mandatory with NOTREACHED in the one that should not be implemented. It is more aligned with the delegate pattern vs CHECK(respondToSelector).
Related details
- Eric Ekey
- Scott Yoder
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. |