Implement functionality to cc relevant owners.

third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
index 5e43fd1fadd0a88a907116f68af3efdc160fc045..f89e115761231214153384ee8a9f72d0113e02f7 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
@@ -5,6 +5,7 @@
 """Pull latest revisions of a W3C test repo and make a local commit."""
 
 import argparse
+import json
 import time
 
 from webkitpy.common.webkit_finder import WebKitFinder
@@ -28,6 +29,8 @@ class DepsUpdater(object):
         self.keep_w3c_repos_around = False
         self.target = None
         self.auto_update = False
+        self.directory_dict = {}
+        self.email_list = []

[dcampb, 2016/07/27 20:54:57]

Why not assign these two to variables and return them in the script? 
Is there a benefit from making attributes??

[qyearsley, 2016/07/27 21:38:57]

I agree, if these attributes were removed and the functions that set them
(parse_imported_test_changes and parse_directory_owners) were instead changed to
return the desired value, then that might overall be easier to understand.

In general, the fewer attributes a class has, the easier it is to understand.
Every attribute represents some state which could potentially be modified by any
other method on the class; so, whenever you reason about what the value of an
attribute might be, you might have to consider that maybe it might be modified
somewhere else.

Whereas, if you have functions that just take an input and have an output and
have no side effects, then it may be easier to understand.

 
     def main(self, argv=None):
         self.parse_args(argv)
@@ -65,8 +68,13 @@ class DepsUpdater(object):
         self.commit_changes_if_needed(chromium_commitish, import_commitish)
         if self.auto_update:
             try_bots = self.host.builders.all_try_builder_names()
+            with open('directory_owners.json') as data_file:

[qyearsley, 2016/07/27 21:38:57]

Would this work if the current working directory is not .../webkitpy/w3c/?

We probably want to use self.finder (a WebKitFinder instance) to get the path to
the file:

  data_file_path = self.finder.path_from_webkit_base(
      'Tools', 'Scripts', 'webkitpy', 'w3c', 'directory_owners.json')

+                self.parse_directory_owners(data_file)
+            self.print_('## Attempting to CC relevant parties.')

[dcampb, 2016/07/27 20:54:57]

Just a comment change:

'Gathering directory owners email to CC'.. or something similar. 

+            _, out = self.run(['git', 'diff', 'master', '--name-only'])
+            self.parse_imported_test_changes(out, self.directory_dict)
             self.print_('## Uploading change list.')
-            self.check_run(['git', 'cl', 'upload', '-f', '-m', 'W3C auto test importer'])
+            self.check_run(['git', 'cl', 'upload', '-f', '-m', 'W3C auto test importer'] + self.email_list)
             self.print_('## Triggering try jobs.')
             for try_bot in try_bots:
                 self.run(['git', 'cl', 'try', '-b', try_bot])
@@ -88,7 +96,7 @@ class DepsUpdater(object):
                 self.check_run(['git', 'commit', '-a', '-m', '\'Modified Test Expectations from W3C Test Auto-roller\''])
                 self.check_run(['git', 'cl', 'upload', '-m', '\'Wrote lines to TestExpectations\''])
             else:
-                self.print_('No Failures, committing patch.')
+                self.print_('No Failures, landing patch.')
             quit()
             self.run(['git', 'cl', 'land', '-f'])

[qyearsley, 2016/07/27 21:38:57]

Somewhat separate from this CL:

If possible, it would be nice to extract the body of this if-clause (everything
under `if self.auto_update`) to a separate method. It might also be nice to
extract other sub-functions.

We should consider doing this refactoring in a separate CL.

         return 0
@@ -297,6 +305,21 @@ class DepsUpdater(object):
                 sets[result_type].add(line.split()[0])
         return sets
 
+    def parse_imported_test_changes(self, results, directory_dict):
+        folders_changed = set()
+        for line in results.splitlines():
+            line = line.strip('third_party/WebKit/LayoutTests/')

[qyearsley, 2016/07/27 21:38:56]

I wonder if there's a way that we can avoid having a literal path here.

Some tools we can use:

  self.fs.relpath(path, start): returns the relative path to |path| from start
  self.finder.layout_tests_dir(): Returns the absolute path (I think) to the
layout test dir
  self.finder.chromium_base(): Returns the absolute path (I think) to the
chromium base dir

So maybe something like:

  layout_tests_relative_path = self.fs.relpath(self.finder.layout_tests_dir(),
self.finder.chromium_base())
  test_path = self.fs.relpath(line, layout_tests_relative_path)

But there may be a simpler way.

+            directory_sep = line.rfind('/')
+            line = line[:directory_sep]

[qyearsley, 2016/07/27 21:38:56]

This looks like it may be the same as:

  line = self.fs.dirname(line)

You may want to change this to use a new variable name:

  test_directory = self.fs.dirname(line)

+            if line in directory_dict and line not in folders_changed:
+                self.email_list.append('--cc=' + directory_dict[line])
+                folders_changed.add(line)

[dcampb, 2016/07/27 20:54:57]

So I'm guessing this function:

1. Looks for the last '/' in the directory name and takes from 'imported..../'
as the directory
2. Searches for that string in the directory_dict and if the directory hasn't
already been added, it adds it to the email list.

What happens if the directory has no notifications email set to it. Will it try
and CC an empty string?

folders_changed --> directories
line --> directory_path

+
+    def parse_directory_owners(self, data_file):
+        data = json.load(data_file)
+        for dict_set in data:

[dcampb, 2016/07/27 20:54:57]

dict_set --> directory_dictionary

+            self.directory_dict[dict_set['directory']] = dict_set['notification-email']
+
     def check_run(self, command):
         return_code, out = self.run(command)
         if return_code: