Built-in language detection using CLD3 [chromium/src : main]
Pooja Patel (Gerrit)
Message from Pooja Patel![]( "Open in Gerrit")
Set Ready For Review
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: dch...@chromium.org, na...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): dch...@chromium.org, na...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
dch...@chromium.org, na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Cheng
- Nasko Oskov
- Stephen White
- Sushanth Rajasankar
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
The newly added services/language_detection directory should have OWNERS file to indicate who are the people that are familiar with the directory and can review changes to it.
blink::features::kBuiltInLanguageDetectionAPI)) {Do we need to support this interface across all worker types? Are those APIs available and usable in those contexts? If I read the explainer correctly, those are not supported.
Related details
- Daniel Cheng
- Pooja Patel
- Stephen White
- Sushanth Rajasankar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sushanth Rajasankar (Gerrit)
Sushanth Rajasankar added 1 comment![]( "Open in Gerrit")
Adding Adolf to review from the Microsoft side.
Related details
- Adolf Daniel
- Daniel Cheng
- Pooja Patel
- Stephen White
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adolf Daniel (Gerrit)
Adolf Daniel added 8 comments![]( "Open in Gerrit")
Please find my review comments.
if (!g_service_provider_initialized) {qq: do we need a global variable? I do see some usage here, just curious.
std::move(built_in_detector));qq: can't we just use `std::make_unique<language_detection::BuiltInLanguageDetectorImpl>()` here?
std::move(callback).Run("und", 1.0f);should this be `0.0f`?
std::move(callback).Run("und", 0.5f);should this be 0.0f?
chrome_lang_id::NNetLanguageIdentifier language_identifier(Could this be done in the constructor so that we don't need to create this for every call?
/*min_num_bytes=*/0, /*max_num_bytes=*/1000);Can we use a constant or feature param driven max/min bytes?
base::BindOnce(&ServiceClient::OnDisconnect, base::Unretained(this)));use `weak_ptr_factory_.GetWeakPtr()`
Related details
Pooja Patel (Gerrit)
Pooja Patel added 7 comments![]( "Open in Gerrit")
qq: do we need a global variable? I do see some usage here, just curious.
Done
qq: can't we just use `std::make_unique<language_detection::BuiltInLanguageDetectorImpl>()` here?
Done
should this be `0.0f`?
Done
std::move(callback).Run("und", 0.5f);Pooja Patelshould this be 0.0f?
Done
chrome_lang_id::NNetLanguageIdentifier language_identifier(Could this be done in the constructor so that we don't need to create this for every call?
Done
Can we use a constant or feature param driven max/min bytes?
I add contants for these
base::BindOnce(&ServiceClient::OnDisconnect, base::Unretained(this)));Pooja Pateluse `weak_ptr_factory_.GetWeakPtr()`
Done
Related details
- Adolf Daniel
- Daniel Cheng
- Stephen White
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pooja Patel (Gerrit)
Pooja Patel added 1 comment![]( "Open in Gerrit")
Do we need to support this interface across all worker types? Are those APIs available and usable in those contexts? If I read the explainer correctly, those are not supported.
yes thanks for pointing this out, let's follow the explainer
Related details
- Adolf Daniel
- Anthony Cui
- Daniel Cheng
- Ian Wells
- Nasko Oskov
- Stephen White
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 5 comments![]( "Open in Gerrit")
Can you please add a tracking bug for this CL?
// Use the new DetectLanguages method that returns multiple resultsnit: If you are using full sentences, they should end with a period.
content::BrowserContext* browser_context,This parameter doesn't seem to be used. Why are we passing it in?
Alternatively, this code is likely to have a cross-profile leak issues as it is not taking that into account. This looks like a security issue to me and should be addressed before we can land this code.
// uses the built-in CLD3 model shipped with the browser.nit: This is a bit too specific for the //content/ module, as the embedder of //content/ can be many things, including not a full browser. You can replace the string with just "model" and I think it will be sufficient.
"+services/language_detection",This strikes me as a bit odd. The language detector is delegated to the //content/ embedder to execute, yet //content/ adds dependency on the service. Why is that?
I would expect you can register the service in the //chrome/ layer as //content calls out to the embedder for additional services being registered through https://source.chromium.org/chromium/chromium/src/+/main:content/public/utility/content_utility_client.h;drc=181e7a80687c55d02e2e74d7d5db3d75ad53c0de;l=49.
Related details
- Adolf Daniel
- Anthony Cui
- Daniel Cheng
- Ian Wells
- Pooja Patel
- Stephen White
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |