[site_info] Create UI for TrackingProtectionSettings section [chromium/src : main]
Gauthier Ambard (Gerrit)
Gauthier Ambard added 6 comments![]( "Open in Gerrit")
- (GURL)currentURL {Can you put the private methods in a#pragma mark - Private?
// TODO: Add logic to open the Settings page.What is supposed to be opened here? Can't the VC open it?
[snapshot
appendItemsWithIdentifiers:@[ @(ItemIdentifierTrackingProtection) ]
intoSectionWithIdentifier:@(SectionIdentifierTrackingProtection)];
[snapshot
appendItemsWithIdentifiers:@[ @(
ItemIdentifierTrackingProtectionButton) ]
intoSectionWithIdentifier:@(SectionIdentifierTrackingProtection)];Can't you bundle both?
DequeueTableViewCell<TableViewDetailIconCell>(tableView);Why not using the same as the other cells? That would ensure they have the same design.
DequeueTableViewCell<TableViewTextCell>(tableView);Same, you should be able to reuse ContentConfiguration?
- (void)createDetailTextLabel;Use content configuration and remove that.
Related details
- Cam Patton
- Chirag Arora
- Fiona Macintosh
- Maranda Bodas
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 4 comments![]( "Open in Gerrit")
// TODO: Add logic to open the Settings page.What is supposed to be opened here? Can't the VC open it?
The settings page which is specific for Tracking Protection. Therefore, I thought this was a more appropriate place to put it.
DequeueTableViewCell<TableViewDetailIconCell>(tableView);Why not using the same as the other cells? That would ensure they have the same design.
They are all extended from the same base class but there is a specific type of cell for each use-case. Here we want to show the cell with an icon and detail text. Hence, the usage of this class. Visually, it blends in with other cells and looks exactly as it should.
Here are all the cell types the library offers: https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/
DequeueTableViewCell<TableViewTextCell>(tableView);Same, you should be able to reuse ContentConfiguration?
Are you referring to [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)?
- (void)createDetailTextLabel;Use content configuration and remove that.
I tried using that initially, but it doesn't offers a way to set the detail text like we want. There is a `subtitle` property but it doesn't has correct padding we need for the `title`. Plus, we'd like to set an `NSAttributedString` to highlight the "Send feedback" text. But `subtitle` is of type `NSString`.
Let me know if you are referring to some other class. Because I was trying to use [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)
Related details
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 3 comments![]( "Open in Gerrit")
// TODO: Add logic to open the Settings page.Chirag AroraWhat is supposed to be opened here? Can't the VC open it?
The settings page which is specific for Tracking Protection. Therefore, I thought this was a more appropriate place to put it.
Mediators are supposed to interact with the model. This looks like a command that can be sent from the VC.
DequeueTableViewCell<TableViewDetailIconCell>(tableView);Chirag AroraWhy not using the same as the other cells? That would ensure they have the same design.
They are all extended from the same base class but there is a specific type of cell for each use-case. Here we want to show the cell with an icon and detail text. Hence, the usage of this class. Visually, it blends in with other cells and looks exactly as it should.
Here are all the cell types the library offers: https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/
That's no longer true. `TableViewDetailIconCell` inherit from `LegacyTableViewCell` and cells from `TableViewCellContentConfiguration` are `TableViewCell`.
Yes they are very similar but I am also in the middle of unifying them so it would be nice if this is using the new ones directly.
Is there something missing from the configurations preventing you from using it?
DequeueTableViewCell<TableViewTextCell>(tableView);Chirag AroraSame, you should be able to reuse ContentConfiguration?
Are you referring to [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)?
- Chirag Arora
- Fiona Macintosh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 2 comments![]( "Open in Gerrit")
NSMutableAttributedString* detailText = [[NSMutableAttributedString
alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT)];
NSAttributedString* sendFeedbackText = [[NSAttributedString alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK)
attributes:@{
NSForegroundColorAttributeName :
[UIColor colorNamed:kBlueColor],
}];
[detailText appendAttributedString:[[NSAttributedString alloc]
initWithString:kSpaceDelimiter]];
[detailText appendAttributedString:sendFeedbackText];I am not sure this works for all languages. Could you make it a unique string for translation?
- (void)createDetailTextLabel;Chirag AroraUse content configuration and remove that.
I tried using that initially, but it doesn't offers a way to set the detail text like we want. There is a `subtitle` property but it doesn't has correct padding we need for the `title`. Plus, we'd like to set an `NSAttributedString` to highlight the "Send feedback" text. But `subtitle` is of type `NSString`.
Let me know if you are referring to some other class. Because I was trying to use [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)
Ah, I haven't added support for attributed strings yet, true. Maybe I should do that. Can you use a string for now?
Chirag Arora (Gerrit)
Chirag Arora added 1 comment![]( "Open in Gerrit")
DequeueTableViewCell<TableViewDetailIconCell>(tableView);Chirag AroraWhy not using the same as the other cells? That would ensure they have the same design.
Gauthier AmbardThey are all extended from the same base class but there is a specific type of cell for each use-case. Here we want to show the cell with an icon and detail text. Hence, the usage of this class. Visually, it blends in with other cells and looks exactly as it should.
Here are all the cell types the library offers: https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/
That's no longer true. `TableViewDetailIconCell` inherit from `LegacyTableViewCell` and cells from `TableViewCellContentConfiguration` are `TableViewCell`.
Yes they are very similar but I am also in the middle of unifying them so it would be nice if this is using the new ones directly.
Is there something missing from the configurations preventing you from using it?
With `TableViewCellContentConfiguration`, is there a way to add a detail text of type `NSAttribuedString`?
Also, refer to this previous comment: https://chromium-review.googlesource.com/c/chromium/src/+/6983503/comment/8f57348f_46113376/
Related details
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 2 comments![]( "Open in Gerrit")
NSMutableAttributedString* detailText = [[NSMutableAttributedString
alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT)];
NSAttributedString* sendFeedbackText = [[NSAttributedString alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK)
attributes:@{
NSForegroundColorAttributeName :
[UIColor colorNamed:kBlueColor],
}];
[detailText appendAttributedString:[[NSAttributedString alloc]
initWithString:kSpaceDelimiter]];
[detailText appendAttributedString:sendFeedbackText];I am not sure this works for all languages. Could you make it a unique string for translation?
"Send feedback" is actually not part of the sentence. It's after the full-stop and I think should be treated like a separate string.
- (void)createDetailTextLabel;Chirag AroraUse content configuration and remove that.
Gauthier AmbardI tried using that initially, but it doesn't offers a way to set the detail text like we want. There is a `subtitle` property but it doesn't has correct padding we need for the `title`. Plus, we'd like to set an `NSAttributedString` to highlight the "Send feedback" text. But `subtitle` is of type `NSString`.
Let me know if you are referring to some other class. Because I was trying to use [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)
Ah, I haven't added support for attributed strings yet, true. Maybe I should do that. Can you use a string for now?
Then I cannot highlight the "Send feedback" text which should look like a link.
Fiona Macintosh (Gerrit)
Fiona Macintosh added 1 comment![]( "Open in Gerrit")
<!-- Site Info (iOS) -->
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER" meaning="DoNotTranslate" desc="Footer text shown below the Tracking Protection section of the 'Site Info' sheet. Provides a quick link to the Tracking Protection Settings page by highlighting the 'Settings' part of the text.">
Review available Incognito tracking protections in <ph name="BEGIN_LINK">BEGIN_LINK</ph>Settings<ph name="END_LINK">END_LINK</ph>.
</message>
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT" meaning="DoNotTranslate" desc="Descriptive text on the 'Site Info' sheet shown when tracking protections are paused on the current site. Explains that tracking protections will remain paused on this site until the user closes all Incognito tabs.">
Protections are paused until all Incognito tabs are closed.
</message>
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK" meaning="DoNotTranslate" desc="Part of the descriptive text on the 'Site Info' sheet shown when tracking protections are paused on the current site. This is part of the feedback link so the user can give feedback as to why they paused tracking protections on this site.">
Send feeback
</message>
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_RESUMED" meaning="DoNotTranslate" desc="Title of tracking protection section when tracking protection is enabled for the current site.">
Site not working?
</message>Please follow instructions I wrote [here](https://docs.google.com/document/d/1gx3qCIo3pY_9aE7LIpny0w0kqgT0rFJWHz5K-egtavA/edit?tab=t.0#bookmark=id.rihyix3zcf8y) and use `translateable="false"` rather than `meaning="DoNotTranslate"`. Then remove `Skip-Translation-Screenshots-Check: True` from your CL description; if you're correctly marking these strings as not yet translateable then it's not needed
Related details
- Chirag Arora
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 8 comments![]( "Open in Gerrit")
<!-- Site Info (iOS) -->
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER" meaning="DoNotTranslate" desc="Footer text shown below the Tracking Protection section of the 'Site Info' sheet. Provides a quick link to the Tracking Protection Settings page by highlighting the 'Settings' part of the text.">
Review available Incognito tracking protections in <ph name="BEGIN_LINK">BEGIN_LINK</ph>Settings<ph name="END_LINK">END_LINK</ph>.
</message>
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT" meaning="DoNotTranslate" desc="Descriptive text on the 'Site Info' sheet shown when tracking protections are paused on the current site. Explains that tracking protections will remain paused on this site until the user closes all Incognito tabs.">
Protections are paused until all Incognito tabs are closed.
</message>
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK" meaning="DoNotTranslate" desc="Part of the descriptive text on the 'Site Info' sheet shown when tracking protections are paused on the current site. This is part of the feedback link so the user can give feedback as to why they paused tracking protections on this site.">
Send feeback
</message>
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_RESUMED" meaning="DoNotTranslate" desc="Title of tracking protection section when tracking protection is enabled for the current site.">
Site not working?
</message>Please follow instructions I wrote [here](https://docs.google.com/document/d/1gx3qCIo3pY_9aE7LIpny0w0kqgT0rFJWHz5K-egtavA/edit?tab=t.0#bookmark=id.rihyix3zcf8y) and use `translateable="false"` rather than `meaning="DoNotTranslate"`. Then remove `Skip-Translation-Screenshots-Check: True` from your CL description; if you're correctly marking these strings as not yet translateable then it's not needed
Done
Can you put the private methods in a#pragma mark - Private?
Done
Chirag AroraWhat is supposed to be opened here? Can't the VC open it?
Gauthier AmbardThe settings page which is specific for Tracking Protection. Therefore, I thought this was a more appropriate place to put it.
Mediators are supposed to interact with the model. This looks like a command that can be sent from the VC.
Done
[snapshot
appendItemsWithIdentifiers:@[ @(ItemIdentifierTrackingProtection) ]
intoSectionWithIdentifier:@(SectionIdentifierTrackingProtection)];
[snapshot
appendItemsWithIdentifiers:@[ @(
ItemIdentifierTrackingProtectionButton) ]
intoSectionWithIdentifier:@(SectionIdentifierTrackingProtection)];Can't you bundle both?
Done
DequeueTableViewCell<TableViewDetailIconCell>(tableView);Chirag AroraWhy not using the same as the other cells? That would ensure they have the same design.
Gauthier AmbardThey are all extended from the same base class but there is a specific type of cell for each use-case. Here we want to show the cell with an icon and detail text. Hence, the usage of this class. Visually, it blends in with other cells and looks exactly as it should.
Here are all the cell types the library offers: https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/
Chirag AroraThat's no longer true. `TableViewDetailIconCell` inherit from `LegacyTableViewCell` and cells from `TableViewCellContentConfiguration` are `TableViewCell`.
Yes they are very similar but I am also in the middle of unifying them so it would be nice if this is using the new ones directly.
Is there something missing from the configurations preventing you from using it?
With `TableViewCellContentConfiguration`, is there a way to add a detail text of type `NSAttribuedString`?
Also, refer to this previous comment: https://chromium-review.googlesource.com/c/chromium/src/+/6983503/comment/8f57348f_46113376/
Done
NSMutableAttributedString* detailText = [[NSMutableAttributedString
alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT)];
NSAttributedString* sendFeedbackText = [[NSAttributedString alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK)
attributes:@{
NSForegroundColorAttributeName :
[UIColor colorNamed:kBlueColor],
}];
[detailText appendAttributedString:[[NSAttributedString alloc]
initWithString:kSpaceDelimiter]];
[detailText appendAttributedString:sendFeedbackText];Chirag AroraI am not sure this works for all languages. Could you make it a unique string for translation?
"Send feedback" is actually not part of the sentence. It's after the full-stop and I think should be treated like a separate string.
Done
Chirag AroraSame, you should be able to reuse ContentConfiguration?
Gauthier AmbardAre you referring to [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)?
Yes
Done
Chirag AroraUse content configuration and remove that.
Gauthier AmbardI tried using that initially, but it doesn't offers a way to set the detail text like we want. There is a `subtitle` property but it doesn't has correct padding we need for the `title`. Plus, we'd like to set an `NSAttributedString` to highlight the "Send feedback" text. But `subtitle` is of type `NSString`.
Let me know if you are referring to some other class. Because I was trying to use [TableViewCellContentConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h)
Chirag AroraAh, I haven't added support for attributed strings yet, true. Maybe I should do that. Can you use a string for now?
Then I cannot highlight the "Send feedback" text which should look like a link.
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted Code-Review+1![]( "Open in Gerrit")
- Chirag Arora
- Fiona Macintosh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiona Macintosh (Gerrit)
Fiona Macintosh added 15 comments![]( "Open in Gerrit")
This commit adds the frontend UI components for the TrackingProtectionSettings section on the SiteInfo sheet. The logic for dispatching initial state of the UI from the model has also been added.nit: this line is over the limit, please split onto multiple lines
Please add a screenshot of your local build along with the mock (ex: crrev.com/c/6972505)
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER" translateable="false" desc="Footer text shown below the Tracking Protection section of the 'Site Info' sheet. Provides a quick link to the Tracking Protection Settings page by highlighting the 'Settings' part of the text.">```suggestion
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER" translateable="false" desc="Footer text shown below the tracking protections section of the 'Site Info' sheet. Provides a link to the tracking protections settings page.">
```
Remember that "Tracking Protection" is a separate concept, using it in these string descriptions may confuse translators. In general, the purpose of these strings is to help translators, so to ensure consistent translations we should be consistent with our language in these descriptions. The string descriptions you've added have variance in terms of capitalization and pluralization, and we want to avoid this to ensure the translated strings are consistent and accurate
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK" translateable="false" desc="Part of the descriptive text on the 'Site Info' sheet shown when tracking protections are paused on the current site. This is part of the feedback link so the user can give feedback as to why they paused tracking protections on this site.">```suggestion
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK" translateable="false" desc="A send feedback link that appears as part of the descriptive text on the 'Site Info' sheet when tracking protections are paused on the current site. Allows the user to give feedback on why they paused tracking protections on this site.">
```
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_RESUMED" translateable="false" desc="Title of tracking protection section when tracking protection is enabled for the current site.">```suggestion
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_RESUMED" translateable="false" desc="">
```
nit: would just leave this as false for now, this is a tricky string to come up with a good description for and we can fill it out later when we mark the strings for translation
const GURL& URL = [self currentURL];
return _trackingProtectionSettings->HasTrackingProtectionException(URL);```suggestion
return _trackingProtectionSettings->HasTrackingProtectionException([self currentURL]);
```
nit: don't think you need the local variable here (go/totw/161)
// TODO: implement the toggling logic here.nit: TODOs should always have a bug attached
```suggestion
// TODO(crbug.com/442799468): Implement toggling logic.
```
@implementation PageInfoTrackingProtectionInfo
@endAdd a TODO to fill this out?
// Coordinator for tracking protection settings.Mediator?
// URL to identify where the request is coming from to the delegate.This comment doesn't line up with the usage IIUC, isn't this URL for the link in the tracking protections footer?
"settings://open_tracking_protection_settings";OOC does this URL work or is it a placeholder?
case ItemIdentifierTrackingProtection: {
// TODO: implement tap functionality for the info cell.
break;
}
case ItemIdentifierTrackingProtectionButton: {
// TODO: implement tap functionality for the button cell.
break;
}Same as in other file, attach these TODOs to your bug
TableViewLinkHeaderFooterView* footer =
DequeueTableViewHeaderFooter<TableViewLinkHeaderFooterView>(
self.tableView);
footer.delegate = self;
footer.urls = @[ [[CrURL alloc]
initWithGURL:GURL(kTrackingProtectionSettingsURL)] ];
[footer setText:l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER)
withColor:[UIColor colorNamed:kTextSecondaryColor]];
return footer;```suggestion
TableViewLinkHeaderFooterView* footer =
DequeueTableViewHeaderFooter<TableViewLinkHeaderFooterView>(
self.tableView);
footer.delegate = self;
footer.urls = @[ [[CrURL alloc]
initWithGURL:GURL(kTrackingProtectionSettingsURL)] ];
[footer setText:l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER)
withColor:[UIColor colorNamed:kTextSecondaryColor]];
return footer;
```
nit: avoid unnecessary vertical whitespace (go/cstyle#Vertical_Whitespace)
// TODO: Add logic to open the Settings page.Same as elsewhere, attach this to a bug
NSMutableAttributedString* detailText = [[NSMutableAttributedString
alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT)];
NSAttributedString* sendFeedbackText = [[NSAttributedString alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK)
attributes:@{
NSForegroundColorAttributeName :
[UIColor colorNamed:kBlueColor],
}];
[detailText appendAttributedString:[[NSAttributedString alloc]
initWithString:kSpaceDelimiter]];
[detailText appendAttributedString:sendFeedbackText];Chirag AroraI am not sure this works for all languages. Could you make it a unique string for translation?
Chirag Arora"Send feedback" is actually not part of the sentence. It's after the full-stop and I think should be treated like a separate string.
Done
This works from a translation perspective, but why can't it be part of the `detailText` string like the [android string](https://source.chromium.org/chromium/chromium/src/+/main:components/privacy_sandbox_strings.grd;l=416;drc=1a2142329abc0be6b6c3590be11764858bf4ae54)?
Related details
- Chirag Arora
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 4 comments![]( "Open in Gerrit")
@implementation PageInfoTrackingProtectionInfo
@endAdd a TODO to fill this out?
Actually, it will remain like this.
// URL to identify where the request is coming from to the delegate.This comment doesn't line up with the usage IIUC, isn't this URL for the link in the tracking protections footer?
It's not really a URL in itself, just an identifier as I understand from other places in the codebase. Since the handler is common for all links, the identifier helps in triggering the correct code to open the page we want.
Example: https://source.chromium.org/search?q=%22settings:%2F%2F%22&sq=
"settings://open_tracking_protection_settings";OOC does this URL work or is it a placeholder?
Just an identifier. Check lines 321-323.
NSMutableAttributedString* detailText = [[NSMutableAttributedString
alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT)];
NSAttributedString* sendFeedbackText = [[NSAttributedString alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK)
attributes:@{
NSForegroundColorAttributeName :
[UIColor colorNamed:kBlueColor],
}];
[detailText appendAttributedString:[[NSAttributedString alloc]
initWithString:kSpaceDelimiter]];
[detailText appendAttributedString:sendFeedbackText];Chirag AroraI am not sure this works for all languages. Could you make it a unique string for translation?
Chirag Arora"Send feedback" is actually not part of the sentence. It's after the full-stop and I think should be treated like a separate string.
Fiona MacintoshDone
This works from a translation perspective, but why can't it be part of the `detailText` string like the [android string](https://source.chromium.org/chromium/chromium/src/+/main:components/privacy_sandbox_strings.grd;l=416;drc=1a2142329abc0be6b6c3590be11764858bf4ae54)?
It's because we don't have specific class properties to handle the templated links in strings for `detailText`. Recall links aren't supported directly here and we need to make the entire cell a touch target. For the Settings link in the footer, it's all a single string because that use-case is supported natively.
Related details
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiona Macintosh (Gerrit)
Fiona Macintosh added 4 comments![]( "Open in Gerrit")
@implementation PageInfoTrackingProtectionInfo
@endChirag AroraAdd a TODO to fill this out?
Actually, it will remain like this.
Gotcha, thanks!
// URL to identify where the request is coming from to the delegate.Chirag AroraThis comment doesn't line up with the usage IIUC, isn't this URL for the link in the tracking protections footer?
It's not really a URL in itself, just an identifier as I understand from other places in the codebase. Since the handler is common for all links, the identifier helps in triggering the correct code to open the page we want.
Example: https://source.chromium.org/search?q=%22settings:%2F%2F%22&sq=
```suggestion
// Used as an identifier to open the tracking protection settings page.
```
Got it, then I would update the comment to something like this
"settings://open_tracking_protection_settings";Chirag AroraOOC does this URL work or is it a placeholder?
Just an identifier. Check lines 321-323.
Resolved via other comment
Yeah I understand we can't support a link on the text directly, but I'm not sure how that connects to us needing a separate string for the "send feedback" text; as you say, the whole cell is going to be the touch target anyway. IIUC we can just have one string and then only provide the color formatting to the range of the "send feedback" text
Related details
- Chirag Arora
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 1 comment![]( "Open in Gerrit")
Yes, but to get the range of "Send feedback" text, we'll need to have it stored separately as a string anyway since, this is not static and will change for different locales.
Related details
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiona Macintosh (Gerrit)
Fiona Macintosh added 1 comment![]( "Open in Gerrit")
Yes, it's not static, which is why we have the `BEGIN_LINK` and `END_LINK` tags and [this function](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/common/string_util.h;l=49-52;drc=05d8a25b5b0b09734619068a7247c4c4f3db732c) to retrieve the range. I understand that the whole cell will end up being a link, but for the purposes of applying styling to the string - and for the sake of translations - making it one string with these tags is the right approach
Related details
- Chirag Arora
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 1 comment![]( "Open in Gerrit")
const GURL& URL = [self currentURL];
return _trackingProtectionSettings->HasTrackingProtectionException(URL);```suggestion
return _trackingProtectionSettings->HasTrackingProtectionException([self currentURL]);
```
nit: don't think you need the local variable here (go/totw/161)
Will add it in the next CL along with the toggling logic.
Related details
- Chirag Arora
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 1 comment![]( "Open in Gerrit")
const GURL& URL = [self currentURL];
return _trackingProtectionSettings->HasTrackingProtectionException(URL);Chirag Arora```suggestion
return _trackingProtectionSettings->HasTrackingProtectionException([self currentURL]);
```
nit: don't think you need the local variable here (go/totw/161)
Will add it in the next CL along with the toggling logic.
- Fiona Macintosh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 13 comments![]( "Open in Gerrit")
This commit adds the frontend UI components for the TrackingProtectionSettings section on the SiteInfo sheet. The logic for dispatching initial state of the UI from the model has also been added.nit: this line is over the limit, please split onto multiple lines
Done
Please add a screenshot of your local build along with the mock (ex: crrev.com/c/6972505)
Done
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER" translateable="false" desc="Footer text shown below the Tracking Protection section of the 'Site Info' sheet. Provides a quick link to the Tracking Protection Settings page by highlighting the 'Settings' part of the text.">```suggestion
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER" translateable="false" desc="Footer text shown below the tracking protections section of the 'Site Info' sheet. Provides a link to the tracking protections settings page.">
```
Remember that "Tracking Protection" is a separate concept, using it in these string descriptions may confuse translators. In general, the purpose of these strings is to help translators, so to ensure consistent translations we should be consistent with our language in these descriptions. The string descriptions you've added have variance in terms of capitalization and pluralization, and we want to avoid this to ensure the translated strings are consistent and accurate
Done
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK" translateable="false" desc="Part of the descriptive text on the 'Site Info' sheet shown when tracking protections are paused on the current site. This is part of the feedback link so the user can give feedback as to why they paused tracking protections on this site.">```suggestion
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK" translateable="false" desc="A send feedback link that appears as part of the descriptive text on the 'Site Info' sheet when tracking protections are paused on the current site. Allows the user to give feedback on why they paused tracking protections on this site.">
```
Done
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_RESUMED" translateable="false" desc="Title of tracking protection section when tracking protection is enabled for the current site.">```suggestion
<message name="IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_RESUMED" translateable="false" desc="">
```
nit: would just leave this as false for now, this is a tricky string to come up with a good description for and we can fill it out later when we mark the strings for translation
Done
const GURL& URL = [self currentURL];
return _trackingProtectionSettings->HasTrackingProtectionException(URL);Chirag Arora```suggestion
return _trackingProtectionSettings->HasTrackingProtectionException([self currentURL]);
```
nit: don't think you need the local variable here (go/totw/161)
Chirag AroraWill add it in the next CL along with the toggling logic.
Sorry, ignore this.
Done
nit: TODOs should always have a bug attached
```suggestion
// TODO(crbug.com/442799468): Implement toggling logic.
```
Done
// Coordinator for tracking protection settings.Chirag AroraMediator?
Done
// URL to identify where the request is coming from to the delegate.Chirag AroraThis comment doesn't line up with the usage IIUC, isn't this URL for the link in the tracking protections footer?
Fiona MacintoshIt's not really a URL in itself, just an identifier as I understand from other places in the codebase. Since the handler is common for all links, the identifier helps in triggering the correct code to open the page we want.
Example: https://source.chromium.org/search?q=%22settings:%2F%2F%22&sq=
```suggestion
// Used as an identifier to open the tracking protection settings page.
```
Got it, then I would update the comment to something like this
Done
case ItemIdentifierTrackingProtection: {
// TODO: implement tap functionality for the info cell.
break;
}
case ItemIdentifierTrackingProtectionButton: {
// TODO: implement tap functionality for the button cell.
break;
}Same as in other file, attach these TODOs to your bug
Done
TableViewLinkHeaderFooterView* footer =
DequeueTableViewHeaderFooter<TableViewLinkHeaderFooterView>(
self.tableView);
footer.delegate = self;
footer.urls = @[ [[CrURL alloc]
initWithGURL:GURL(kTrackingProtectionSettingsURL)] ];
[footer setText:l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER)
withColor:[UIColor colorNamed:kTextSecondaryColor]];
return footer;```suggestion
TableViewLinkHeaderFooterView* footer =
DequeueTableViewHeaderFooter<TableViewLinkHeaderFooterView>(
self.tableView);
footer.delegate = self;
footer.urls = @[ [[CrURL alloc]
initWithGURL:GURL(kTrackingProtectionSettingsURL)] ];
[footer setText:l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_SECTION_FOOTER)
withColor:[UIColor colorNamed:kTextSecondaryColor]];
return footer;
```
nit: avoid unnecessary vertical whitespace (go/cstyle#Vertical_Whitespace)
Done
Same as elsewhere, attach this to a bug
Done
NSMutableAttributedString* detailText = [[NSMutableAttributedString
alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT)];
NSAttributedString* sendFeedbackText = [[NSAttributedString alloc]
initWithString:
l10n_util::GetNSString(
IDS_IOS_PAGE_INFO_TRACKING_PROTECTIONS_PAUSED_DETAIL_TEXT_SEND_FEEDBACK)
attributes:@{
NSForegroundColorAttributeName :
[UIColor colorNamed:kBlueColor],
}];
[detailText appendAttributedString:[[NSAttributedString alloc]
initWithString:kSpaceDelimiter]];
[detailText appendAttributedString:sendFeedbackText];Chirag AroraI am not sure this works for all languages. Could you make it a unique string for translation?
Chirag Arora"Send feedback" is actually not part of the sentence. It's after the full-stop and I think should be treated like a separate string.
Fiona MacintoshDone
Chirag AroraThis works from a translation perspective, but why can't it be part of the `detailText` string like the [android string](https://source.chromium.org/chromium/chromium/src/+/main:components/privacy_sandbox_strings.grd;l=416;drc=1a2142329abc0be6b6c3590be11764858bf4ae54)?
Fiona MacintoshIt's because we don't have specific class properties to handle the templated links in strings for `detailText`. Recall links aren't supported directly here and we need to make the entire cell a touch target. For the Settings link in the footer, it's all a single string because that use-case is supported natively.
Chirag AroraYeah I understand we can't support a link on the text directly, but I'm not sure how that connects to us needing a separate string for the "send feedback" text; as you say, the whole cell is going to be the touch target anyway. IIUC we can just have one string and then only provide the color formatting to the range of the "send feedback" text
Fiona MacintoshYes, but to get the range of "Send feedback" text, we'll need to have it stored separately as a string anyway since, this is not static and will change for different locales.
Yes, it's not static, which is why we have the `BEGIN_LINK` and `END_LINK` tags and [this function](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/common/string_util.h;l=49-52;drc=05d8a25b5b0b09734619068a7247c4c4f3db732c) to retrieve the range. I understand that the whole cell will end up being a link, but for the purposes of applying styling to the string - and for the sake of translations - making it one string with these tags is the right approach
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiona Macintosh (Gerrit)
Fiona Macintosh voted and added 5 comments![]( "Open in Gerrit")
Votes added by Fiona Macintosh
| Code-Review | +1 |
5 comments
// Determines if the model currently has an exception for the current site.```suggestion
// Determines if there is an exception for the current site.
```
TableViewDetailIconCell* cell =
DequeueTableViewCell<TableViewDetailIconCell>(tableView);
cell.accessoryType = UITableViewCellAccessoryNone;```suggestion
TableViewDetailIconCell* cell =
DequeueTableViewCell<TableViewDetailIconCell>(tableView);
cell.accessoryType = UITableViewCellAccessoryNone;
```
DCHECK_EQ(parsedDetailText.ranges.size(), 1UL);IIUC DCHECK is deprecated, let's just use CHECK instead
TableViewCellContentConfiguration* configuration =
[[TableViewCellContentConfiguration alloc] init];
if (_trackingProtectionInfo.hasTrackingProtectionException) {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_RESUME_PROTECTIONS_LABEL);
} else {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_PAUSE_PROTECTIONS_LABEL);
}
configuration.titleColor = [UIColor colorNamed:kBlueColor];
TableViewCell* cell =
[TableViewCellContentConfiguration dequeueTableViewCell:tableView];
cell.contentConfiguration = configuration;
return cell;```suggestion
TableViewCellContentConfiguration* configuration =
[[TableViewCellContentConfiguration alloc] init];
if (_trackingProtectionInfo.hasTrackingProtectionException) {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_RESUME_PROTECTIONS_LABEL);
} else {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_PAUSE_PROTECTIONS_LABEL);
}
configuration.titleColor = [UIColor colorNamed:kBlueColor];
TableViewCell* cell =
[TableViewCellContentConfiguration dequeueTableViewCell:tableView];
cell.contentConfiguration = configuration;
return cell;
```
nit: reducing more unneeded blank lines
Related details
- Chirag Arora
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 3 comments![]( "Open in Gerrit")
// Determines if the model currently has an exception for the current site.```suggestion
// Determines if there is an exception for the current site.
```
Fix applied.
TableViewDetailIconCell* cell =
DequeueTableViewCell<TableViewDetailIconCell>(tableView);
cell.accessoryType = UITableViewCellAccessoryNone;```suggestion
TableViewDetailIconCell* cell =
DequeueTableViewCell<TableViewDetailIconCell>(tableView);
cell.accessoryType = UITableViewCellAccessoryNone;
```
Fix applied.
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(parsedDetailText.ranges.size(), 1UL);IIUC DCHECK is deprecated, let's just use CHECK instead
Done
Related details
- Fiona Macintosh
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fiona Macintosh (Gerrit)
Fiona Macintosh voted Code-Review+1![]( "Open in Gerrit")
- Chirag Arora
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chirag Arora (Gerrit)
Chirag Arora added 3 comments![]( "Open in Gerrit")
// Determines if the model currently has an exception for the current site.Chirag Arora```suggestion
// Determines if there is an exception for the current site.
```
Fix applied.
Done
TableViewDetailIconCell* cell =
DequeueTableViewCell<TableViewDetailIconCell>(tableView);
cell.accessoryType = UITableViewCellAccessoryNone;Chirag Arora```suggestion
TableViewDetailIconCell* cell =
DequeueTableViewCell<TableViewDetailIconCell>(tableView);
cell.accessoryType = UITableViewCellAccessoryNone;
```
Fix applied.
Done
TableViewCellContentConfiguration* configuration =
[[TableViewCellContentConfiguration alloc] init];
if (_trackingProtectionInfo.hasTrackingProtectionException) {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_RESUME_PROTECTIONS_LABEL);
} else {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_PAUSE_PROTECTIONS_LABEL);
}
configuration.titleColor = [UIColor colorNamed:kBlueColor];
TableViewCell* cell =
[TableViewCellContentConfiguration dequeueTableViewCell:tableView];
cell.contentConfiguration = configuration;
return cell;Chirag Arora```suggestion
TableViewCellContentConfiguration* configuration =
[[TableViewCellContentConfiguration alloc] init];
if (_trackingProtectionInfo.hasTrackingProtectionException) {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_RESUME_PROTECTIONS_LABEL);
} else {
configuration.title = l10n_util::GetNSString(
IDS_TRACKING_PROTECTIONS_BUTTON_PAUSE_PROTECTIONS_LABEL);
}
configuration.titleColor = [UIColor colorNamed:kBlueColor];TableViewCell* cell =
[TableViewCellContentConfiguration dequeueTableViewCell:tableView];
cell.contentConfiguration = configuration;
return cell;
```
nit: reducing more unneeded blank lines
Fix applied.
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |