Diff with Chrome

[rsheeter]

Fixes #338

[anthrotype]

I had to apply the following changes to make it run on macOS.

"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" is a file, not a directory. Since it contains spaces I had to call shlex.quote before constructing the ninja rule (or should ninja take care of escaping spaces?).

Also, you want to pass relative paths to --lhs_dir and --rhs_dir flags in write_diffreport script, otherwise Chrome Mac doesn't find the PNGs.

diff --git a/src/nanoemoji/nanoemoji.py b/src/nanoemoji/nanoemoji.py
index a849e70..59ffa1a 100644
--- a/src/nanoemoji/nanoemoji.py
+++ b/src/nanoemoji/nanoemoji.py
@@ -41,6 +41,7 @@ import os
 from pathlib import Path
 import platform
 import re
+import shlex
 import shutil
 import subprocess
 import sys
@@ -257,8 +258,8 @@ def _chrome_command() -> str:
     cmd, validator = {
         "Linux": ("google-chrome", shutil.which),
         "Darwin": (
-            '"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome"',
-            lambda s: Path(s).is_dir(),
+            "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome",
+            lambda s: Path(s).is_file(),
         ),
         "Windows": ("chrome", shutil.which),
     }[platform.system()]
@@ -266,7 +267,7 @@ def _chrome_command() -> str:
     if not validator(cmd):
         raise ValueError(f"{cmd} failed validation")

-    return cmd
+    return shlex.quote(cmd)

 def write_config_preamble(nw, font_config: FontConfig):
@@ -311,7 +312,9 @@ def write_config_preamble(nw, font_config: FontConfig):
         module_rule(
             nw,
             "write_diffreport",
-            f"--lhs_dir {svg2png_dir()} --rhs_dir {font2png_dir()} --output_file $out @$out.rsp",
+            f"--lhs_dir {rel_build(svg2png_dir())} "
+            f"--rhs_dir {rel_build(font2png_dir())} "
+            "--output_file $out @$out.rsp",
             rspfile="$out.rsp",
             rspfile_content="$in",
         )

[rsheeter]

I had to apply the following changes to make it run on macOS.

Ty! Hadn't tried it on Mac yet :)

[rsheeter]

Alright, I think this works well enough to review now that I have, sigh, --enable-features=COLRV1Fonts

[anthrotype]

--enable-features=COLRV1Fonts

I think we don't actually want to test on Chrome < 98, which contains several bugs that were fixed later. Should we simply look for Chrome Canary instead, at least until it reaches stable?

[anthrotype]

If I use Canary (with the following patch), I am getting some glitchy rendering, both for the SVG->PNG (left hand side) and for the COLRv1=>PNG (right hand side). I have no idea why.

diff --git a/src/nanoemoji/nanoemoji.py b/src/nanoemoji/nanoemoji.py
index c35ca10..7e6bc08 100644
--- a/src/nanoemoji/nanoemoji.py
+++ b/src/nanoemoji/nanoemoji.py
@@ -256,12 +256,12 @@ def write_preamble(nw):
 @functools.lru_cache()
 def _chrome_command() -> str:
     cmd, validator = {
-        "Linux": ("google-chrome", shutil.which),
+        "Linux": ("google-chrome-canary", shutil.which),
         "Darwin": (
-            "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome",
+            "/Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary",
             lambda s: Path(s).is_file(),
         ),
-        "Windows": ("chrome", shutil.which),
+        "Windows": ("chrome-canary", shutil.which),
     }[platform.system()]

     if not validator(cmd):
@@ -299,7 +299,6 @@ def write_config_preamble(nw, font_config: FontConfig):
                 f"--window-size={res},{res}",
                 "--force-device-scale-factor=1",
                 "--virtual-time-budget=1000",
-                "--enable-features=COLRV1Fonts",  # unnecessary for 98+
                 "--screenshot=$out",
                 "$in",
             )

This is in Chrome stable (Version 97.0.4692.99 (Official Build) (x86_64)):

This in latest Canary (Version 100.0.4861.0 (Official Build) canary (x86_64))

[anthrotype]

I tried to use Chrome Beta (Version 98.0.4758.80 (Official Build) beta (x86_64)), and it works as expected, without the COLRv1 flag. Should we make the chrome executable configurable with CLI option?

[anthrotype]

or we just wait to merge this until chrome 98 stable actually rolls out to everyone, which is.. today apparently

[rsheeter]

Since 98 is going out nowish I'm ok just running the default for now