media: Provide detailed UserAgent string for provisioning [chromium/src : main]
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Feras Aldahlawi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Feras Aldahlawi (Gerrit)
Feras Aldahlawi added 1 comment![]( "Open in Gerrit")
const std::string& GetDetailedUserAgent() {We already have android specific directory. Minimize buildflag usage in this file and add a new android only functionality in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/android/?q=f:content%20f:browser%20f:media%20f:android
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
const std::string& GetDetailedUserAgent() {We already have android specific directory. Minimize buildflag usage in this file and add a new android only functionality in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/android/?q=f:content%20f:browser%20f:media%20f:android
This function is pretty small and is directly related to the provisioning request which is why I kept it here in the anonymous namespace.
Related details
- Feras Aldahlawi
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. |
Feras Aldahlawi (Gerrit)
Feras Aldahlawi added 1 comment![]( "Open in Gerrit")
const std::string& GetDetailedUserAgent() {Vikram PasupathyWe already have android specific directory. Minimize buildflag usage in this file and add a new android only functionality in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/android/?q=f:content%20f:browser%20f:media%20f:android
This function is pretty small and is directly related to the provisioning request which is why I kept it here in the anonymous namespace.
Creations of provision fetcher is already platform dependent. You can add another parameter to `CreateProvisionFetcher` that takes a custom user agent with a reasonable default and store that as member variable.
Then in `Retrieve` call, you can use the custom user agent without buildflags.
That is better because it removes the custom logic from this common implementation and make it the responsibility of a platform dependent files (e.g. `#include "media/mojo/services/android_mojo_util.h"`) responsible for creating their desired user agents.
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
const std::string& GetDetailedUserAgent() {Vikram PasupathyWe already have android specific directory. Minimize buildflag usage in this file and add a new android only functionality in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/android/?q=f:content%20f:browser%20f:media%20f:android
Feras AldahlawiThis function is pretty small and is directly related to the provisioning request which is why I kept it here in the anonymous namespace.
Creations of provision fetcher is already platform dependent. You can add another parameter to `CreateProvisionFetcher` that takes a custom user agent with a reasonable default and store that as member variable.
Then in `Retrieve` call, you can use the custom user agent without buildflags.
That is better because it removes the custom logic from this common implementation and make it the responsibility of a platform dependent files (e.g. `#include "media/mojo/services/android_mojo_util.h"`) responsible for creating their desired user agents.
Please take a look now, I did it so its explicitly specified in Fuchsia.
I would have set the default argument for the user agent string, but binding doesn't take supply the default arg, and also now its more explicit at each call site.
Related details
- Feras Aldahlawi
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. |
Feras Aldahlawi (Gerrit)
Feras Aldahlawi added 4 comments![]( "Open in Gerrit")
std::string user_agent_;make this `const`.
explicit URLProvisionFetcher(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
std::string user_agent);What you can do to have a 'default' user_agent is to leave this ctor unchanged, and have another ctor that takes two parameters. Then in Android, you call the ctor that takes two arguments and pass in a custom `user_agent`.
const std::string& GetDetailedUserAgent() {Vikram PasupathyWe already have android specific directory. Minimize buildflag usage in this file and add a new android only functionality in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/android/?q=f:content%20f:browser%20f:media%20f:android
Feras AldahlawiThis function is pretty small and is directly related to the provisioning request which is why I kept it here in the anonymous namespace.
Vikram PasupathyCreations of provision fetcher is already platform dependent. You can add another parameter to `CreateProvisionFetcher` that takes a custom user agent with a reasonable default and store that as member variable.
Then in `Retrieve` call, you can use the custom user agent without buildflags.
That is better because it removes the custom logic from this common implementation and make it the responsibility of a platform dependent files (e.g. `#include "media/mojo/services/android_mojo_util.h"`) responsible for creating their desired user agents.
Please take a look now, I did it so its explicitly specified in Fuchsia.
I would have set the default argument for the user agent string, but binding doesn't take supply the default arg, and also now its more explicit at each call site.
Much better. Thank you for doing this.
#if BUILDFLAG(IS_ANDROID)
TEST_F(URLProvisionFetcherTest, AndroidUserAgent) {
const std::string expected_user_agent = base::StringPrintf(
"Widevine CDM v1.0 (Linux; U; Android %d; %s; %s; %s; %s Build/%s; %s)",
base::android::android_info::sdk_int(),
base::android::GetDefaultLocaleString().c_str(),
base::android::android_info::manufacturer(),
base::android::android_info::model(),
base::android::android_info::device(),
base::android::android_info::android_build_id(),
base::android::android_info::build_type());
base::RunLoop run_loop;
auto fetcher = std::make_unique<URLProvisionFetcher>(
shared_url_loader_factory_, expected_user_agent);
fetcher->Retrieve(GURL(kTestUrl), kTestRequestBody,
base::BindOnce([](bool, const std::string&) {
}).Then(run_loop.QuitClosure()));
// We need to manually complete the request to trigger the callback.
EXPECT_EQ(test_url_loader_factory_.NumPending(), 1);
EXPECT_EQ(
expected_user_agent,
test_url_loader_factory_.GetPendingRequest(0)->request.headers.GetHeader(
net::HttpRequestHeaders::kUserAgent));
test_url_loader_factory_.SimulateResponseForPendingRequest(
kExpectedRequestUrl, "");
run_loop.Run();
}
#else // BUILDFLAG(IS_ANDROID)
TEST_F(URLProvisionFetcherTest, NonAndroidUserAgent) {
constexpr char kDefaultUserAgent[] = "Widevine CDM v1.0";
base::RunLoop run_loop;
auto fetcher = std::make_unique<URLProvisionFetcher>(
shared_url_loader_factory_, kDefaultUserAgent);
fetcher->Retrieve(GURL(kTestUrl), kTestRequestBody,
base::BindOnce([](bool, const std::string&) {
}).Then(run_loop.QuitClosure()));
// We need to manually complete the request to trigger the callback.
EXPECT_EQ(test_url_loader_factory_.NumPending(), 1);
EXPECT_EQ(
kDefaultUserAgent,
test_url_loader_factory_.GetPendingRequest(0)->request.headers.GetHeader(
net::HttpRequestHeaders::kUserAgent));
test_url_loader_factory_.SimulateResponseForPendingRequest(
kExpectedRequestUrl, "");
run_loop.Run();
}
#endif // BUILDFLAG(IS_ANDROID)
} // namespace contentThis should be one test that looks like this:
```
TEST_F(... ) {
// arrange
base::Runloop run_loop;
...
// act
fetcher->Retrieve(..);
// expectations
#if BUILDFLAG(ANDROID)
EXPECT_EQ(android_user_agent);
#else
EXPEC_EQ(default_user_agent);
#endif
```
Alternatively, you can setup expectations in `//arrange` section. But I prefer to have the branching closer to where it is needed.
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 3 comments![]( "Open in Gerrit")
std::string user_agent_;Vikram Pasupathymake this `const`.
Done
explicit URLProvisionFetcher(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
std::string user_agent);What you can do to have a 'default' user_agent is to leave this ctor unchanged, and have another ctor that takes two parameters. Then in Android, you call the ctor that takes two arguments and pass in a custom `user_agent`.
I don't like how I have to modify content/public/browser/provision_fetcher_factory.h to get this to work, but I agree with you that this is the way to go. Any suggestions to get it better?
#if BUILDFLAG(IS_ANDROID)This should be one test that looks like this:
```
TEST_F(... ) {
// arrange
base::Runloop run_loop;
...// act
fetcher->Retrieve(..);// expectations
#if BUILDFLAG(ANDROID)
EXPECT_EQ(android_user_agent);
#else
EXPEC_EQ(default_user_agent);
#endif
```
Alternatively, you can setup expectations in `//arrange` section. But I prefer to have the branching closer to where it is needed.
- Feras Aldahlawi
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. |
Feras Aldahlawi (Gerrit)
Feras Aldahlawi added 4 comments![]( "Open in Gerrit")
explicit URLProvisionFetcher(`explicit` is only for single argument ctors. You can remove this.
class CONTENT_EXPORT URLProvisionFetcher : public media::ProvisionFetcher {You can remove this. See my comment in unittest.
explicit URLProvisionFetcher(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
std::string user_agent);Vikram PasupathyWhat you can do to have a 'default' user_agent is to leave this ctor unchanged, and have another ctor that takes two parameters. Then in Android, you call the ctor that takes two arguments and pass in a custom `user_agent`.
I don't like how I have to modify content/public/browser/provision_fetcher_factory.h to get this to work, but I agree with you that this is the way to go. Any suggestions to get it better?
I think introducing a new function in `content/public/browser/provision_fetcher_factory.h` is the correct way. Good naming as well.
auto fetcher = std::make_unique<URLProvisionFetcher>(
shared_url_loader_factory_, expected_user_agent);Instead of using `URLProvisionFetcher` ctor, use your newly added factory function `CreateProvisionFetcherWithUserAgent`. That way, you are testing your new factory as well, and can avoid `CONTENT_EXPORT`-ing exporting `URLProvisionFetcher`.
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Vikram Pasupathy
| Commit-Queue | +1 |
3 comments
`explicit` is only for single argument ctors. You can remove this.
Done
class CONTENT_EXPORT URLProvisionFetcher : public media::ProvisionFetcher {You can remove this. See my comment in unittest.
Done
auto fetcher = std::make_unique<URLProvisionFetcher>(
shared_url_loader_factory_, expected_user_agent);Instead of using `URLProvisionFetcher` ctor, use your newly added factory function `CreateProvisionFetcherWithUserAgent`. That way, you are testing your new factory as well, and can avoid `CONTENT_EXPORT`-ing exporting `URLProvisionFetcher`.
- Feras Aldahlawi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Feras Aldahlawi (Gerrit)
Feras Aldahlawi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Feras Aldahlawi
| Code-Review | +1 |
3 comments
std::string user_agent);Change to const string&.
#include "content/common/content_export.h"Remove.
return std::make_unique<URLProvisionFetcher>(std::move(url_loader_factory));Did you want to remove this?
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 3 comments![]( "Open in Gerrit")
std::string user_agent);Vikram PasupathyChange to const string&.
Done
#include "content/common/content_export.h"Vikram PasupathyRemove.
Done
return std::make_unique<URLProvisionFetcher>(std::move(url_loader_factory));Did you want to remove this?
No, didn't mean to remove it. Thanks
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
Related details
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
Could you please add a privacy/net owner to review this change? Thanks!
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 1 comment![]( "Open in Gerrit")
The code itself looks ok, but the bug mentioned getting Security and Privacy owners approval prior to shipping the change. Since this is not behind a flag, can you point me to the Privacy approval?
Related details
- Vikram Pasupathy
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. |
Christian Dullweber (Gerrit)
Christian Dullweber added 1 comment![]( "Open in Gerrit")
The code itself looks ok, but the bug mentioned getting Security and Privacy owners approval prior to shipping the change. Since this is not behind a flag, can you point me to the Privacy approval?
Yes, this needs a proper privacy review. It seems to expose significantly more information than the regular user agent.
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vikram Pasupathy
| Commit-Queue | +1 |
Christian DullweberThe code itself looks ok, but the bug mentioned getting Security and Privacy owners approval prior to shipping the change. Since this is not behind a flag, can you point me to the Privacy approval?
Yes, this needs a proper privacy review. It seems to expose significantly more information than the regular user agent.
- Camille Lamy
- Christian Dullweber
- Feras Aldahlawi
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. |
Christian Dullweber (Gerrit)
Christian Dullweber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
1 comment
Related details
- Camille Lamy
- Feras Aldahlawi
- Vikram Pasupathy
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. |
Xiaohan Wang (Gerrit)
Xiaohan Wang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Xiaohan Wang
| Code-Review | +1 |
1 comment
Bug: 444552657nit: use b:<bug> format for internal bugs
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
nit: use b:<bug> format for internal bugs
Thanks, done.
Related details
- Camille Lamy
- Feras Aldahlawi
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. |
Camille Lamy (Gerrit)
Camille Lamy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
1 comment
Related details
- Feras Aldahlawi
- Vikram Pasupathy
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. |