Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Create module bridge for Telemetry API Client #4

Draft
wants to merge 12 commits into
base: tt135/develop
Choose a base branch
from

Conversation

benedict-lim-ahrefs
Copy link
Collaborator

@benedict-lim-ahrefs benedict-lim-ahrefs commented Apr 2, 2025

Background

Integrate telemetry API interface in Java for the Android Client

Details

Research notes

04-07 16:03:35.244  6227  6227 I chromium: [INFO:cs_api_client_service.cc(49)] |>> Sending url : http://www.ahrefs.com/
04-07 16:03:35.245  6227  6227 I chromium: [INFO:cs_api_client_service.cc(62)] |>> Sending payload : {"k":"RrZUA7XvGI6R2DvyqbnE0w","n":"pageview","u":"http://www.ahrefs.com/","v":"C5D39C1ED3314D180C8716B56C84B359"}
04-07 16:03:35.245  6227  6227 I chromium: [INFO:web_request_helper.cc(242)] POST https://analytics.ahrefs.com/api/event
04-07 16:03:35.323  6227  6227 I chromium: [INFO:web_request_helper.cc(375)] [[OnResponse]] Response received
04-07 16:03:35.324  6227  6227 I chromium: [INFO:web_request_helper.cc(337)] Creating DataDecoder for WebRequestHelper
04-07 16:03:35.324  6227  6227 I chromium: [INFO:cs_api_client_module_bridge.cc(134)]  |>> CS post response code : 200
04-07 16:03:35.324  6227  6227 I chromium: [INFO:cs_api_client_module_bridge.cc(136)]  |>> CS post error code : 0

@benedict-lim-ahrefs benedict-lim-ahrefs self-assigned this Apr 2, 2025
@benedict-lim-ahrefs benedict-lim-ahrefs marked this pull request as draft April 2, 2025 06:03
Comment on lines +56 to +57
std::string profile_id = profile_->UniqueId();
dict.Set("v", profile_id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly copy paste from cs_handler.cc but set the unique ID from Profile derived from BrowserContext.


using web_request_helper::WebRequestResult;

class CSApiClientService : public KeyedService {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has to be a KeyedService so that it can get BrowserContext required to retrieve the unique ID. So, I cannot reuse the existing cs_api_client.cc.

return instance.get();
}

CSApiClientServiceFactory::CSApiClientServiceFactory()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the example of start_suggest_service_factory.cc.

void CSApiClientModuleBridge::Handle(
JNIEnv* env,
const JavaParamRef<jobject>& obj,
const JavaParamRef<jstring>& url
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly copy paste from cs_handler.cc but accept Java String as parameter. C++ GURL and Java GURL don't seem to be interoperable.

Comment on lines +21 to +23
generate_jni("jni_headers") {
sources = [ "java/src/org/chromium/chrome/browser/cs/CSApiClientModuleBridge.java" ]
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autogenerated at out/Default/gen/jni_headers/chrome/browser/cs/android/CSApiClientModuleBridge_jni.h.

@@ -275,6 +275,7 @@
#include "chrome/browser/android/webapk/webapk_sync_service_factory.h"
#include "chrome/browser/auxiliary_search/auxiliary_search_provider.h"
#include "chrome/browser/commerce/merchant_viewer/merchant_viewer_data_manager_factory.h"
#include "chrome/browser/cs/android/cs_api_client_service_factory.h"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to get Service class instance from Factory within Profile a.k.a BrowserContext, this has to be included.

Comment on lines +842 to +844
CSApiClientModuleBridge csApiClientModuleBridge =
new CSApiClientModuleBridge(tab.getProfile());
csApiClientModuleBridge.handle(url.getSpec());
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the place for listening to every URL load. There are other specific areas which invokes loadUrl and onLoadUrl etc. but are not universally triggered.

@benedict-lim-ahrefs benedict-lim-ahrefs marked this pull request as ready for review April 9, 2025 02:59
@benedict-lim-ahrefs
Copy link
Collaborator Author

@nyinyithann

Although Chromium Android will be frozen for awhile, I think this PR is safe to merge because it does not affect the web client build, except perhaps the initial Ninja build taking some time to compile the Android files.

I am just afraid that there might be conflicts to resolve if I leave this PR unmerged for weeks and months. Please take a look whenever you have the time!

@nyinyithann
Copy link
Collaborator

nyinyithann commented Apr 9, 2025

Hey @benedict-lim-ahrefs,

Given that the Chromium Android project is currently on hold, I would recommend leaving the PR open instead of merging it into tt135/develop. This is because tt135/develop is based on the canary branch, and I need to continue merging with upstream branches until release time, at which point I will select a stable branch to merge into. This means I have to maintain the Android code and resolve merge conflicts each time I sync with upstream.
Once the Android project is resumed, I will be happy to assist in merging the PR with whichever upstream branch we are using at that time.

@nyinyithann
Copy link
Collaborator

Or you create your own branch out of tt135/develop and name it probably tt135/android/develop and merge the PR to it for the record. But I would not review the code at this time.

@benedict-lim-ahrefs
Copy link
Collaborator Author

This is because tt135/develop is based on the canary branch, and I need to continue merging with upstream branches until release time, at which point I will select a stable branch to merge into. This means I have to maintain the Android code and resolve merge conflicts each time I sync with upstream.

I see... That is a valid concern. In that case, I am fine to keep the PR open until further notice, and I will resolve the conflicts with the upstream branch when I return to the project.

Since I will not develop more features for Android, I will not create an tt135/android/develop intermediate branch.

@benedict-lim-ahrefs benedict-lim-ahrefs marked this pull request as draft April 9, 2025 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants