Get rid of the chrome dependency inside the ipfs component #25333

vadimstruts · 2024-08-26T20:32:34Z

Resolves brave/brave-browser#39624

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
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 wiki
- npm run presubmit wiki, 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

supermassive · 2024-08-27T02:36:08Z

components/ipfs/BUILD.gn

-      "ipfs_component_cleaner.cc",
-      "ipfs_component_cleaner.h",
-    ]
+    sources += [ "ipfs_component_cleaner_delegate.h" ]
    deps += [ "//chrome/common:constants" ]


~~deps += [ "//chrome/common:constants" ]~~
Also chrome line from DEPS file in this folder

supermassive · 2024-08-29T02:11:16Z

browser/ipfs/ipfs_component_cleaner_unittest.cc

+  file << contents;
+  file.close();


base::WriteFile

supermassive · 2024-08-29T02:13:59Z

browser/ipfs/ipfs_component_cleaner.cc

-    return;
-  }
+namespace ipfs {
+void CleanupIpfsComponent() {
  // Remove IPFS component
  base::ThreadPool::PostTask(
      FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
      base::BindOnce(IgnoreResult(&base::DeletePathRecursively),


base::GetDeletePathRecursivelyCallback

supermassive · 2024-08-29T02:16:04Z

components/ipfs/DEPS

@@ -1,4 +1,3 @@
 include_rules = [
-  "+chrome/common",
  "+content/prefs",


Do we still need content/prefs here?

supermassive · 2024-08-29T09:57:12Z

test/BUILD.gn

+  if (!is_android && !is_ios) {
+    sources += [ "//brave/browser/ipfs/ipfs_component_cleaner_unittest.cc" ]
+  }
+


Don't need !is_ios check. It's already asserted above

supermassive · 2024-08-29T09:57:55Z

components/ipfs/ipfs_utils_unittest.cc

 #include "net/base/url_util.h"
 #include "testing/gtest/include/gtest/gtest.h"



Whole includes section needs some cleanup

supermassive · 2024-08-29T09:58:38Z

components/ipfs/ipfs_prefs.cc

@@ -9,10 +9,6 @@
 #include "build/build_config.h"


~~#include "base/files/file_path.h"~~

supermassive · 2024-08-29T09:59:13Z

chromium_src/chrome/browser/prefs/browser_prefs.cc

+#include <memory>
+#include <utility>
+


Don't need this

supermassive · 2024-08-29T10:00:58Z

browser/brave_browser_main_parts.cc

+#if !BUILDFLAG(IS_IOS) && !BUILDFLAG(IS_ANDROID)
+  ipfs::CleanupIpfsComponent();
+#endif
 }


Don't need IOS checks in this file

supermassive · 2024-08-29T10:03:02Z

browser/ipfs/sources.gni

+brave_browser_ipfs_sources = []
+brave_browser_ipfs_deps = []
+
+if (!is_android && !is_ios) {


~~&& !is_ios~~

supermassive · 2024-08-29T10:04:09Z

browser/ipfs/sources.gni

+import("//build/config/features.gni")
+
+brave_browser_ipfs_sources = []
+brave_browser_ipfs_deps = []


strictly speaking needs some deps based on #includes from ipfs_component_cleaner.cc/h

supermassive

lgtm with some suggestions

iefremov · 2024-09-02T13:39:04Z

test/BUILD.gn

@@ -146,6 +146,11 @@ test("brave_unit_tests") {
    "//components/sync/service/sync_auth_manager_unittest.cc",
    "//components/sync_device_info/device_info_sync_bridge_unittest.cc",
  ]
+
+  if (!is_android) {


pls add a source set to browser/ipfs/BUILD.gn instead

I just removed that line

iefremov · 2024-09-02T16:07:33Z

browser/brave_browser_main_parts.cc

@@ -161,6 +162,10 @@ void BraveBrowserMainParts::PostBrowserStart() {
    }
  }
 #endif  // !BUILDFLAG(IS_ANDROID)
+
+#if !BUILDFLAG(IS_ANDROID)
+  ipfs::CleanupIpfsComponent();


AFAIU moving the code from components to chrome was done only becase of chrome::USER_DATA. If that's correct, I'd suggest to move only this call (and pass the FilePath with user data dir into it) and keep the component cleaner in components, it just looks more logical to me.

iefremov

lgtm once the unit test target is put into separate BUILD.gn
pls consider keeping the cleaner in components instead of moving it b/browser/ipfs entirely

Signed-off-by: Vadym Struts <vstruts@brave.com>

supermassive · 2024-09-05T10:13:26Z

test/BUILD.gn

@@ -146,6 +146,7 @@ test("brave_unit_tests") {
    "//components/sync/service/sync_auth_manager_unittest.cc",
    "//components/sync_device_info/device_info_sync_bridge_unittest.cc",
  ]
+


nit: unnecessary change

Signed-off-by: Vadym Struts <vstruts@brave.com>

bridiver · 2024-10-07T23:38:19Z

components/ipfs/test/BUILD.gn

@@ -11,6 +11,13 @@ source_set("brave_ipfs_unit_tests") {

  sources = [ "//brave/components/ipfs/ipfs_utils_unittest.cc" ]

+  if (!is_android && !is_ios) {
+    sources += [
+      "//brave/components/ipfs/ipfs_component_cleaner.cc",


why are you adding a source here instead of a dep on the target for that source? This should also have a new buildflag for enable_ipfs_component or similar. os buildflags should only be used for os-specific code, not for things that just aren't enabled on all platforms

Created follow up issue for that: brave/brave-browser#41626

bridiver · 2024-10-07T23:50:46Z

browser/brave_browser_main_parts.cc

@@ -67,6 +67,12 @@
 #include "extensions/browser/extension_system.h"
 #endif

+#if !BUILDFLAG(IS_ANDROID)
+#include "base/path_service.h"


buildflag guard in gn should match buildflag guards in cpp. It doesn't matter that this is only used inside !IS_ANDROID, the headers themselves are not guarded by !is_android in gn. Also there is already an existing !IS_ANDROID guard in this file.

Created follow up issue for that: brave/brave-browser#41626

github-actions bot assigned vadimstruts Aug 26, 2024

supermassive reviewed Aug 27, 2024

View reviewed changes

vadimstruts marked this pull request as ready for review August 27, 2024 18:01

vadimstruts requested review from a team as code owners August 27, 2024 18:01

supermassive reviewed Aug 29, 2024

View reviewed changes

vadimstruts requested a review from a team as a code owner August 29, 2024 09:54

supermassive reviewed Aug 29, 2024

View reviewed changes

supermassive approved these changes Aug 29, 2024

View reviewed changes

vadimstruts force-pushed the 39624-get-rid-of-the-chrome-dependency-inside-the-ipfs-component branch from 349a267 to 6705afd Compare August 29, 2024 11:13

iefremov reviewed Sep 2, 2024

View reviewed changes

iefremov approved these changes Sep 2, 2024

View reviewed changes

vadimstruts force-pushed the 39624-get-rid-of-the-chrome-dependency-inside-the-ipfs-component branch 2 times, most recently from b9343c1 to 12defaa Compare September 3, 2024 21:00

vadimstruts added 7 commits September 5, 2024 12:09

moved platform dependency from the component

8ff1216

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed according review suggestions

b273852

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed deps file

3edd20e

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed according review suggestions

875a7a8

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed according review suggestions

5021f3d

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed according review suggestions

bdbb84e

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed according review suggestions

15bc75c

Signed-off-by: Vadym Struts <vstruts@brave.com>

vadimstruts added 2 commits September 5, 2024 12:09

fixed according review suggestions

da21ed0

Signed-off-by: Vadym Struts <vstruts@brave.com>

added ipfs common header

61ed0ff

Signed-off-by: Vadym Struts <vstruts@brave.com>

vadimstruts force-pushed the 39624-get-rid-of-the-chrome-dependency-inside-the-ipfs-component branch from f0edb95 to 61ed0ff Compare September 5, 2024 10:10

supermassive reviewed Sep 5, 2024

View reviewed changes

vadimstruts added 4 commits September 5, 2024 12:28

added ipfs common header

fb950bf

Signed-off-by: Vadym Struts <vstruts@brave.com>

added ipfs common header

b807c83

Signed-off-by: Vadym Struts <vstruts@brave.com>

remove unneded change

c306cdc

Signed-off-by: Vadym Struts <vstruts@brave.com>

fix format

e199b10

Signed-off-by: Vadym Struts <vstruts@brave.com>

vadimstruts merged commit 1148af4 into master Sep 5, 2024
17 checks passed

vadimstruts deleted the 39624-get-rid-of-the-chrome-dependency-inside-the-ipfs-component branch September 5, 2024 15:26

github-actions bot added this to the 1.71.x - Nightly milestone Sep 5, 2024

bridiver reviewed Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of the chrome dependency inside the ipfs component #25333

Get rid of the chrome dependency inside the ipfs component #25333

vadimstruts commented Aug 26, 2024 •

edited

Loading

supermassive Aug 27, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024

supermassive Aug 29, 2024 •

edited

Loading

supermassive left a comment

iefremov Sep 2, 2024

vadimstruts Sep 3, 2024

iefremov Sep 2, 2024

vadimstruts Sep 3, 2024

iefremov left a comment

supermassive Sep 5, 2024

bridiver Oct 7, 2024 •

edited

Loading

vadimstruts Oct 15, 2024

bridiver Oct 7, 2024

vadimstruts Oct 15, 2024

		#include "net/base/url_util.h"
		#include "testing/gtest/include/gtest/gtest.h"

Get rid of the chrome dependency inside the ipfs component #25333

Get rid of the chrome dependency inside the ipfs component #25333

Conversation

vadimstruts commented Aug 26, 2024 • edited Loading

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

supermassive Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

supermassive left a 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

iefremov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Oct 7, 2024 • edited Loading

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

vadimstruts commented Aug 26, 2024 •

edited

Loading

supermassive Aug 29, 2024 •

edited

Loading

bridiver Oct 7, 2024 •

edited

Loading