Display IPFS interstitial page when failed to use local node #6782

yrliou · 2020-10-06T04:05:05Z

Resolves brave/brave-browser#11558

Submitter Checklist:

Test Plan:

Make sure IPFS feature flag is enabled.
Go to brave://settings and search for ipfs, make sure local node is selected and check the new switch should be disabled at the first place.

Turn off your network and visit ipfs://QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR
An interstitial page should be shown

Go to brave://ipfs, verified that daemon was launched with 0 connected peers.

Turn your network back on and go click the proceed button in the interstitial page.
The page should be redirected to public gateway

Go the brave://settings and search for ipfs, the new gateway fallback switch should now be enabled.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

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.

…ts/ipfs/common

yrliou · 2020-10-07T06:11:21Z

IpfsNavigationThrottoleBrowserTest.ShowInterstitialForEmptyConnectedPeers currently times out on CI builds (Release builds) but not my local component builds, currently investigating at the moment.
Update: This is now fixed.

iefremov · 2020-10-07T15:02:38Z

components/ipfs/browser/BUILD.gn

    "ipfs_service.cc",
    "ipfs_service.h",
    "ipfs_service_observer.h",
-    "translate_ipfs_uri.cc",
-    "translate_ipfs_uri.h",


basically there is no need to split a component into browser, common, etc unless all code lives in the browser process. This is needed only when there is something needed for renderer and something for browesr

@bbondy has a different PR in progress for moving all files into browser folder, I'll leave it as is here, I move this one to common in this PR because I'm using a function in translate_ipfs_uri.cc in ipfs_utils which is currently under common folder.

yeah, np, just a side note. We basically can just keep everything in components/ipfs

iefremov · 2020-10-07T15:19:02Z

will take another look tomorrow

…de failed

iefremov · 2020-10-08T08:41:24Z

components/ipfs/common/ipfs_utils.h

@@ -13,6 +13,10 @@ namespace ipfs {
 class IpfsUtils {
 public:
  static bool IsIPFSURL(const GURL& url);
+  static bool IsDefaultGatewayURL(const GURL& url);


all of these should be free functions, no need to put them into a class

Thanks for pointing it out, I'll open a small followup for this to minimize the scope here.

Follow-up PR at #6821.

iefremov

LGTM!

iefremov · 2020-10-08T09:19:41Z

components/ipfs/browser/ipfs_not_connected_page.h

+
+namespace ipfs {
+
+class IPFSNotConnectedPage


can you please briefly describe what this page is for?

Fixed in cb284f6

bbondy

lgtm, feel free to do the comments I mentioned in a follow up if you prefer too.

bbondy · 2020-10-06T18:22:34Z

app/brave_generated_resources.grd

+        IPFS public gateway fallback
+      </message>
+      <message name="IDS_SETTINGS_IPFS_AUTO_FALLBACK_TO_GATEWAY_DESC" desc="The description for the switch of automatically fallback to IPFS public gateway">
+        Automatically fallback to IPFS public gateway when failed to access through the local node.


Automatically fallback to the IPFS public gateway when the local node cannot be accessed.

Fixed in cb284f6

bbondy · 2020-10-08T17:54:35Z

components/ipfs/browser/ipfs_navigation_throttle.cc

+    return;
+
+  const GURL url = IpfsUtils::ToPublicGatewayURL(
+      navigation_handle()->GetURL());


nit: I think this can fit on 1 line
clang-format -i components/ipfs/browser/ipfs_navigation_throttle.cc

Fixed in cb284f6

bbondy · 2020-10-08T17:56:54Z

components/ipfs/browser/ipfs_navigation_throttle_unittest.cc

@@ -115,6 +115,12 @@ TEST_F(IpfsNavigationThrottleUnitTest, DeferUntilIpfsProcessLaunched) {
  profile()->GetPrefs()->SetInteger(
      kIPFSResolveMethod, static_cast<int>(IPFSResolveMethodTypes::IPFS_LOCAL));

+  auto peers = std::vector<std::string>{


nit: minor changes to this file if you run clang-format

Fixed in cb284f6

bbondy · 2020-10-08T17:59:35Z

components/resources/ipfs_strings.grdp

+      Hide Advanced
+    </message>
+    <message name="IDS_IPFS_NOT_CONNECTED_EXPLANATION" desc="The text of the IPFS not connected interstitial that will be displayed when the user presses the 'Advanced' button for additional information.">
+      We cannot connect to the requested content using the local IPFS node at the moment because the local node doesn't seem to start or there are no connected peers available at the moment. By proceeding we will automatically fallback to the default public gateway in the future when we cannot connect to IPFS contents using the local IPFS node, this could be turned off later in settings.


We cannot connect to the requested content using the local IPFS node because the local node didn't start or because there are no connected peers available at the moment. By proceeding, we will automatically fallback to the default public gateway in the future when we cannot connect to IPFS content using the local IPFS node. This can be turned off later in settings.

Fixed in cb284f6

bbondy · 2020-10-08T18:01:41Z

components/ipfs/common/ipfs_utils.cc

+  DCHECK(IsIPFSScheme(url) || IsIPFSURL(url));
+  GURL new_url;
+
+  // For ipfs/ipfs schemes, use TranslateIPFSURI directly.


nit: ipfs/ipns I assume

Yes indeed! Fixed in cb284f6.

bbondy · 2020-10-08T18:05:07Z

components/ipfs/common/BUILD.gn

  ]

  deps = [
    "//extensions/common",
+    "//net",


I'm not sure if this dep can cause problems being included in common code. I guess we aren't actually depending on this common target from common code though so I think it's ok. And will be cleaned up when I get rid of browser and common.

- Add a comment for IPFSNotConnectedPage. - Strings changes. - Fix typo in a comment. - clang-format

yrliou · 2020-10-08T18:45:34Z

chromium_src/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

+    {"ipfsAutoFallbackToGatewayLabel",
+      IDS_SETTINGS_IPFS_AUTO_FALLBACK_TO_GATEWAY_LABEL},
+    {"ipfsAutoFallbackToGatewayDesc",
+      IDS_SETTINGS_IPFS_AUTO_FALLBACK_TO_GATEWAY_DESC},


cc @bridiver here for chromium_src owner review.

No need for @bridiver to review this particular chromium_src change. The codeowner file has some false positives in some cases like this which doesn't need his review.

yrliou self-assigned this Oct 6, 2020

yrliou force-pushed the ipfs_interstitial_page branch 2 times, most recently from 78e08e5 to 899b80a Compare October 6, 2020 23:40

yrliou marked this pull request as ready for review October 6, 2020 23:52

yrliou requested review from bridiver and iefremov as code owners October 6, 2020 23:52

yrliou requested a review from bbondy October 6, 2020 23:53

yrliou changed the title ~~[WIP] Display IPFS interstitial page when failed to use local node~~ Display IPFS interstitial page when failed to use local node Oct 6, 2020

yrliou force-pushed the ipfs_interstitial_page branch from 899b80a to ff9933c Compare October 7, 2020 00:00

yrliou added this to the 1.17.x - Nightly milestone Oct 7, 2020

yrliou force-pushed the ipfs_interstitial_page branch from ff9933c to 142dfce Compare October 7, 2020 04:37

Move translate_ipfs_uri.cc/h from components/ipfs/browser to componen…

10c3c72

…ts/ipfs/common

yrliou force-pushed the ipfs_interstitial_page branch from 142dfce to 939b8f0 Compare October 7, 2020 04:48

iefremov reviewed Oct 7, 2020

View reviewed changes

yrliou added the CI/skip Do not run CI builds (except noplatform) label Oct 7, 2020

yrliou force-pushed the ipfs_interstitial_page branch 2 times, most recently from c34b1b5 to 7f242c3 Compare October 7, 2020 17:51

yrliou removed the CI/skip Do not run CI builds (except noplatform) label Oct 8, 2020

yrliou added 3 commits October 7, 2020 19:31

Show interstitial page to offer public gateway fallback when local no…

b9431fb

…de failed

Add a settings switch for IPFS public gateway fallback

6849210

Add tests for IPFS interstitial page

3157a2e

yrliou force-pushed the ipfs_interstitial_page branch from 7f242c3 to 3157a2e Compare October 8, 2020 02:31

iefremov reviewed Oct 8, 2020

View reviewed changes

iefremov approved these changes Oct 8, 2020

View reviewed changes

iefremov reviewed Oct 8, 2020

View reviewed changes

bbondy approved these changes Oct 8, 2020

View reviewed changes

Address review comments

cb284f6

- Add a comment for IPFSNotConnectedPage. - Strings changes. - Fix typo in a comment. - clang-format

yrliou commented Oct 8, 2020

View reviewed changes

bbondy approved these changes Oct 8, 2020

View reviewed changes

bbondy merged commit 2badd76 into master Oct 8, 2020

bbondy deleted the ipfs_interstitial_page branch October 8, 2020 19:13

yrliou mentioned this pull request Oct 8, 2020

Remove redundant IpfsUtils class brave/brave-browser#12059

Closed

yrliou mentioned this pull request Jan 11, 2021

Create an interstitial page for when there are no IPFS peers brave/brave-browser#11558

Closed

bbondy mentioned this pull request Jan 11, 2021

Native IPFS support in Brave brave/brave-browser#10220

Closed

LaurenWags mentioned this pull request Jan 13, 2021

buttons on interstitial page do not work brave/brave-browser#13544

Closed

mkarolin mentioned this pull request Oct 30, 2023

Upgrade from Chromium 119 to Chromium 120. brave/brave-browser#33471

Closed

btlechowski mentioned this pull request Dec 4, 2023

Upgrade from Chromium 120.0.6099.35 to Chromium 120.0.6099.56. brave/brave-browser#34648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display IPFS interstitial page when failed to use local node #6782

Display IPFS interstitial page when failed to use local node #6782

yrliou commented Oct 6, 2020 •

edited

Loading

yrliou commented Oct 7, 2020 •

edited

Loading

iefremov Oct 7, 2020 •

edited

Loading

yrliou Oct 7, 2020

iefremov Oct 8, 2020

iefremov commented Oct 7, 2020

iefremov Oct 8, 2020

yrliou Oct 8, 2020

yrliou Oct 8, 2020

iefremov left a comment

iefremov Oct 8, 2020

yrliou Oct 8, 2020

bbondy left a comment

bbondy Oct 6, 2020

yrliou Oct 8, 2020

bbondy Oct 8, 2020

yrliou Oct 8, 2020

bbondy Oct 8, 2020

yrliou Oct 8, 2020

bbondy Oct 8, 2020

yrliou Oct 8, 2020

bbondy Oct 8, 2020

yrliou Oct 8, 2020

bbondy Oct 8, 2020

yrliou Oct 8, 2020 •

edited

Loading

bbondy Oct 8, 2020

Display IPFS interstitial page when failed to use local node #6782

Display IPFS interstitial page when failed to use local node #6782

Conversation

yrliou commented Oct 6, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

yrliou commented Oct 7, 2020 • edited Loading

iefremov Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Oct 7, 2020

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

Choose a reason for hiding this comment

bbondy 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

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

yrliou Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou commented Oct 6, 2020 •

edited

Loading

yrliou commented Oct 7, 2020 •

edited

Loading

iefremov Oct 7, 2020 •

edited

Loading

yrliou Oct 8, 2020 •

edited

Loading