Introduce IPFS promo infobar #19588
Introduce IPFS promo infobar #19588
Conversation
cypt4
force-pushed
the
brave_32010
branch
4 times, most recently
from
ae618e8
to
d3b30ea
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| [this](ConfirmInfoBarDelegate::InfoBarButton type, | ||
| void (BraveConfirmInfoBar::*click_function)()) { | ||
| auto* button = AddChildView(std::make_unique<views::MdTextButton>( | ||
| base::BindRepeating(click_function, base::Unretained(this)), |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:
- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/c/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
There was a problem hiding this comment.
This was copied from ConfirmInfobar, i will use weak ptr here
There was a problem hiding this comment.
I think we don't need weak ptr here because button is owned by BraveConfirmInforBar.
Passed |click_function| is valid always till |button| is gone.
app/brave_generated_resources.grd
Outdated
| @@ -876,6 +876,21 @@ Or change later at <ph name="SETTINGS_EXTENIONS_LINK">$2<ex>brave://settings/ext | |||
| <message name="IDS_BRAVE_IPFS_LEARN_MORE"> | |||
| Learn more about IPFS and privacy | |||
| </message> | |||
| <message name="IDS_BRAVE_IPFS_INFOBAR_TEXT" desc="Text that promotes turning on autoredirect to the configured gateway setting"> | |||
| Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave? | |||
There was a problem hiding this comment.
@rmcfadden3 any thoughts on this?
Maybe just move the parents like so:
Would you like to handle content-addressed resources (ipfs:// and ipns://) with native IPFS support built into Brave?
or
Would you like Brave to natively handle content-addressed resources such as ipfs:// and ipns://?
or
Would you like to use native IPFS support built into Brave to handle content-addressed resources (ipfs:// and ipns://)?
There was a problem hiding this comment.
Per Slack convo with @mkarolin …
Let's avoid phrasing like "content-addressed resources" as they may not be fully understood by all users. Instead, we landed on the following:
If a site can be opened with an IPFS link (like ipfs:// or ipns://), would you like to open it with Brave's built-in IPFS support?
There was a problem hiding this comment.
I am afraid these changed sentences bury the lead – this feature is not about enabling ipfs:// but UPGRADING https:// to ipfs:// and ipns:// when possible.
"IPFS resources" on the web could be https:// URLs of public gateways like:
- https://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.w3s.link
- https://cloudflare-ipfs.com/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi
Or DNSLink website like:
Ref. https://docs.ipfs.tech/how-to/address-ipfs-on-web
Brave detects these resources and instead of loading them over HTTP, it uses local IPFS node and presents them as loaded via ipfs:// or ipns:// in the address bar, to indicate native IPFS was used and the hashes were verified.
With that in mind, perhaps below will work better?
| Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave? | |
| Would you like to upgrade IPFS resources like this and load them using the native support (ipfs:// and ipns://) built into Brave? |
Or even shorter:
| Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave? | |
| Would you like to upgrade IPFS resources to native handler (ipfs:// and ipns://) built into Brave? |
There was a problem hiding this comment.
Last version is good, would just like one change:
"opened with an IPFS link" is not ideal, given how IPFS works. just making a link is not enough.
"If a site is available on IPFS" or "on the IPFS network" or something like that would be better.
There was a problem hiding this comment.
Didn't see Lidel's comment before adding this.
He's correct about the overall purpose of the bar.
However, I think upgrade is not correct - it implies a value to the change.
"enable" or something like that would be better.
There was a problem hiding this comment.
Apologies for spending so much time on this text. It's a major conversion point, so it's important to us to get it right 😄
"such" works for en-uk but not for en-us.
How's this attempt to simplify + clarify:
This page is available on the IPFS network. Load IPFS pages using Brave's built-in support via
ipfs://andipns://? [Always][Only This Time][No Thanks]
Changes
-
s/content/page/, as it's more descriptive, aligns w/ web terminology, and "content" is producer/publisher language not end-user language
-
remove parens to be more explicit that the presence of those schemes are what it looks like when the built-in support is being used
-
highlight the protocol schemes
There was a problem hiding this comment.
@autonome — fair points. But I'd suggest we keep the "Would you…" at the start of the sentence to signify this is a question. And the ? after the "ipns://" reads a bit confusing (it appears like it could be part of the scheme). So what if we compromise at:
This page is available on the IPFS network. Would you like to load pages like this using Brave's built-in IPFS support (via ipfs:// or ipns://)? [Always] [Only This Time] [No Thanks]
There was a problem hiding this comment.
If not the above, here's an alt version that's a bit closer to what you suggested:
This page is available on the IPFS network. Would you like to load IPFS pages using Brave's built-in support (via ipfs:// or ipns://)? [Always] [Only This Time] [No Thanks]
There was a problem hiding this comment.
@rmcfadden3 thanks, either of those two wfm!
| BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() {} | ||
|
|
||
| BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() {} |
There was a problem hiding this comment.
nit:
| BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() {} | |
| BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() {} | |
| BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() = default; | |
| BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() = default; |
browser/ipfs/ipfs_tab_helper.cc
Outdated
| } | ||
| } | ||
|
|
||
| ~BraveIPFSInfoBarDelegateObserverImpl() override {} |
There was a problem hiding this comment.
ditto
| ~BraveIPFSInfoBarDelegateObserverImpl() override {} | |
| ~BraveIPFSInfoBarDelegateObserverImpl() override = default; |
| PrefService* local_state) | ||
| : observer_(std::move(observer)), local_state_(local_state) {} | ||
|
|
||
| BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() {} |
There was a problem hiding this comment.
| BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() {} | |
| BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() = default; |
| NOTREACHED(); | ||
| } | ||
| return std::u16string(); |
There was a problem hiding this comment.
| NOTREACHED(); | |
| } | |
| return std::u16string(); | |
| NOTREACHED_NORETURN(); | |
| } |
| class BraveIPFSInfoBarDelegateObserver { | ||
| public: | ||
| BraveIPFSInfoBarDelegateObserver(); | ||
| virtual ~BraveIPFSInfoBarDelegateObserver(); |
There was a problem hiding this comment.
this dtor could be moved to protected section as this'll be destroyed from subclass.
There was a problem hiding this comment.
There is std::uniqie_ptr usage in the BraveIPFSInfoBarDelegate
| // We don't consider case where ok/cancel button and check box exist | ||
| // together. | ||
| DCHECK(!ok_button_ && !cancel_button_); |
There was a problem hiding this comment.
Let's delete above three lines as we could have all together.
| const int button_count = | ||
| (ok_button_ ? 1 : 0) + (cancel_button_ ? 1 : 0) + (extra_button_ ? 1 : 0); |
There was a problem hiding this comment.
nit: how about simply like below?
| const int button_count = | |
| (ok_button_ ? 1 : 0) + (cancel_button_ ? 1 : 0) + (extra_button_ ? 1 : 0); | |
| const int button_count = GetDelegate()->GetButtonsOrder().size(); |
| BraveConfirmInfoBarDelegate* GetBraveDelegate(); | ||
| BraveConfirmInfoBarDelegate* GetDelegate(); | ||
|
|
||
| views::MdTextButton* ok_button_for_testing() { return ok_button_; } |
There was a problem hiding this comment.
This method could be deleted.
| NOTREACHED(); | ||
| return std::u16string(); |
There was a problem hiding this comment.
| NOTREACHED(); | |
| return std::u16string(); | |
| NOTREACHED_NORETURN(); |
|
|
||
| protected: | ||
| // InfoBarView: | ||
| int GetContentMinimumWidth() const override; |
There was a problem hiding this comment.
It seems there is no reason to have this in protected section.
| [this](ConfirmInfoBarDelegate::InfoBarButton type, | ||
| void (BraveConfirmInfoBar::*click_function)()) { | ||
| auto* button = AddChildView(std::make_unique<views::MdTextButton>( | ||
| base::BindRepeating(click_function, base::Unretained(this)), |
There was a problem hiding this comment.
I think we don't need weak ptr here because button is owned by BraveConfirmInforBar.
Passed |click_function| is valid always till |button| is gone.
| if (features::IsChromeRefresh2023()) { | ||
| extra_button_->SetStyle(ui::ButtonStyle::kTonal); | ||
| } |
There was a problem hiding this comment.
Forgot to mention. I think we don't need this as we'll not have different UI for this feature.
|
I validated that the unretained usage was updated to a week pointer so I remove the |
|
Verification passed on
19588.mp4 |
Uplift of #19588 (squashed) to beta
Resolves brave/brave-browser#32010
Privacy review: https://github.com/brave/reviews/issues/1371
New infobar is shown when user enters some page which can be loaded via ipfs:// (dnslink pages, gateway-like pages).
IpfsTabHelper creates infobar on expected conditions via BraveIpfsInfobarDelegate.
BraveConfirmInfobar is forked from ConfirmInfobar to allow reordering of buttons.
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Also need to check obsolete system infobar brave/brave-browser#32129