Moves backup results function to service workers #8493

SergeyZhukovsky · 2021-04-09T20:56:17Z

Resolves brave/brave-browser#15217

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

bridiver · 2021-04-12T15:45:21Z

components/brave_search/renderer/brave_search_sw_holder.h

+      const GURL& script_url);
+
+ private:
+  base::ThreadLocalPointer<std::vector<std::unique_ptr<BraveSearchJSHandler>>>


why is this a thread local?

can we add a comment explaining that WillEvaluateServiceWorkerOnWorkerThread and WillDestroyServiceWorkerContextOnWorkerThread are called from the worker thread so this makes accessing the list thread safe?

bridiver · 2021-04-12T15:48:26Z

renderer/brave_content_renderer_client.cc

+    int64_t service_worker_version_id,
+    const GURL& service_worker_scope,
+    const GURL& script_url) {
+  brave_search::BraveSearchSWHolder::GetInstance()


why isn't this class just owned by BraveContentRendererClient?

changed it to be a member

bridiver · 2021-04-12T15:51:35Z

patches/content-browser-browser_interface_binders.cc.patch

+ void PopulateBinderMap(ServiceWorkerHost* host, mojo::BinderMap* map) {
+   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
+   PopulateServiceWorkerBinders(host, map);
+  PopulateServiceWorkerBindersBrave(host, map);


this is not an extensible patch, it should BRAVE_POPULATE_BINDER_MAP

bridiver · 2021-04-12T16:03:19Z

components/brave_search/renderer/brave_search_sw_holder.h

+
+class BraveSearchJSHandler;
+
+class BraveSearchSWHolder {


this name is a bit confusing to me, maybe BraveSearchServiceWorkerHandler? Using JS is ok because that's used more commonly than Javascript, but SW is not immediately obvious and in general abbreviations should be avoided unless it's to follow existing naming conventions, but in this case the convention is to use ServiceWorker

I will rename it in a follow up with tests

bridiver · 2021-04-12T16:12:12Z

third_party/blink/renderer/includes.gni

@@ -1,29 +1,27 @@
 # common includes which can help minimize patches for
 # src/third_party/blink/renderer/core/BUILD.gn
 brave_blink_renderer_core_visibility = [
-  "//brave/third_party/blink/renderer/*"
+  "//brave/third_party/blink/renderer/*",
+  "//brave/components/brave_search/renderer:renderer",


why is this being added to blink visibility? Is this maybe leftover from previous patching?

redid the mojo and removed it from here

bridiver · 2021-04-12T16:13:43Z

renderer/brave_content_renderer_client.cc

@@ -6,7 +6,7 @@
 #include "brave/renderer/brave_content_renderer_client.h"

 #include "base/feature_list.h"
-#include "brave/components/brave_search/renderer/brave_search_render_frame_observer.h"
+#include "brave/components/brave_search/renderer/brave_search_sw_holder.h"


missing a dep for this in brave_chrome_renderer_public_deps

brave_chrome_renderer_public_deps = [
"//brave/components/brave_search/renderer",

bridiver · 2021-04-12T16:15:25Z

renderer/brave_content_renderer_client.cc

  if (observer)
    observer->RunScriptsAtDocumentStart();

  ChromeContentRendererClient::RunScriptsAtDocumentStart(render_frame);
 }
+
+void BraveContentRendererClient::WillEvaluateServiceWorkerOnWorkerThread(
+    blink::WebServiceWorkerContextProxy* context_proxy,


missing includes for this and GURL which are both forward decls in the header

bridiver · 2021-04-12T17:36:31Z

components/brave_search/renderer/brave_search_js_handler.cc

-    v8::Local<v8::Context> context) {
-  v8::Isolate* isolate = blink::MainThreadIsolate();
+v8::Local<v8::Context> BraveSearchJSHandler::Context() {
+  return v8::Local<v8::Context>::New(isolate_, *context_);


this could potentially be null if it is called before AddJavaScriptObject. You can pass the v8::Context into the BraveSearchJSHandler constructor because you have it in WillEvaluateServiceWorkerOnWorkerThread

bridiver · 2021-04-12T17:38:08Z

components/brave_search/renderer/brave_search_js_handler.cc

    v8::Local<v8::Object> javascript_object,
    const std::string& name,
    const base::RepeatingCallback<Sig>& callback) {
-  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  v8::Local<v8::Context> context = isolate_->GetCurrentContext();


you already have the context in context_

bridiver · 2021-04-12T17:40:27Z

components/brave_search/renderer/brave_search_sw_holder.cc

+    return;
+
+  std::unique_ptr<BraveSearchJSHandler> js_handler(new BraveSearchJSHandler());
+  js_handler->AddJavaScriptObject(v8_context);


just pass the v8_context into the BraveSearchJSHandler as mentioned above

bridiver · 2021-04-12T17:53:49Z

components/brave_search/renderer/brave_search_js_handler.cc

-  v8::HandleScope handle_scope(isolate);
-  v8::Local<v8::Context> context = context_old->Get(isolate);
+  v8::HandleScope handle_scope(isolate_);
+  v8::Local<v8::Context> context = context_->Get(isolate_);
  v8::Context::Scope context_scope(context);


I think you also want v8::MicrotasksScope microtasks(isolate(), v8::MicrotasksScope::kDoNotRunMicrotasks); here.

bridiver · 2021-04-12T20:37:31Z

chromium_src/content/public/browser/content_browser_client.h

 // Brave-specific: allows the embedder to modify the referrer string
 // according to user preferences.
 #define BRAVE_CONTENT_BROWSER_CLIENT_H                                       \
  virtual void MaybeHideReferrer(                                            \
      BrowserContext* browser_context, const GURL& request_url,              \
      const GURL& document_url, blink::mojom::ReferrerPtr* referrer) {}      \
  virtual std::string GetEffectiveUserAgent(BrowserContext* browser_context, \
-                                            const GURL& url);
+                                            const GURL& url);                \
+  virtual void RegisterBrowserInterfaceBindersForHost(                       \


this isn't needed anymore

bridiver · 2021-04-12T20:39:31Z

components/brave_search/renderer/brave_search_js_handler.h

      const std::string& response);

-  content::RenderFrame* render_frame_;
+  blink::ThreadSafeBrowserInterfaceBrokerProxy* broker_;


// not owned

bridiver · 2021-04-12T20:42:06Z

renderer/brave_content_renderer_client.cc

@@ -46,6 +47,8 @@ void BraveContentRendererClient::RenderThreadStarted() {

  brave_observer_ = std::make_unique<BraveRenderThreadObserver>();
  content::RenderThread::Get()->AddObserver(brave_observer_.get());
+  brave_search_sw_holder_.InitBrowserInterfaceBrokerProxy(


I guess I was thinking this would be a unique_ptr and you would initialize it here with browser_interface_broker_, but this is ok too. I would, however, change the name because Init has special meaning for mojo interface so maybe just set_browser_interface_broker?

bridiver · 2021-04-12T20:42:46Z

components/brave_search/renderer/brave_search_sw_holder.cc

+    int64_t service_worker_version_id,
+    const GURL& service_worker_scope,
+    const GURL& script_url) {
+  if (!service_worker_scope.is_valid() ||


we should also DCHECK broker_ here if we're not initializing this object with it

bridiver · 2021-04-12T20:44:58Z

third_party/blink/renderer/includes.gni

@@ -1,29 +1,25 @@
 # common includes which can help minimize patches for


you can probably just drop these changes? Also fine if you leave them I guess, but you don't have to fix the lint errors if you don't touch it :)

Moves backup results function to service workers

SergeyZhukovsky requested review from bridiver, bbondy and pilgrim-brave April 9, 2021 20:56

SergeyZhukovsky requested a review from a team as a code owner April 9, 2021 20:56

SergeyZhukovsky self-assigned this Apr 9, 2021

Moves backup results function to service workers

e257f91

SergeyZhukovsky force-pushed the backup_results_sw branch from 38d4977 to e257f91 Compare April 9, 2021 22:22

Removes content/browser/BUILD.gn patch

8160abd

bridiver reviewed Apr 12, 2021

View reviewed changes

Addresses code review comments

b6b836d

bridiver reviewed Apr 12, 2021

View reviewed changes

Addresses code review comments

85ddac9

bridiver approved these changes Apr 12, 2021

View reviewed changes

SergeyZhukovsky merged commit 5690893 into master Apr 13, 2021

SergeyZhukovsky deleted the backup_results_sw branch April 13, 2021 14:06

SergeyZhukovsky added this to the 1.25.x - Nightly milestone Apr 13, 2021

bbondy pushed a commit that referenced this pull request Apr 16, 2021

Merge pull request #8493 from brave/backup_results_sw

1b31d0f

Moves backup results function to service workers

This was referenced Apr 16, 2021

Backup search API (Uplift to 1.24.x) #8552

Merged

Backup search API (Uplift to 1.23.x Release #2) #8553

Merged

kylehickinson pushed a commit that referenced this pull request Jan 4, 2024

Fix #8493: Privacy Hub hide action approval alert (#8508)

6f6a550

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moves backup results function to service workers #8493

Moves backup results function to service workers #8493

SergeyZhukovsky commented Apr 9, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

SergeyZhukovsky Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

SergeyZhukovsky Apr 12, 2021

bridiver Apr 12, 2021

SergeyZhukovsky Apr 12, 2021

bridiver Apr 12, 2021

SergeyZhukovsky Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

		@@ -1,29 +1,25 @@
		# common includes which can help minimize patches for

Moves backup results function to service workers #8493

Moves backup results function to service workers #8493

Conversation

SergeyZhukovsky commented Apr 9, 2021

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment