From 55b8b55a764b4038f00949297b7120e89ff65378 Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Wed, 20 Mar 2024 21:42:11 -0700 Subject: [PATCH] review feedback --- .../ai_chat/content/browser/ai_chat_tab_helper.cc | 3 +-- components/ai_chat/core/browser/conversation_driver.cc | 10 ++++------ components/ai_chat/core/browser/conversation_driver.h | 2 +- .../renderer/ai_chat_resource_sniffer_throttle.cc | 1 - .../renderer/ai_chat_resource_sniffer_url_loader.cc | 2 -- components/ai_chat/renderer/page_content_extractor.cc | 8 ++------ components/ai_chat/renderer/page_content_extractor.h | 2 +- components/ai_chat/renderer/yt_util.cc | 8 ++++---- components/ai_chat/renderer/yt_util.h | 4 ++-- renderer/brave_url_loader_throttle_provider_impl.cc | 2 +- 10 files changed, 16 insertions(+), 26 deletions(-) diff --git a/components/ai_chat/content/browser/ai_chat_tab_helper.cc b/components/ai_chat/content/browser/ai_chat_tab_helper.cc index 2727d1618618c..8170e729be57d 100644 --- a/components/ai_chat/content/browser/ai_chat_tab_helper.cc +++ b/components/ai_chat/content/browser/ai_chat_tab_helper.cc @@ -67,7 +67,7 @@ AIChatTabHelper::PDFA11yInfoLoadObserver::~PDFA11yInfoLoadObserver() = default; void AIChatTabHelper::BindPageContentExtractorHost( content::RenderFrameHost* rfh, mojo::PendingAssociatedReceiver receiver) { - DCHECK(rfh); + CHECK(rfh); if (!rfh->IsInPrimaryMainFrame()) { DVLOG(4) << "Not binding extractor host to non-main frame"; return; @@ -221,7 +221,6 @@ void AIChatTabHelper::OnFaviconUpdated( // mojom::PageContentExtractorHost void AIChatTabHelper::OnInterceptedPageContentChanged() { - DVLOG(2) << __func__; // Maybe mark that the page changed, if we didn't detect it already via title // change after a same-page navigation. This is the main benefit of this // function. diff --git a/components/ai_chat/core/browser/conversation_driver.cc b/components/ai_chat/core/browser/conversation_driver.cc index b5bf50956638c..110345788e8f4 100644 --- a/components/ai_chat/core/browser/conversation_driver.cc +++ b/components/ai_chat/core/browser/conversation_driver.cc @@ -472,8 +472,8 @@ void ConversationDriver::OnGeneratePageContentComplete( std::string contents_text, bool is_video, std::string invalidation_token) { - VLOG(1) << "OnGeneratePageContentComplete"; - VLOG(4) << "Contents(is_video=" << is_video + DVLOG(1) << "OnGeneratePageContentComplete"; + DVLOG(4) << "Contents(is_video=" << is_video << ", invalidation_token=" << invalidation_token << "): " << contents_text; if (navigation_id != current_navigation_id_) { @@ -483,15 +483,13 @@ void ConversationDriver::OnGeneratePageContentComplete( // Ignore if we received content from observer in the meantime if (!is_page_text_fetch_in_progress_) { - VLOG(1) << __func__ + DVLOG(1) << __func__ << " but already received contents from observer. Ignoring."; return; } OnPageContentUpdated(contents_text, is_video, invalidation_token); - VLOG(4) << "calling callback with text: " << article_text_; - std::move(callback).Run(article_text_, is_video_, content_invalidation_token_); } @@ -500,7 +498,7 @@ void ConversationDriver::OnExistingGeneratePageContentComplete( GetPageContentCallback callback) { // Don't need to check navigation ID since existing event will be // deleted when there's a new conversation. - VLOG(1) << "Existing page content fetch completed, proceeding with " + DVLOG(1) << "Existing page content fetch completed, proceeding with " "the results of that operation."; std::move(callback).Run(article_text_, is_video_, content_invalidation_token_); diff --git a/components/ai_chat/core/browser/conversation_driver.h b/components/ai_chat/core/browser/conversation_driver.h index e47fa6407b418..10d18ba3175ea 100644 --- a/components/ai_chat/core/browser/conversation_driver.h +++ b/components/ai_chat/core/browser/conversation_driver.h @@ -147,7 +147,7 @@ class ConversationDriver { virtual void OnFaviconImageDataChanged(); // Implementer should call this when the content is updated in a way that - // will not be detected by the on-demand techniquesused by GetPageContent. + // will not be detected by the on-demand techniques used by GetPageContent. // For example for sites where GetPageContent does not read the live DOM but // reads static JS from HTML that doesn't change for same-page navigation and // we need to intercept new JS data from subresource loads. diff --git a/components/ai_chat/renderer/ai_chat_resource_sniffer_throttle.cc b/components/ai_chat/renderer/ai_chat_resource_sniffer_throttle.cc index 5819b9ca6fe02..8220cd48fc87a 100644 --- a/components/ai_chat/renderer/ai_chat_resource_sniffer_throttle.cc +++ b/components/ai_chat/renderer/ai_chat_resource_sniffer_throttle.cc @@ -29,7 +29,6 @@ AIChatResourceSnifferThrottle::MaybeCreateThrottleFor( // |mojom::PageContent|. if (url.SchemeIsHTTPOrHTTPS() && base::Contains(kYouTubeHosts, url.host()) && base::EqualsCaseInsensitiveASCII(url.path(), kYouTubePlayerAPIPath)) { - VLOG(1) << __func__ << " Creating throttle for url: " << url.spec(); return std::make_unique(task_runner, delegate); } diff --git a/components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.cc b/components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.cc index 8a2ab4026ccd7..ba1136d043dce 100644 --- a/components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.cc +++ b/components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.cc @@ -86,8 +86,6 @@ void AIChatResourceSnifferURLLoader::OnBodyWritable(MojoResult r) { } void AIChatResourceSnifferURLLoader::CompleteLoading(std::string body) { - DVLOG(4) << __func__ << ": got body length: " << body.size() - << " for url: " << response_url_.spec(); if (!body.empty()) { auto content = std::make_unique< AIChatResourceSnifferThrottleDelegate::InterceptedContent>(); diff --git a/components/ai_chat/renderer/page_content_extractor.cc b/components/ai_chat/renderer/page_content_extractor.cc index 5e716fb99bae7..3c31b14d79fd2 100644 --- a/components/ai_chat/renderer/page_content_extractor.cc +++ b/components/ai_chat/renderer/page_content_extractor.cc @@ -113,7 +113,7 @@ void PageContentExtractor::ExtractPageContent( if (intercepted_content_) { auto intercepted_content = std::move(intercepted_content_); intercepted_content_.reset(); - VLOG(1) << "Using intercepted content."; + DVLOG(1) << "Using intercepted content."; DCHECK_EQ(intercepted_content->type, InterceptedContentType::kYouTubeMetadataString) << "Unexpected intercepted content type"; @@ -219,11 +219,7 @@ void PageContentExtractor::OnInterceptedPageContentChanged( // "page" change. mojo::AssociatedRemote host; render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(&host); - if (host.is_bound()) { - host->OnInterceptedPageContentChanged(); - } else { - DVLOG(1) << __func__ << " - no host bound to forward content event to."; - } + host->OnInterceptedPageContentChanged(); } void PageContentExtractor::BindReceiver( diff --git a/components/ai_chat/renderer/page_content_extractor.h b/components/ai_chat/renderer/page_content_extractor.h index 66c571b63cac8..c2fd55517b6c4 100644 --- a/components/ai_chat/renderer/page_content_extractor.h +++ b/components/ai_chat/renderer/page_content_extractor.h @@ -71,7 +71,7 @@ class PageContentExtractor int32_t isolated_world_id_; std::unique_ptr - intercepted_content_ = nullptr; + intercepted_content_; base::WeakPtrFactory weak_ptr_factory_{this}; }; diff --git a/components/ai_chat/renderer/yt_util.cc b/components/ai_chat/renderer/yt_util.cc index 563eb7632f2c5..b13171b4f834e 100644 --- a/components/ai_chat/renderer/yt_util.cc +++ b/components/ai_chat/renderer/yt_util.cc @@ -71,7 +71,7 @@ std::optional ChooseCaptionTrackUrl( return *caption_url_raw; } -std::optional ParseAndChooseCaptionTrackUrl(std::string& body) { +std::optional ParseAndChooseCaptionTrackUrl(std::string_view body) { if (!body.size()) { return std::nullopt; } @@ -80,17 +80,17 @@ std::optional ParseAndChooseCaptionTrackUrl(std::string& body) { base::JSONReader::ReadAndReturnValueWithError(body, base::JSON_PARSE_RFC); if (!result_value.has_value() || result_value->is_string()) { - VLOG(1) << __func__ << ": parsing error: " << result_value.ToString(); + DVLOG(1) << __func__ << ": parsing error: " << result_value.ToString(); return std::nullopt; } else if (!result_value->is_dict()) { - VLOG(1) << __func__ << ": parsing error: not a dict"; + DVLOG(1) << __func__ << ": parsing error: not a dict"; return std::nullopt; } auto* caption_tracks = result_value->GetDict().FindListByDottedPath( "captions.playerCaptionsTracklistRenderer.captionTracks"); if (!caption_tracks) { - VLOG(1) << __func__ << ": no caption tracks found"; + DVLOG(1) << __func__ << ": no caption tracks found"; return std::nullopt; } diff --git a/components/ai_chat/renderer/yt_util.h b/components/ai_chat/renderer/yt_util.h index 33b0159c155dc..db0bdfc14ca12 100644 --- a/components/ai_chat/renderer/yt_util.h +++ b/components/ai_chat/renderer/yt_util.h @@ -25,11 +25,11 @@ inline constexpr auto kYouTubeHosts = // Extract a caption url from an array of YT caption tracks, from the YT page // API. std::optional ChooseCaptionTrackUrl( - base::Value::List* caption_tracks); + const base::Value::List* caption_tracks); // Parse YT metadata json string and choose the most appropriate caption track // url. -std::optional ParseAndChooseCaptionTrackUrl(std::string& body); +std::optional ParseAndChooseCaptionTrackUrl(std::string_view body); } // namespace ai_chat diff --git a/renderer/brave_url_loader_throttle_provider_impl.cc b/renderer/brave_url_loader_throttle_provider_impl.cc index 209a97e8afdb7..459ae50eee8c7 100644 --- a/renderer/brave_url_loader_throttle_provider_impl.cc +++ b/renderer/brave_url_loader_throttle_provider_impl.cc @@ -101,7 +101,7 @@ BraveURLLoaderThrottleProviderImpl::CreateThrottles( ai_chat_resource_throttle = ai_chat::AIChatResourceSnifferThrottle::MaybeCreateThrottleFor( page_content_delegate->GetWeakPtr(), request.url, - base::SingleThreadTaskRunner::GetCurrentDefault()); + base::SequencedTaskRunner::GetCurrentDefault()); if (ai_chat_resource_throttle) { throttles.emplace_back(std::move(ai_chat_resource_throttle)); }