-
Notifications
You must be signed in to change notification settings - Fork 893
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
Leo AI becomes an installable PWA #27025
Conversation
ceb70ba
to
6e90cd6
Compare
A Storybook has been deployed to preview UI for the latest push |
@@ -0,0 +1,42 @@ | |||
// Copyright (c) 2024 The Brave Authors. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it would be better to have this browser test in our test target because we don't run upstream browser test on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to brave/browser/banners/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, I forgot to actually delete this file
scoped_feature_list_.InitWithFeatures( | ||
/*enabled_features=*/{}, GetDisabledFeatures()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it's not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed
{ | ||
base::RunLoop run_loop; | ||
manager->PrepareDone(run_loop.QuitClosure()); | ||
|
||
ASSERT_TRUE(ui_test_utils::NavigateToURL( | ||
browser(), GURL(base::StrCat({"chrome://", kAIChatUIHost})))); | ||
run_loop.Run(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be simplifed.
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), GURL(base::StrCat({"chrome://", kAIChatUIHost}))));
manager->WaitForInstallableCheck();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why the chromium browser test to check the same thing for chrome://password-manager doesn't do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But great spot - this works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string reviewers ++
6e90cd6
to
879ea14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
# License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
# You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
if (!is_android) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we don't need this check as this browser_tests
target is already filtered in test/BUILD.gn
?
879ea14
to
941c22a
Compare
Windows CI failed with unrelated flakey test:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with few things to improve
@@ -0,0 +1,3 @@ | |||
include_rules = [ | |||
"+brave/components/constants", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this dependency should also appear in components/webapps/browser/BUILD.gn
bool IsValidWebAppUrl(const GURL& app_url) { | ||
return IsValidWebAppUrl_ChromiumImpl(app_url) || | ||
(app_url.SchemeIs(content::kChromeUIScheme) && | ||
base::Contains(kInstallablePWAWebUIHosts, app_url.host())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host_piece()
// Add some extra items to WebUI hosts considered valid for PWAs | ||
#define kChromeUIPasswordManagerHost \ | ||
kChromeUIPasswordManagerHost && \ | ||
!base::Contains(kInstallablePWAWebUIHosts, url.host()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host_piece()
941c22a
to
7b673e7
Compare
Released in v1.75.105 |
Issues:
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: