Discharge7443/brave-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Addressed review comments

Browse Source
This commit is contained in:
Simon Hong
2022-09-21 18:57:29 +09:00
parent 0d9ed5df7e
commit 518e3dedaf
15 changed files with 79 additions and 82 deletions
Download Patch File Download Diff File Expand all files Collapse all files
browser/brave_content_browser_client.cc
+7 -3
View File
@@ -212,7 +212,8 @@ using extensions::ChromeContentBrowserClientExtensionsPart;
#endif
#if BUILDFLAG(ENABLE_PLAYLIST)
#include "brave/components/playlist/playlist_service_helper.h"
#include "brave/browser/playlist/playlist_service_factory.h"
#include "brave/components/playlist/playlist_service.h"
#endif
#if BUILDFLAG(ENABLE_PLAYLIST_WEBUI)
@@ -1053,8 +1054,11 @@ void BraveContentBrowserClient::OverrideWebkitPrefs(WebContents* web_contents,
web_prefs->allow_non_empty_navigator_plugins = true;
#if BUILDFLAG(ENABLE_PLAYLIST)
if (playlist::IsBackgroundWebContents(web_contents)) {
web_prefs->allow_cosmetic_filtering = true;
if (auto* playlist_service =
playlist::PlaylistServiceFactory::GetForBrowserContext(
web_contents->GetBrowserContext())) {
playlist_service->ConfigureWebPrefsforBackgroundWebContents(web_contents,
web_prefs);
}
#endif
}
browser/brave_shields/ad_block_service_browsertest.cc
+13 -16
View File
@@ -54,7 +54,6 @@
#if BUILDFLAG(ENABLE_PLAYLIST)
#include "brave/browser/playlist/playlist_service_factory.h"
#include "brave/components/playlist/features.h"
#include "brave/components/playlist/playlist_download_request_manager.h"
#include "brave/components/playlist/playlist_service.h"
#endif
@@ -1800,20 +1799,26 @@ IN_PROC_BROWSER_TEST_F(CosmeticFilteringFlagDisabledTest,
#if BUILDFLAG(ENABLE_PLAYLIST)
namespace playlist {
class CosmeticFilteringPlaylistFlagEnabledTest : public AdBlockServiceTest {
public:
CosmeticFilteringPlaylistFlagEnabledTest() {
feature_list_.InitAndEnableFeature(features::kPlaylist);
feature_list_.InitAndEnableFeature(playlist::features::kPlaylist);
}
content::WebContents* GetBackgroundWebContents() {
auto* playlist_service =
playlist::PlaylistServiceFactory::GetForBrowserContext(
browser()->profile());
return playlist_service->GetBackgroundWebContentsForTesting();
}
private:
base::test::ScopedFeatureList feature_list_;
};
// Ensure cosmetic filtering occurs always when AllowCosmeticFiltering() is
// called.
// Check cosmetic filtering is applied to any loading from Playlist's
// background web contents.
IN_PROC_BROWSER_TEST_F(CosmeticFilteringPlaylistFlagEnabledTest,
AllowCosmeticFiltering) {
ASSERT_TRUE(InstallDefaultAdBlockExtension());
@@ -1825,25 +1830,17 @@ IN_PROC_BROWSER_TEST_F(CosmeticFilteringPlaylistFlagEnabledTest,
content_settings(), brave_shields::ControlType::ALLOW, url);
UpdateAdBlockInstanceWithRules("b.com###ad-banner");
auto* playlist_service_factory = PlaylistServiceFactory::GetInstance();
auto* playlist_service =
playlist_service_factory->GetForBrowserContext(browser()->profile());
auto* download_request_manager =
playlist_service->download_request_manager_.get();
download_request_manager->CreateWebContents();
auto* web_contents = download_request_manager->web_contents_.get();
auto* web_contents = GetBackgroundWebContents();
web_contents->GetController().LoadURLWithParams(
content::NavigationController::LoadURLParams(url));
content::WaitForLoadStop(web_contents);
EXPECT_TRUE(content::WaitForLoadStop(web_contents));
// Check filter is applied properly.
EXPECT_EQ(false, EvalJs(web_contents,
"checkSelector('#ad-banner', 'display', 'block')"));
}
} // namespace playlist
#endif
// Ensure no cosmetic filtering occurs when the shields setting is disabled
chromium_src/third_party/blink/common/web_preferences/web_preferences.cc
Vendored
-4
View File
@@ -17,14 +17,10 @@ namespace blink {
namespace web_pref {
WebPreferences::WebPreferences(const WebPreferences& other) = default;
WebPreferences::WebPreferences(WebPreferences&& other) = default;
WebPreferences::~WebPreferences() = default;
WebPreferences& WebPreferences::operator=(const WebPreferences& other) =
default;
WebPreferences& WebPreferences::operator=(WebPreferences&& other) = default;
} // namespace web_pref
chromium_src/third_party/blink/common/web_preferences/web_preferences_mojom_traits.cc
Vendored
+1 -1
View File
@@ -23,7 +23,7 @@ bool StructTraits<blink::mojom::WebPreferencesDataView,
return false;
}
out->allow_cosmetic_filtering = data.allow_cosmetic_filtering();
out->force_cosmetic_filtering = data.force_cosmetic_filtering();
return true;
}
chromium_src/third_party/blink/public/common/web_preferences/web_preferences.h
Vendored
+1 -2
View File
@@ -25,8 +25,7 @@ struct BLINK_COMMON_EXPORT WebPreferences : public WebPreferences_ChromiumImpl {
WebPreferences& operator=(const WebPreferences& other);
WebPreferences& operator=(WebPreferences&& other);
// Force to allow cosmetic filtering.
bool allow_cosmetic_filtering = false;
bool force_cosmetic_filtering = false;
};
} // namespace web_pref
chromium_src/third_party/blink/public/common/web_preferences/web_preferences_mojom_traits.h
Vendored
+2 -2
View File
@@ -22,9 +22,9 @@ struct BLINK_COMMON_EXPORT StructTraits<blink::mojom::WebPreferencesDataView,
blink::web_pref::WebPreferences>
: public StructTraits<blink::mojom::WebPreferencesDataView,
blink::web_pref::WebPreferences_ChromiumImpl> {
static bool allow_cosmetic_filtering(
static bool force_cosmetic_filtering(
const blink::web_pref::WebPreferences& r) {
return r.allow_cosmetic_filtering;
return r.force_cosmetic_filtering;
}
static bool Read(blink::mojom::WebPreferencesDataView r,
chromium_src/third_party/blink/public/mojom/webpreferences/web_preferences.mojom
Vendored
+1 -1
View File
@@ -7,5 +7,5 @@ module blink.mojom;
[BraveExtend]
struct WebPreferences {
bool allow_cosmetic_filtering;
bool force_cosmetic_filtering;
};
components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc
+3 -3
View File
@@ -276,9 +276,9 @@ bool CosmeticFiltersJSHandler::ProcessURL(
static_cast<content_settings::BraveContentSettingsAgentImpl*>(
content_settings::ContentSettingsAgentImpl::Get(render_frame_));
const bool always_allow_cosmetic_filtering =
render_frame_->GetBlinkPreferences().allow_cosmetic_filtering;
if (!always_allow_cosmetic_filtering &&
const bool force_cosmetic_filtering =
render_frame_->GetBlinkPreferences().force_cosmetic_filtering;
if (!force_cosmetic_filtering &&
!content_settings->IsCosmeticFilteringEnabled(url_)) {
return false;
}
components/playlist/DEPS
+1 -1
View File
@@ -8,6 +8,6 @@ include_rules = [
"+content/public/common",
"+services/network/public/cpp",
"+services/preferences/public/cpp",
"+third_party/blink/public/common",
"+third_party/blink/public/common/web_preferences",
"+third_party/re2",
]
components/playlist/playlist_download_request_manager.cc
+15 -2
View File
@@ -12,13 +12,13 @@
#include "base/strings/utf_string_conversions.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "brave/components/playlist/playlist_service_helper.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/isolated_world_ids.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "third_party/blink/public/common/web_preferences/web_preferences.h"
#include "third_party/re2/src/re2/re2.h"
#include "ui/base/page_transition_types.h"
@@ -72,7 +72,6 @@ void PlaylistDownloadRequestManager::CreateWebContents() {
// |web_contents_| is created on demand.
content::WebContents::CreateParams create_params(context_, nullptr);
web_contents_ = content::WebContents::Create(create_params);
MarkAsBackgroundWebContents(web_contents_.get());
}
Observe(web_contents_.get());
@@ -257,4 +256,18 @@ void PlaylistDownloadRequestManager::DestroyWebContents() {
web_contents_.reset();
}
void PlaylistDownloadRequestManager::ConfigureWebPrefsforBackgroundWebContents(
content::WebContents* web_contents,
blink::web_pref::WebPreferences* web_prefs) {
if (web_contents_ && web_contents_.get() == web_contents) {
web_prefs->force_cosmetic_filtering = true;
}
}
content::WebContents*
PlaylistDownloadRequestManager::GetBackgroundWebContentsForTesting() {
CreateWebContents();
return web_contents_.get();
}
} // namespace playlist
components/playlist/playlist_download_request_manager.h
+11 -3
View File
@@ -25,6 +25,10 @@ class OneShotTimer;
class Value;
} // namespace base
namespace blink::web_pref {
struct WebPreferences;
} // namespace blink::web_pref
namespace content {
class BrowserContext;
} // namespace content
@@ -66,10 +70,14 @@ class PlaylistDownloadRequestManager
// Request::callback will be called with generated param.
void GetMediaFilesFromPage(Request request);
private:
FRIEND_TEST_ALL_PREFIXES(CosmeticFilteringPlaylistFlagEnabledTest,
AllowCosmeticFiltering);
// Update |web_prefs| if we want for |web_contents|.
void ConfigureWebPrefsforBackgroundWebContents(
content::WebContents* web_contents,
blink::web_pref::WebPreferences* web_prefs);
content::WebContents* GetBackgroundWebContentsForTesting();
private:
// Calling this will trigger loading |url| on a web contents,
// and we'll inject javascript on the contents to get a list of
// media files on the page.
components/playlist/playlist_service.cc
+11
View File
@@ -219,6 +219,17 @@ base::FilePath PlaylistService::GetPlaylistItemDirPath(
return base_dir_.AppendASCII(id);
}
void PlaylistService::ConfigureWebPrefsforBackgroundWebContents(
content::WebContents* web_contents,
blink::web_pref::WebPreferences* web_prefs) {
download_request_manager_->ConfigureWebPrefsforBackgroundWebContents(
web_contents, web_prefs);
}
content::WebContents* PlaylistService::GetBackgroundWebContentsForTesting() {
return download_request_manager_->GetBackgroundWebContentsForTesting();
}
void PlaylistService::UpdatePlaylistItemValue(const std::string& id,
base::Value value) {
prefs::ScopedDictionaryPrefUpdate update(prefs_, kPlaylistItemsPref);
components/playlist/playlist_service.h
+13 -2
View File
@@ -25,12 +25,17 @@ namespace base {
class SequencedTaskRunner;
} // namespace base
namespace blink::web_pref {
struct WebPreferences;
} // namespace blink::web_pref
namespace content {
class BrowserContext;
class WebContents;
} // namespace content
class PrefService;
class CosmeticFilteringPlaylistFlagEnabledTest;
namespace playlist {
@@ -119,15 +124,19 @@ class PlaylistService : public KeyedService,
base::FilePath GetPlaylistItemDirPath(const std::string& id) const;
// Update |web_prefs| if we want for |web_contents|.
void ConfigureWebPrefsforBackgroundWebContents(
content::WebContents* web_contents,
blink::web_pref::WebPreferences* web_prefs);
private:
friend class ::CosmeticFilteringPlaylistFlagEnabledTest;
FRIEND_TEST_ALL_PREFIXES(PlaylistBrowserTest, ApiFunctions);
FRIEND_TEST_ALL_PREFIXES(PlaylistBrowserTest, CreatePlaylist);
FRIEND_TEST_ALL_PREFIXES(PlaylistBrowserTest, CreatePlaylistItem);
FRIEND_TEST_ALL_PREFIXES(PlaylistBrowserTest, MediaDownloadFailed);
FRIEND_TEST_ALL_PREFIXES(PlaylistBrowserTest, ThumbnailFailed);
FRIEND_TEST_ALL_PREFIXES(PlaylistBrowserTest, RemoveAndRestoreLocalData);
FRIEND_TEST_ALL_PREFIXES(CosmeticFilteringPlaylistFlagEnabledTest,
AllowCosmeticFiltering);
// KeyedService overrides:
void Shutdown() override;
@@ -179,6 +188,8 @@ class PlaylistService : public KeyedService,
void OnGetMetadata(base::Value value);
content::WebContents* GetBackgroundWebContentsForTesting();
const base::FilePath base_dir_;
base::ObserverList<PlaylistServiceObserver> observers_;
components/playlist/playlist_service_helper.cc
-36
View File
@@ -9,37 +9,9 @@
#include "brave/components/playlist/playlist_constants.h"
#include "brave/components/playlist/playlist_types.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_user_data.h"
namespace playlist {
namespace {
class PlaylistBackgroundWebContentsTag
: public content::WebContentsUserData<PlaylistBackgroundWebContentsTag> {
public:
PlaylistBackgroundWebContentsTag(const PlaylistBackgroundWebContentsTag&) =
delete;
PlaylistBackgroundWebContentsTag& operator=(
const PlaylistBackgroundWebContentsTag&) = delete;
~PlaylistBackgroundWebContentsTag() override = default;
private:
friend class content::WebContentsUserData<PlaylistBackgroundWebContentsTag>;
explicit PlaylistBackgroundWebContentsTag(content::WebContents* web_contents)
: content::WebContentsUserData<PlaylistBackgroundWebContentsTag>(
*web_contents) {}
WEB_CONTENTS_USER_DATA_KEY_DECL();
};
WEB_CONTENTS_USER_DATA_KEY_IMPL(PlaylistBackgroundWebContentsTag);
} // namespace
base::Value::Dict GetValueFromPlaylistItemInfo(const PlaylistItemInfo& info) {
base::Value::Dict playlist_value;
playlist_value.Set(kPlaylistItemIDKey, info.id);
@@ -53,12 +25,4 @@ base::Value::Dict GetValueFromPlaylistItemInfo(const PlaylistItemInfo& info) {
return playlist_value;
}
void MarkAsBackgroundWebContents(content::WebContents* web_contents) {
PlaylistBackgroundWebContentsTag::CreateForWebContents(web_contents);
}
bool IsBackgroundWebContents(content::WebContents* web_contents) {
return !!PlaylistBackgroundWebContentsTag::FromWebContents(web_contents);
}
} // namespace playlist
components/playlist/playlist_service_helper.h
-6
View File
@@ -10,17 +10,11 @@
#include "base/values.h"
namespace content {
class WebContents;
} // namespace content
namespace playlist {
struct PlaylistItemInfo;
base::Value::Dict GetValueFromPlaylistItemInfo(const PlaylistItemInfo& info);
void MarkAsBackgroundWebContents(content::WebContents* web_contents);
bool IsBackgroundWebContents(content::WebContents* web_contents);
} // namespace playlist