Add a permission details subpage for Automatic Picture-in-Picture [chromium/src : main]
Christian Dullweber (Gerrit)
Christian Dullweber added 2 comments![]( "Open in Gerrit")
sorry, I'm ooo for the next week and didn't quite get to this review. Could you ask one of the other pageinfo owners to continue the review?
This change adds a subpage to the Automatic Picture-in-Picture contentThis change also seems to add a webui settings page? Could you add that to the description?
Related details
- Benjamin Keen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 2 comments![]( "Open in Gerrit")
sorry, I'm ooo for the next week and didn't quite get to this review. Could you ask one of the other pageinfo owners to continue the review?
Absolutely, enjoy your time off!
This change adds a subpage to the Automatic Picture-in-Picture contentThis change also seems to add a webui settings page? Could you add that to the description?
- Christian Dullweber
- Emily Stark
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emily Stark (Gerrit)
Emily Stark voted Code-Review+1![]( "Open in Gerrit")
- Benjamin Keen
- Christian Dullweber
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
Adding:
* @dpen...@chromium.org for c/b/u/chrome_pages.cc
* @the...@chromium.org for c/c/webui_url_constants.h
* @jop...@chromium.org for m/b/media_switches.*
Tx!
Related details
- David Pennington
- Jordan Bayles
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted Code-Review+1![]( "Open in Gerrit")
- Benjamin Keen
- David Pennington
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jordan Bayles
| Code-Review | +1 |
2 comments
media/base LGTM % feedback.
MEDIA_EXPORT bool IsAutoPictureInPicturePageInfoDetailsEnabled();Since this is only used once? consider deleting this function in favor of just checking the feature list directly.
I think these helpers can be useful if whether something is enabled depends on more than just the base::Feature (e.g. build flags, configs), but since it's just wrapping a feature list check I actually think it makes things less clear.
Related details
- Benjamin Keen
- David Pennington
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
MEDIA_EXPORT bool IsAutoPictureInPicturePageInfoDetailsEnabled();Since this is only used once? consider deleting this function in favor of just checking the feature list directly.
I think these helpers can be useful if whether something is enabled depends on more than just the base::Feature (e.g. build flags, configs), but since it's just wrapping a feature list check I actually think it makes things less clear.
I think the method saves on visual clutter, but I agree that not having it does make things clearer. Updated.
Related details
- David Pennington
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |