Clean up what we've declared "//visibility:public" in the build files.

README.md

@@ -159,7 +159,7 @@ Segmentation plans are in a [textproto format](https://protobuf.dev/reference/pr
 This repo currently provides a few experimental utilities that can generate segmentation plans for you. It is also
 possible to write plans by hand, or develop new utilities to generate plans.
 
-In this repo 3 options are currently provided:
+In this repo two options are currently provided:
 
 1. [Recommended] `util/closure_glyph_keyed_segmenter_util`: this utility uses a subsetting closure based approach
     to generate a glyph keyed segmentation plan (extension segments that augment glyph data). It can optionally
@@ -188,15 +188,6 @@ In this repo 3 options are currently provided:
       latin.txt cyrillic.txt greek.txt > table_keyed.txtpb
     ```
 
-3.  `util/iftb2config`: this utility converts a segmentation obtained from the
-    [binned incremental font transfer prototype](https://github.com/adobe/binned-ift-reference)
-    into and equivalent segmentation plan. Example execution:
-
-    ```sh
-    iftb -VV info my_iftb_font.ttf 2>&1 | \
-      bazel run util:iftb2config > segmentation_plan.txtpb
-    ```
-
 If separate glyph keyed and table keyed configs were generated using #1 and #2 they can then be combined into one
 complete plan by concatenating them:
 

check-public-headers.sh

@@ -0,0 +1,38 @@
+#!/bin/bash
+#
+# Checks that all headers referenced from public api headers are also
+# in the public set.
+
+public_hdrs=$(bazel query 'labels(hdrs, attr("visibility", ".*//visibility:public.*", kind("cc_.* rule", //...)))' 2>/dev/null | sed -e 's|^//||' -e 's|:|/|')
+echo "$public_hdrs" | sort > /tmp/public_hdrs.txt
+
+echo "Public header list:"
+cat /tmp/public_hdrs.txt
+echo ""
+
+exit_code=0
+
+if grep -q "^common/try\.h$" /tmp/public_hdrs.txt; then
+  echo "Error: common/try.h should not be in the public headers list."
+  exit_code=1
+fi
+
+for hdr in $public_hdrs; do
+  if [[ ! -f "$hdr" ]]; then
+    continue
+  fi
+
+  includes=$(grep -h -E '^[ \t]*#[ \t]*include[ \t]+"[^"]+"' "$hdr" | sed -E 's/.*"([^"]+)".*/\1/')
+
+  for inc in $includes; do
+    if [[ -f "$inc" ]]; then
+      if ! grep -q "^${inc}$" /tmp/public_hdrs.txt; then
+        echo "Error: Public header '$hdr' includes non-public header '$inc'"
+        exit_code=1
+      fi
+    fi
+  done
+done
+
+rm /tmp/public_hdrs.txt
+exit $exit_code

common/BUILD

@@ -12,7 +12,6 @@ cc_library(
         "brotli_binary_diff.cc",
         "brotli_binary_patch.cc",
         "compat_id.cc",
-        "compat_id.h",
         "file_font_provider.cc",
         "font_helper.cc",
         "hb_set_unique_ptr.cc",
@@ -37,7 +36,6 @@ cc_library(
         "hb_set_unique_ptr.h",
         "int_set.h",
         "sparse_bit_set.h",
-        "try.h",
         "woff2.h",
     ],
     visibility = [
@@ -55,9 +53,15 @@ cc_library(
         "@brotli//:brotlienc",
         "@harfbuzz",
         "@woff2",
+        ":try",
     ],
 )
 
+cc_library(
+    name = "try",
+    hdrs = ["try.h"],
+)
+
 cc_library(
     name = "mocks",
     srcs = [
@@ -71,7 +75,7 @@ cc_library(
         "mock_font_provider.h",
     ],
     visibility = [
-        "//visibility:public",
+
     ],
     deps = [
         ":common",
@@ -83,7 +87,9 @@ filegroup(
     name = "testdata",
     srcs = glob(["testdata/**"]),
     visibility = [
-        "//visibility:public",
+        "//ift:__subpackages__",
+        "//brotli:__pkg__",
+        "//util:__pkg__",
     ],
 )
 

ift/BUILD

@@ -5,12 +5,11 @@ cc_library(
     name = "ift",
     srcs = [
         "glyph_keyed_diff.cc",
-        "glyph_keyed_diff.h",
         "table_keyed_diff.cc",
-        "table_keyed_diff.h",
         "url_template.cc",
     ],
     hdrs = [
+        "glyph_keyed_diff.h",
         "table_keyed_diff.h",
         "url_template.h",
     ],
@@ -42,7 +41,7 @@ cc_library(
         "testdata/test_segments.h",
     ],
     visibility = [
-        "//visibility:public",
+        "//ift/encoder:__pkg__",
     ],
     deps = [
         "@abseil-cpp//absl/container:flat_hash_set",

ift/dep_graph/dependency_graph_test.cc

@@ -13,6 +13,9 @@
 
 #include "gtest/gtest.h"
 
+using ift::proto::PATCH;
+
+
 using absl::flat_hash_map;
 using common::hb_face_unique_ptr;
 using common::CodepointSet;

ift/encoder/BUILD

@@ -5,17 +5,17 @@ cc_library(
     name = "encoder",
     srcs = [
         "closure_glyph_segmenter.cc",
-        "closure_glyph_segmenter.h",
         "compiler.cc",
-        "compiler.h",
+    ],
+    hdrs = [
+      "closure_glyph_segmenter.h",
+      "compiler.h",
     ],
     visibility = [
-        "//ift:__subpackages__",
-        "//ift/client:__pkg__",
-        "//js_client:__pkg__",
-        "//util:__pkg__",
+        "//visibility:public",
     ],
     deps = [
+        ":merge_strategy",
         ":activation_condition",
         ":common",
         ":segmentation_context",
@@ -64,13 +64,9 @@ cc_library(
         "glyph_groupings.cc",
         "glyph_groupings.h",
         "invalidation_set.h",
-        "merge_strategy.cc",
-        "merge_strategy.h",
         "merger.cc",
         "merger.h",
         "patch_size_cache.h",
-        "requested_segmentation_information.cc",
-        "requested_segmentation_information.h",
         "segmentation_context.cc",
         "segmentation_context.h",
     ] + select({
@@ -81,9 +77,8 @@ cc_library(
             "dependency_closure_disabled.cc",
         ],
     }),
-    visibility = [
-    ],
     deps = [
+        ":merge_strategy",
         ":segmentation_info",
         ":activation_condition",
         ":common",
@@ -110,6 +105,7 @@ cc_library(
         "requested_segmentation_information.h",
         "glyph_closure_cache.cc",
         "glyph_closure_cache.h",
+        "init_subset_defaults.h",
     ],
     visibility = [
         "//ift/dep_graph:__pkg__",
@@ -124,22 +120,28 @@ cc_library(
     name = "activation_condition",
     srcs = [
         "activation_condition.cc",
-        "activation_condition.h",
         "glyph_segmentation.cc",
+    ],
+    hdrs = [
+        "activation_condition.h",
         "glyph_segmentation.h",
     ],
     deps = [
         ":common",
         "//ift/freq",
     ],
+    visibility = [
+        "//visibility:public",
+    ],
 )
 
 cc_library(
     name = "common",
     srcs = [
-        "init_subset_defaults.h",
-        "segment.h",
         "subset_definition.cc",
+    ],
+    hdrs = [
+        "segment.h",
         "subset_definition.h",
         "types.h",
     ],
@@ -157,6 +159,22 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "merge_strategy",
+    srcs = [
+        "merge_strategy.cc",
+    ],
+    hdrs = [
+        "merge_strategy.h",
+    ],
+    visibility = [
+        "//visibility:public",
+    ],
+    deps = [
+        "//ift/freq",
+    ],
+)
+
 cc_test(
     name = "compiler_test",
     size = "small",

ift/encoder/activation_condition.cc

@@ -11,6 +11,10 @@
 #include "ift/proto/patch_encoding.h"
 #include "ift/proto/patch_map.h"
 
+using ift::proto::ActivationConditionProto;
+using ift::proto::SegmentsProto;
+
+
 using absl::btree_set;
 using absl::flat_hash_map;
 using absl::Span;

ift/encoder/activation_condition.h

@@ -119,7 +119,7 @@ class ActivationCondition {
       const common::SegmentSet& merged_segments, const Segment& merged_segment,
       const ift::freq::ProbabilityCalculator& calculator) const;
 
-  ActivationConditionProto ToConfigProto() const;
+  ift::proto::ActivationConditionProto ToConfigProto() const;
 
   bool operator<(const ActivationCondition& other) const;
 

ift/encoder/candidate_merge_test.cc

@@ -15,6 +15,10 @@
 #include "ift/freq/probability_bound.h"
 #include "ift/freq/unicode_frequencies.h"
 
+using ift::proto::CLOSURE_ONLY;
+using ift::proto::PATCH;
+
+
 using common::CodepointSet;
 using common::FontData;
 using common::hb_face_unique_ptr;
@@ -123,7 +127,10 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas) {
 
   ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY);
   auto context =
-      segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
+      SegmentationContext::InitializeSegmentationContext(
+          roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(),
+          segmenter.condition_analysis_mode(), segmenter.brotli_quality(),
+          segmenter.init_font_merging_brotli_quality());
   ASSERT_TRUE(context.ok()) << context.status();
 
   Merger merger = *Merger::New(
@@ -189,7 +196,10 @@ TEST_F(CandidateMergeTest, AssessMerge_WithBestCandidate) {
 
   ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY);
   auto context =
-      segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
+      SegmentationContext::InitializeSegmentationContext(
+          roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(),
+          segmenter.condition_analysis_mode(), segmenter.brotli_quality(),
+          segmenter.init_font_merging_brotli_quality());
   ASSERT_TRUE(context.ok()) << context.status();
 
   Merger merger = *Merger::New(
@@ -254,7 +264,10 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas_Complex) {
 
   ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY);
   auto context =
-      segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
+      SegmentationContext::InitializeSegmentationContext(
+          roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(),
+          segmenter.condition_analysis_mode(), segmenter.brotli_quality(),
+          segmenter.init_font_merging_brotli_quality());
   ASSERT_TRUE(context.ok()) << context.status();
 
   Merger merger = *Merger::New(
@@ -306,7 +319,10 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas_Complex_ModifiedConditions) {
 
   ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY);
   auto context =
-      segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
+      SegmentationContext::InitializeSegmentationContext(
+          roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(),
+          segmenter.condition_analysis_mode(), segmenter.brotli_quality(),
+          segmenter.init_font_merging_brotli_quality());
   ASSERT_TRUE(context.ok()) << context.status();
 
   Merger merger = *Merger::New(
@@ -394,7 +410,10 @@ TEST_F(CandidateMergeTest, AssessPatchMerge) {
 
   ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY);
   auto context =
-      segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
+      SegmentationContext::InitializeSegmentationContext(
+          roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(),
+          segmenter.condition_analysis_mode(), segmenter.brotli_quality(),
+          segmenter.init_font_merging_brotli_quality());
   ASSERT_TRUE(context.ok()) << context.status();
 
   Merger merger = *Merger::New(
@@ -445,7 +464,10 @@ TEST_F(CandidateMergeTest, AssessPatchMerge_RequiresPatches) {
 
   ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY);
   auto context =
-      segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
+      SegmentationContext::InitializeSegmentationContext(
+          roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(),
+          segmenter.condition_analysis_mode(), segmenter.brotli_quality(),
+          segmenter.init_font_merging_brotli_quality());
   ASSERT_TRUE(context.ok()) << context.status();
 
   Merger merger = *Merger::New(