content/DEPS - Issue 2946113002: Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode.

Unified Diff: content/DEPS

Issue 2946113002: Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. (Closed)

Patch Set: Doh... need to also change s/Default/Unspecified/ in chrome_content_browser_client.cc Created 3 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« chrome/browser/chrome_content_browser_client.cc ('K') | « chrome/browser/ui/extensions/hosted_app_browsertest.cc ('k') | content/browser/frame_host/render_frame_host_manager.cc » ('j') | content/browser/frame_host/render_frame_host_manager.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/DEPS

diff --git a/content/DEPS b/content/DEPS

index fd8706dfecc9fad191b4f32bd76a9b6bb603c0f7..71c63055b6b65c5bfa0997ce723c6c8e8dd64528 100644

--- a/content/DEPS

+++ b/content/DEPS

@@ -25,6 +25,7 @@ include_rules = [

# shared with iOS. This includes, but isn't limited to, browser features such

# as autofill or extensions, and chrome implementation details such as

# settings, packaging details, installation or crash reporting.

+ "+components/variations",

Alexei Svitkine (slow) 2017/07/07 16:55:05 I do wonder if we shouldn't perhaps restrict this

Łukasz Anforowicz 2017/07/07 20:41:42 Good point. I've made changes that I think should

Good point. I've made changes that I think should address this concern. I've included a long set of notes below, but after making the changes I've made, I am not sure if they are that interesting. These are the includes I see after the CL: $ git grep include.*components.variations -- content/ content/browser/memory/memory_condition_observer.cc:#include "components/variations/variations_associated_data.h" content/browser/renderer_host/render_view_host_impl.cc:#include "components/variations/variations_associated_data.h" content/ppapi_plugin/ppapi_thread.h:#include "components/variations/child_process_field_trial_syncer.h" content/public/test/test_utils.cc:#include "components/variations/variations_switches.h" content/renderer/render_thread_impl.h:#include "components/variations/child_process_field_trial_syncer.h" I see the following groups of related functionality: - components/variations/child_process_field_trial_syncer.h (variations::ChildProcessFieldTrialSyncer) - For now I moved it back to content/renderer/DEPS and content/ppapi_plugin/DEPS and made it specific to this particular header. - This is a field in both content::RenderThreadImpl and content::PpapiThread. - Maybe this field should be moved to content::ChildThreadImpl? - Such move would help hide/encapsulate the dependency on components/variations/child_process_field_trial_syncer.h - This might require moving field_trial_syncer_.InitFieldTrialObserving(...) call from RenderThreadImpl::Init to the constructor of ChildThreadImpl. - This might require having ChildThreadImpl's constructor pass base::CommandLine::ForCurrentProcess as an argument for field_trial_syncer_.InitFieldTrialObserving (like RenderThreadImpl::Init does today; a little bit unlike PpapiThread does today [although ultimately the value of MainFunctionParams::command_line also comes from base::CommandLine::ForCurrentProcess - called in ContentMainRunnerImpl::Run] - I am not sure what to do about RenderThreadImpl::SetFieldTrialGroup. Maybe this can also move to ChildThreadImpl. - I wonder if not having SetFieldTrialGroup on PpapiThread might indicate a bug in propagation of field trial parameters to the ppapi process. - components/variations/variations_associated_data.h (GetVariations... functions) - This seems unused in memory_condition_observer.cc I've removed the include here. - This seems removable in render_view_host_impl.cc. OTOH, https://crbug.com/681160 is fixed and there is no active bug to track the removal... FWIW, I opened a new bug and moved this to "specific_include_rules" in content/browser/renderer_host/DEPS. - components/variations/variations_switches.h - This is the addition in this CL needed for testing. - As you suggested I am trying to use VariationsParamManager instead. - I've added a narrow dependency from //content's browser tests and test utils.

Alexei Svitkine (slow) 2017/07/10 18:46:24 That SGTM for now. Maybe we can file a crbug for t

That SGTM for now. Maybe we can file a crbug for the suggestions you have below?

That SGTM. My mental model is we can add that include for now and it can be removed over time.

Łukasz Anforowicz 2017/07/10 22:17:39 Done - https://crbug.com/740726.

Done - https://crbug.com/740726.

Actually, I was able to already do it after rebasing - servolk@ has already kindly fixed https://crbug.com/740174 :-)

"+crypto",

"+grit/blink_resources.h",