Use an opaque type for FrameTreeNode IDs, part 26

Various layering violation commentary.

Bug: 361344235, 364652019, 40686246, 40683815
Change-Id: I128ea94acbeb279c1f8ae75c30f02ccbeaac70fc
Include-Ci-Only-Tests: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5836091
Reviewed-by: Christine Hollingsworth <christinesm@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Owners-Override: Avi Drissman <avi@chromium.org>
Commit-Queue: Christine Hollingsworth <christinesm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1351526}

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h

@@ -267,11 +267,12 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker {
   const net::HttpRequestHeaders headers_;
   const int load_flags_;
   const bool has_user_gesture_;
-  // TODO(crbug.com/40683815): |weak_web_state_| is only used on iOS, and
-  // |web_contents_getter_|, |render_process_id_|, |render_frame_token_|, and
-  // |frame_tree_node_id_| are used on all other platforms.  This class should
-  // be refactored to use only the common functionality can be shared across
-  // platforms.
+  // TODO(https://crbug.com/40683815): |weak_web_state_| is only used on iOS,
+  // and |web_contents_getter_|, |render_process_id_|, |render_frame_token_|,
+  // and |frame_tree_node_id_| are used on all other platforms.  This class
+  // should be refactored to use only the common functionality can be shared
+  // across platforms. Note that this blocks the refactoring of
+  // components/security_interstitials, https://crbug.com/40686246.
   base::RepeatingCallback<content::WebContents*()> web_contents_getter_;
   const security_interstitials::UnsafeResource::RenderProcessId
       render_process_id_ =

components/security_interstitials/core/unsafe_resource.h

@@ -48,17 +48,23 @@ struct UnsafeResource {
 
   using UrlCheckCallback = base::RepeatingCallback<void(UrlCheckResult)>;
 
-  // TODO(crbug.com/40686246): These are content/ specific ids that need to be
-  // plumbed through this struct.
+  // TODO(https://crbug.com/40686246): These are content/ specific types that
+  // are used in this struct, in violation of layering. Refactor and remove
+  // them.
+  //
+  // TODO(https://crbug.com/40683815): Note that components/safe_browsing relies
+  // on this violation of layering to implement its own layering violation, so
+  // that issue might need to be fixed first.
+
   // Equivalent to GlobalRenderFrameHostId.
   using RenderProcessId = int;
   using RenderFrameToken = std::optional<base::UnguessableToken>;
-  // See RenderFrameHost::GetFrameTreeNodeId.
+  // This is the underlying value type of content::FrameTreeNodeId.
   using FrameTreeNodeId = int;
   // Copies of the sentinel values used in content/.
   // Equal to ChildProcessHost::kInvalidUniqueID.
   static constexpr RenderProcessId kNoRenderProcessId = -1;
-  // Equal to RenderFrameHost::kNoFrameTreeNodeId.
+  // Equal to the invalid value of content::FrameTreeNodeId.
   static constexpr FrameTreeNodeId kNoFrameTreeNodeId = -1;
 
   UnsafeResource();

storage/browser/file_system/file_system_request_info.h

@@ -20,6 +20,10 @@ struct COMPONENT_EXPORT(STORAGE_BROWSER) FileSystemRequestInfo {
   // The storage domain (always set).
   std::string storage_domain;
   // Set by the network service for use by callbacks.
+  // TODO(https://crbug.com/364652019): Do something about this. This is really
+  // a content::FrameTreeNodeId, but DEPS don't allow it to be correctly typed.
+  // This is used to smuggle a FrameTreeNodeId from content/ to chrome/ in
+  // violation of layering practices.
   int content_id = 0;
   // The original request blink::StorageKey (always set).
   blink::StorageKey storage_key;