Skip to content
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.

Sign up for GitHub

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

[AI Chat]: Cleanup SiteInfo - now the presence/absence of the struct indicates whether content association is possible #27243

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+194 −220
Open

[AI Chat]: Cleanup SiteInfo - now the presence/absence of the struct indicates whether content association is possible #27243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad97372
[AI Chat]: More natural API
fallaciousreasoning Jan 14, 2025
a2ede93
Maybe fix ios build?
fallaciousreasoning Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,9 @@ void AIChatUIPageHandler::BindParentUIFrameFromChildFrame(

void AIChatUIPageHandler::GetFaviconImageDataForAssociatedContent(
GetFaviconImageDataCallback callback,
mojom::SiteInfoPtr content_info,
mojom::AssociatedContentPtr content_info,
bool should_send_page_contents) {
if (!content_info->is_content_association_possible ||
!content_info->url.has_value() || !content_info->url->is_valid()) {
if (!content_info || !content_info->url.is_valid()) {
std::move(callback).Run(std::nullopt);
return;
}
Expand All @@ -298,7 +297,7 @@ void AIChatUIPageHandler::GetFaviconImageDataForAssociatedContent(
};

favicon_service_->GetRawFaviconForPageURL(
content_info->url.value(), icon_types, kDesiredFaviconSizePixels, true,
content_info->url, icon_types, kDesiredFaviconSizePixels, true,
base::BindOnce(on_favicon_available, std::move(callback)),
&favicon_task_tracker_);
}
Expand Down
2 changes: 1 addition & 1 deletion browser/ui/webui/ai_chat/ai_chat_ui_page_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class AIChatUIPageHandler : public mojom::AIChatUIHandler,

void GetFaviconImageDataForAssociatedContent(
GetFaviconImageDataCallback callback,
mojom::SiteInfoPtr content_info,
mojom::AssociatedContentPtr content_info,
bool should_send_page_contents);

raw_ptr<AIChatTabHelper> active_chat_tab_helper_ = nullptr;
Expand Down
34 changes: 13 additions & 21 deletions components/ai_chat/core/browser/ai_chat_database.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,12 @@ std::vector<mojom::ConversationPtr> AIChatDatabase::GetAllConversations() {
conversation->updated_time = statement.ColumnTime(index++);
conversation->has_content = true;

conversation->associated_content = mojom::SiteInfo::New();

if (statement.GetColumnType(index) != sql::ColumnType::kNull) {
DVLOG(1) << __func__ << " got associated content";

conversation->associated_content = mojom::AssociatedContent::New();
conversation->associated_content->uuid = statement.ColumnString(index++);
conversation->associated_content->title =
DecryptOptionalColumnToString(statement, index++);
DecryptOptionalColumnToString(statement, index++).value_or("");
auto url_raw = DecryptOptionalColumnToString(statement, index++);
if (url_raw.has_value()) {
conversation->associated_content->url = GURL(url_raw.value());
Expand All @@ -180,9 +178,6 @@ std::vector<mojom::ConversationPtr> AIChatDatabase::GetAllConversations() {
statement.ColumnInt(index++);
conversation->associated_content->is_content_refined =
statement.ColumnBool(index++);
conversation->associated_content->is_content_association_possible = true;
} else {
conversation->associated_content->is_content_association_possible = false;
}
}

Expand Down Expand Up @@ -391,10 +386,10 @@ bool AIChatDatabase::AddConversation(mojom::ConversationPtr conversation,
return false;
}

if (conversation->associated_content->is_content_association_possible) {
if (conversation->associated_content) {
DVLOG(2) << "Adding associated content for conversation "
<< conversation->uuid << " with url "
<< conversation->associated_content->url->spec();
<< conversation->associated_content->url.spec();
if (!AddOrUpdateAssociatedContent(
conversation->uuid, std::move(conversation->associated_content),
contents)) {
Expand All @@ -417,7 +412,7 @@ bool AIChatDatabase::AddConversation(mojom::ConversationPtr conversation,

bool AIChatDatabase::AddOrUpdateAssociatedContent(
std::string_view conversation_uuid,
mojom::SiteInfoPtr associated_content,
mojom::AssociatedContentPtr associated_content,
std::optional<std::string> contents) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!LazyInit()) {
Expand All @@ -426,7 +421,7 @@ bool AIChatDatabase::AddOrUpdateAssociatedContent(

// TODO(petemill): handle multiple associated content per conversation
CHECK(!conversation_uuid.empty());
CHECK(associated_content->uuid.has_value());
CHECK(associated_content);

// Check if we already have persisted this content
static constexpr char kSelectExistingAssociatedContentId[] =
Expand All @@ -436,13 +431,12 @@ bool AIChatDatabase::AddOrUpdateAssociatedContent(
SQL_FROM_HERE, kSelectExistingAssociatedContentId));
CHECK(select_statement.is_valid());
select_statement.BindString(0, conversation_uuid);
select_statement.BindString(1, associated_content->uuid.value());
select_statement.BindString(1, associated_content->uuid);

sql::Statement statement;
if (select_statement.Step()) {
DVLOG(4) << "Updating associated content for conversation "
<< conversation_uuid << " with id "
<< associated_content->uuid.value();
<< conversation_uuid << " with id " << associated_content->uuid;
static constexpr char kUpdateAssociatedContentQuery[] =
"UPDATE associated_content"
" SET title = ?,"
Expand All @@ -467,13 +461,13 @@ bool AIChatDatabase::AddOrUpdateAssociatedContent(
int index = 0;
BindAndEncryptOptionalString(statement, index++, associated_content->title);
BindAndEncryptOptionalString(statement, index++,
associated_content->url->spec());
associated_content->url.spec());
statement.BindInt(index++,
base::to_underlying(associated_content->content_type));
BindAndEncryptOptionalString(statement, index++, contents);
statement.BindInt(index++, associated_content->content_used_percentage);
statement.BindBool(index++, associated_content->is_content_refined);
statement.BindString(index++, associated_content->uuid.value());
statement.BindString(index++, associated_content->uuid);
statement.BindString(index, conversation_uuid);

if (!statement.Run()) {
Expand Down Expand Up @@ -874,8 +868,7 @@ bool AIChatDatabase::DeleteAssociatedWebContent(
// Set any associated content url, title and content to NULL where
// conversation had any entry between begin_time and end_time.
static constexpr char kQuery[] =
"UPDATE associated_content"
" SET url=NULL, title=NULL, last_contents=NULL"
"DELETE FROM associated_content"
" WHERE conversation_uuid IN ("
" SELECT conversation_uuid"
" FROM conversation_entry"
Expand Down Expand Up @@ -979,9 +972,8 @@ bool AIChatDatabase::CreateSchema() {
"title BLOB,"
// Encrypted url string
"url BLOB,"
// Stores SiteInfo.IsVideo. Future-proofed for multiple content types
// 0 for regular content
// 1 for video.
// Stores AssociatedContent.IsVideo. Future-proofed for multiple content
// types 0 for regular content 1 for video.
"content_type INTEGER NOT NULL,"
// Encrypted string value of the content, so that conversations can be
// continued.
Expand Down
7 changes: 4 additions & 3 deletions components/ai_chat/core/browser/ai_chat_database.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,10 @@ class AIChatDatabase {
mojom::ConversationTurnPtr first_entry);

// Update any properties of associated content metadata or full-text content
bool AddOrUpdateAssociatedContent(std::string_view conversation_uuid,
mojom::SiteInfoPtr associated_content,
std::optional<std::string> content);
bool AddOrUpdateAssociatedContent(
std::string_view conversation_uuid,
mojom::AssociatedContentPtr associated_content,
std::optional<std::string> content);

// Adds a new conversation entry to the conversation with the provided UUID
bool AddConversationEntry(
Expand Down
45 changes: 18 additions & 27 deletions components/ai_chat/core/browser/ai_chat_database_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,11 @@ TEST_P(AIChatDatabaseTest, AddAndGetConversationAndEntries) {
// recent entry.
const GURL page_url = GURL("https://example.com/page");
const std::string expected_contents = "Page contents";
mojom::SiteInfoPtr associated_content =
has_content
? mojom::SiteInfo::New(
content_uuid, mojom::ContentType::PageContent, "page title",
page_url.host(), page_url, 62, true, true)
: mojom::SiteInfo::New(
std::nullopt, mojom::ContentType::PageContent, std::nullopt,
std::nullopt, std::nullopt, 0, false, false);
mojom::AssociatedContentPtr associated_content =
has_content ? mojom::AssociatedContent::New(
content_uuid, mojom::ContentType::PageContent,
"page title", page_url, 62, true)
: nullptr;
const mojom::ConversationPtr metadata =
mojom::Conversation::New(uuid, "title", now - base::Hours(2), true,
std::nullopt, std::move(associated_content));
Expand Down Expand Up @@ -264,10 +261,7 @@ TEST_P(AIChatDatabaseTest, UpdateConversationTitle) {
base::StrCat({"for_conversation_title_", initial_title});
const std::string updated_title = "updated title";
mojom::ConversationPtr metadata = mojom::Conversation::New(
uuid, initial_title, base::Time::Now(), true, std::nullopt,
mojom::SiteInfo::New(std::nullopt, mojom::ContentType::PageContent,
std::nullopt, std::nullopt, std::nullopt, 0, false,
false));
uuid, initial_title, base::Time::Now(), true, std::nullopt, nullptr);

// Persist the first entry (and get the response ready)
const auto history = CreateSampleChatHistory(1u);
Expand Down Expand Up @@ -297,9 +291,9 @@ TEST_P(AIChatDatabaseTest, AddOrUpdateAssociatedContent) {
const GURL page_url = GURL("https://example.com/page");
mojom::ConversationPtr metadata = mojom::Conversation::New(
uuid, "title", base::Time::Now() - base::Hours(2), true, std::nullopt,
mojom::SiteInfo::New(content_uuid, mojom::ContentType::PageContent,
"page title", page_url.host(), page_url, 62, true,
true));
mojom::AssociatedContent::New(content_uuid,
mojom::ContentType::PageContent,
"page title", page_url, 62, true));

auto history = CreateSampleChatHistory(1u);

Expand Down Expand Up @@ -328,7 +322,7 @@ TEST_P(AIChatDatabaseTest, AddOrUpdateAssociatedContent) {
result = db_->GetConversationData(uuid);
EXPECT_EQ(result->associated_content.size(), 1u);
EXPECT_EQ(result->associated_content[0]->content_uuid,
metadata->associated_content->uuid.value());
metadata->associated_content->uuid);
EXPECT_EQ(result->associated_content[0]->content, expected_contents);
conversations = db_->GetAllConversations();
EXPECT_EQ(conversations.size(), 1u);
Expand All @@ -340,9 +334,7 @@ TEST_P(AIChatDatabaseTest, DeleteAllData) {
const GURL page_url = GURL("https://example.com/page");
mojom::ConversationPtr metadata = mojom::Conversation::New(
uuid, "title", base::Time::Now() - base::Hours(2), true, std::nullopt,
mojom::SiteInfo::New(std::nullopt, mojom::ContentType::PageContent,
std::nullopt, std::nullopt, std::nullopt, 0, false,
false));
nullptr);

auto history = CreateSampleChatHistory(1u);

Expand Down Expand Up @@ -379,14 +371,14 @@ TEST_P(AIChatDatabaseTest, DeleteAssociatedWebContent) {
// are persisted.
mojom::ConversationPtr metadata_first = mojom::Conversation::New(
"first", "title", base::Time::Now() - base::Hours(2), true, std::nullopt,
mojom::SiteInfo::New("first-content", mojom::ContentType::PageContent,
"page title", page_url.host(), page_url, 62, true,
true));
mojom::AssociatedContent::New("first-content",
mojom::ContentType::PageContent,
"page title", page_url, 62, true));
mojom::ConversationPtr metadata_second = mojom::Conversation::New(
"second", "title", base::Time::Now() - base::Hours(1), true, "model-2",
mojom::SiteInfo::New("second-content", mojom::ContentType::PageContent,
"page title", page_url.host(), page_url, 62, true,
true));
mojom::AssociatedContent::New("second-content",
mojom::ContentType::PageContent,
"page title", page_url, 62, true));

auto history_first = CreateSampleChatHistory(1u, -2);
auto history_second = CreateSampleChatHistory(1u, -1);
Expand Down Expand Up @@ -426,8 +418,7 @@ TEST_P(AIChatDatabaseTest, DeleteAssociatedWebContent) {
conversations = db_->GetAllConversations();
EXPECT_EQ(conversations.size(), 2u);
ExpectConversationEquals(FROM_HERE, conversations[0], metadata_first);
metadata_second->associated_content->url = std::nullopt;
metadata_second->associated_content->title = std::nullopt;
metadata_second->associated_content = nullptr;
ExpectConversationEquals(FROM_HERE, conversations[1], metadata_second);

archive_result = db_->GetConversationData("second");
Expand Down
9 changes: 3 additions & 6 deletions components/ai_chat/core/browser/ai_chat_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,7 @@ ConversationHandler* AIChatService::CreateConversation() {
// Create the conversation metadata
{
mojom::ConversationPtr conversation = mojom::Conversation::New(
conversation_uuid, "", base::Time::Now(), false, std::nullopt,
mojom::SiteInfo::New(base::Uuid::GenerateRandomV4().AsLowercaseString(),
mojom::ContentType::PageContent, std::nullopt,
std::nullopt, std::nullopt, 0, false, false));
conversation_uuid, "", base::Time::Now(), false, std::nullopt, nullptr);
conversations_.insert_or_assign(conversation_uuid, std::move(conversation));
}
mojom::Conversation* conversation =
Expand Down Expand Up @@ -745,7 +742,7 @@ void AIChatService::HandleFirstEntry(
DVLOG(1) << __func__ << " Conversation " << conversation->uuid
<< " being persisted for first time.";
CHECK(entry->uuid.has_value());
CHECK(conversation->associated_content->uuid.has_value());

// We can persist the conversation metadata for the first time as well as the
// entry.
if (ai_chat_db_) {
Expand Down Expand Up @@ -780,7 +777,7 @@ void AIChatService::HandleNewEntry(
conversation->model_key, std::nullopt);

if (associated_content_value.has_value() &&
conversation->associated_content->is_content_association_possible) {
conversation->associated_content) {
ai_chat_db_
.AsyncCall(
base::IgnoreResult(&AIChatDatabase::AddOrUpdateAssociatedContent))
Expand Down
40 changes: 18 additions & 22 deletions components/ai_chat/core/browser/ai_chat_service_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ class MockConversationHandlerClient : public mojom::ConversationUI {
conversation_ui_receiver_.reset();
}


MOCK_METHOD(void, OnConversationHistoryUpdate, (), (override));

MOCK_METHOD(void, OnAPIRequestInProgress, (bool), (override));
Expand All @@ -133,7 +132,7 @@ class MockConversationHandlerClient : public mojom::ConversationUI {

MOCK_METHOD(void,
OnAssociatedContentInfoChanged,
(const mojom::SiteInfoPtr, bool),
(const mojom::AssociatedContentPtr, bool),
(override));

MOCK_METHOD(void, OnFaviconImageDataChanged, (), (override));
Expand Down Expand Up @@ -496,10 +495,10 @@ TEST_P(AIChatServiceUnitTest, GetOrCreateConversationHandlerForContent) {
base::RunLoop run_loop;
conversation_with_content->GetAssociatedContentInfo(
base::BindLambdaForTesting(
[&](mojom::SiteInfoPtr site_info, bool should_send_page_contents) {
EXPECT_TRUE(site_info->is_content_association_possible);
EXPECT_TRUE(site_info->url.has_value());
EXPECT_EQ(site_info->url.value(), GURL("https://example.com"));
[&](mojom::AssociatedContentPtr associated_content,
bool should_send_page_contents) {
EXPECT_TRUE(associated_content);
EXPECT_EQ(associated_content->url, GURL("https://example.com"));
run_loop.Quit();
}));
run_loop.Run();
Expand Down Expand Up @@ -558,10 +557,10 @@ TEST_P(AIChatServiceUnitTest,
base::RunLoop run_loop;
conversation_with_content->GetAssociatedContentInfo(
base::BindLambdaForTesting(
[&](mojom::SiteInfoPtr site_info, bool should_send_page_contents) {
[&](mojom::AssociatedContentPtr associated_content,
bool should_send_page_contents) {
EXPECT_FALSE(should_send_page_contents);
EXPECT_FALSE(site_info->is_content_association_possible);
EXPECT_FALSE(site_info->url.has_value());
EXPECT_FALSE(associated_content);
run_loop.Quit();
}));
run_loop.Run();
Expand Down Expand Up @@ -814,13 +813,12 @@ TEST_P(AIChatServiceUnitTest, DeleteAssociatedWebContent) {
// Verify associated are initially correct
base::RunLoop run_loop;
data[i].conversation_handler->GetAssociatedContentInfo(
base::BindLambdaForTesting([&](mojom::SiteInfoPtr site_info,
base::BindLambdaForTesting([&](mojom::AssociatedContentPtr site_info,
bool should_send_page_contents) {
SCOPED_TRACE(testing::Message() << "data index: " << i);
EXPECT_TRUE(site_info->is_content_association_possible);
EXPECT_TRUE(site_info->url.has_value());
EXPECT_EQ(site_info->url.value(), content_url);
EXPECT_EQ(site_info->title.value(), base::UTF16ToUTF8(page_title));
ASSERT_TRUE(site_info);
EXPECT_EQ(site_info->url, content_url);
EXPECT_EQ(site_info->title, base::UTF16ToUTF8(page_title));
run_loop.Quit();
}));
run_loop.Run();
Expand Down Expand Up @@ -849,18 +847,16 @@ TEST_P(AIChatServiceUnitTest, DeleteAssociatedWebContent) {
for (int i = 0; i < 3; i++) {
base::RunLoop run_loop;
data[i].conversation_handler->GetAssociatedContentInfo(
base::BindLambdaForTesting([&](mojom::SiteInfoPtr site_info,
base::BindLambdaForTesting([&](mojom::AssociatedContentPtr site_info,
bool should_send_page_contents) {
SCOPED_TRACE(testing::Message() << "data index: " << i);
EXPECT_TRUE(site_info->is_content_association_possible);
EXPECT_TRUE(site_info->url.has_value());
EXPECT_TRUE(site_info->title.has_value());
ASSERT_TRUE(site_info);
if (i == 1) {
EXPECT_TRUE(site_info->url->is_empty());
EXPECT_TRUE(site_info->title->empty());
EXPECT_TRUE(site_info->url.is_empty());
EXPECT_TRUE(site_info->title.empty());
} else {
EXPECT_EQ(site_info->url.value(), content_url);
EXPECT_EQ(site_info->title.value(), base::UTF16ToUTF8(page_title));
EXPECT_EQ(site_info->url, content_url);
EXPECT_EQ(site_info->title, base::UTF16ToUTF8(page_title));
}
run_loop.Quit();
}));
Expand Down
Loading
Loading