Require translation screenshot if none exists or message content changed

* The current translation screenshot presubmit check flags messages
  which got modified but for which no new translation screenshot
  got uploaded. It thereby also flags messages that got modified, but of
  which the message content itself did not change. This is the case, for
  example, if only the message description for translators got changed.
* This CL restricts the translation screenshot presubmit flagging
  modified messages. It will trigger 1) if the message did not have a
  screenshot at all so far, or 2) if the message content itself changed.
  Only changing the non-visible part of a message that already has a
  translation screenshot does not trigger it anymore.

Bug: 1103844
Change-Id: I254ce2a7267a7d2308b606b16d080c74188d582b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2328669
Commit-Queue: Rainhard Findling <rainhard@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797648}

PRESUBMIT.py

@@ -5099,6 +5099,11 @@ def CheckStrings(input_api, output_api):
         new_id_to_msg_map = grd_helper.GetGrdMessages(
           StringIO(unicode('\n'.join(f.NewContents()))), file_dir)
 
+    grd_name, ext = input_api.os_path.splitext(
+        input_api.os_path.basename(file_path))
+    screenshots_dir = input_api.os_path.join(
+        input_api.os_path.dirname(file_path), grd_name + ext.replace('.', '_'))
+
     # Compute added, removed and modified message IDs.
     old_ids = set(old_id_to_msg_map)
     new_ids = set(new_id_to_msg_map)
@@ -5108,12 +5113,16 @@ def CheckStrings(input_api, output_api):
     for key in old_ids.intersection(new_ids):
       if (old_id_to_msg_map[key].FormatXml()
           != new_id_to_msg_map[key].FormatXml()):
-        modified_ids.add(key)
-
-    grd_name, ext = input_api.os_path.splitext(
-        input_api.os_path.basename(file_path))
-    screenshots_dir = input_api.os_path.join(
-        input_api.os_path.dirname(file_path), grd_name + ext.replace('.', '_'))
+        sha1_path = input_api.os_path.join(
+          screenshots_dir, key + '.png.sha1')
+        if sha1_path not in new_or_added_paths and \
+           not input_api.os_path.exists(sha1_path):
+          # This message does not yet have a screenshot. Require one.
+          modified_ids.add(key)
+        elif (old_id_to_msg_map[key].ContentsAsXml('', True)
+          != new_id_to_msg_map[key].ContentsAsXml('', True)):
+          # The message content itself changed. Require an updated screenshot.
+          modified_ids.add(key)
 
     if run_screenshot_check:
       # Check the screenshot directory for .png files. Warn if there is any.

PRESUBMIT_test.py

@@ -2872,6 +2872,22 @@ class StringTest(unittest.TestCase):
       '</message>',
     '</grit-part>')
 
+  NEW_GRDP_CONTENTS3 = (
+    '<?xml version="1.0" encoding="utf-8"?>',
+      '<grit-part>',
+        '<message name="IDS_PART_TEST1" desc="Description with typo.">',
+          'Part string 1',
+        '</message>',
+    '</grit-part>')
+
+  NEW_GRDP_CONTENTS4 = (
+    '<?xml version="1.0" encoding="utf-8"?>',
+      '<grit-part>',
+        '<message name="IDS_PART_TEST1" desc="Description with typo fixed.">',
+          'Part string 1',
+        '</message>',
+    '</grit-part>')
+
   # A grdp file with one ICU syntax message without syntax errors.
   NEW_GRDP_CONTENTS_ICU_SYNTAX_OK1 = (
     '<?xml version="1.0" encoding="utf-8"?>',
@@ -2975,6 +2991,25 @@ class StringTest(unittest.TestCase):
         os.path.join('test_grd', 'IDS_TEST2.png.sha1'),
     ], warnings[0].items)
 
+  def testModifiedMessageDescription(self):
+    # CL modified a message description for a message that does not yet have a
+    # screenshot. Should warn.
+    input_api = self.makeInputApi([
+      MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS3,
+                       self.NEW_GRDP_CONTENTS4, action='M')])
+    warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
+    self.assertEqual(1, len(warnings))
+
+    # CL modified a message description for a message that already has a
+    # screenshot. Should not warn.
+    input_api = self.makeInputApi([
+      MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS3,
+                       self.NEW_GRDP_CONTENTS4, action='M'),
+      MockFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
+               'binary', action='A')])
+    warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
+    self.assertEqual(0, len(warnings))
+
   def testPngAddedSha1NotAdded(self):
     # CL added one new message in a grd file and added the png file associated
     # with it, but did not add the corresponding sha1 file. This should warn