Handles default revision ranges.

dashboard/dashboard/speed_releasing.py

Index: dashboard/dashboard/speed_releasing.py
diff --git a/dashboard/dashboard/speed_releasing.py b/dashboard/dashboard/speed_releasing.py
index a58abfa363fbcb57e64edb05b3cba3b804c009d5..f6a1fc67b8da706612a1914825a59040b57449d1 100644
--- a/dashboard/dashboard/speed_releasing.py
+++ b/dashboard/dashboard/speed_releasing.py
@@ -11,8 +11,33 @@ from google.appengine.ext import ndb
 
 from dashboard.common import request_handler
 from dashboard.common import utils
+from dashboard.models import graph_data
 from dashboard.models import table_config
 
+CLANK_MILESTONES = {

[sullivan, 2017/02/08 22:19:53]

Can you add a high-level comment explaining what these are?

And it looks like we will just have one set of hard-coded milestones for all
release pages?

+    56: 1479546161,
+    57: 1485025126,
+}
+
+CHROUMIUM_MILESTONES = {

[sullivan, 2017/02/08 22:19:53]

Nit: CHROMIUM_MILESTONES

+    44: 330231,
+    45: 338390,
+    46: 344925,
+    47: 352221,
+    48: 359700,
+    49: 370022,
+    50: 378097,
+    51: 388729,
+    52: 403382,
+    53: 413836,
+    54: 416640,
+    55: 433391,
+    56: 433400,
+    57: 445288,
+}
+
+CURRENT_MILESTONE = max(CHROUMIUM_MILESTONES.keys())
+
 
 class SpeedReleasingHandler(request_handler.RequestHandler):
   """Request handler for requests for speed releasing page."""
@@ -46,16 +71,16 @@ class SpeedReleasingHandler(request_handler.RequestHandler):
     values = {}
     self.GetDynamicVariables(values)
 
-    master_bot_pairs = _GetMasterBotPairs(table_entity.bots)
-
+    master_bot_pairs, masters = _GetMasterBotPairs(table_entity.bots)

[sullivan, 2017/02/08 22:19:53]

It's confusing to change the return value of this function, and then not update
its name. But then what would the new name be,
"_GetMasterBotPairsAndMasterList"? I think that's a sign that the return value
of the function should be simpler. What about:

master_bot_pairs = _GetMasterBotPairs(table_entity.bots)
masters = set([m.split('/')[0] for m in master_bot_pairs])

If master_bot_pairs instead returned a list of tuples, it could be:
masters = set([m[0] for m in master_bot_pairs])

     rev_a = self.request.get('revA')
     rev_b = self.request.get('revB')
     if not rev_a or not rev_b:
-      self.response.out.write(json.dumps({'error': 'Invalid revisions.'}))
-      return
+      rev_a, rev_b = _GetDefaultRev(rev_a, rev_b, masters, master_bot_pairs,
+                                    table_entity.tests)
     rev_a, rev_b = _CheckRevisions(rev_a, rev_b)
-    revisions = [rev_a, rev_b]
-
+    display_a = _GetDisplayRev(master_bot_pairs, table_entity.tests, rev_a)
+    display_b = _GetDisplayRev(master_bot_pairs, table_entity.tests, rev_b)
+    revisions = [rev_b, rev_a] # In reverse intentionally.
     self.response.out.write(json.dumps({
         'xsrf_token': values['xsrf_token'],
         'table_bots': master_bot_pairs,
@@ -67,6 +92,7 @@ class SpeedReleasingHandler(request_handler.RequestHandler):
         'units': _GetTestToUnitsMap(master_bot_pairs, table_entity.tests),
         'revisions': revisions,
         'categories': _GetCategoryCounts(json.loads(table_entity.table_layout)),
+        'display_revisions': [display_b, display_a] # Similar to revisions.
     }))
 
   def _OutputHomePageJSON(self):
@@ -82,9 +108,11 @@ class SpeedReleasingHandler(request_handler.RequestHandler):
 
 def _GetMasterBotPairs(bots):
   master_bot_pairs = []
+  masters = []
   for bot in bots:
     master_bot_pairs.append(bot.parent().string_id() + '/' + bot.string_id())
-  return master_bot_pairs
+    masters.append(bot.parent().string_id())
+  return master_bot_pairs, masters
 
 def _GetRowValues(revisions, bots, tests):
   """Builds a nested dict organizing values by rev/bot/test.
@@ -135,18 +163,82 @@ def _GetRow(bot, test, rev):
   test_path = bot + '/' + test
   test_key = utils.TestKey(test_path)
   row_key = utils.GetRowKey(test_key, rev)
-  if row_key.get():
-    return row_key.get().value
+  row = row_key.get()
+  if row:
+    return row.value
   else:
     return 0
 
+def _GetMilestone(masters, num_from_end=0):

[sullivan, 2017/02/08 22:19:53]

This is only ever called with num_from_end=0, why not just change it to
_GetLastMilestone()?

Also this is getting the revision of the last milestone, not the milestone. The
naming is confusing with _GetNewestRev below, it returns a revision and so does
this.

+  """Returns the specified milestone.
+
+  The last milestone is 0 from the end. The 2nd most recent milestone is 1
+  from the end.
+  """
+  end_position = CURRENT_MILESTONE - num_from_end
+  if 'ClankInternal' in masters:
+    return CLANK_MILESTONES[end_position]
+  else:
+    return CHROUMIUM_MILESTONES[end_position]
+
+def _GetNewestRev(bot, test, masters):
+  if bot and test:
+    test_path = bot[0] + '/' + test[0]
+    test_key = utils.TestKey(test_path)
+    query = graph_data.Row.query()
+    query = query.filter(
+        graph_data.Row.parent_test == utils.OldStyleTestKey(test_key))
+    query = query.order(-graph_data.Row.revision)
+    row = query.fetch(limit=1)
+    if len(row):
+      return row[0].revision
+  return _GetMilestone(masters, 0) # Return most recent milestone.
+
+def _GetMilestoneRevAfter(rev, masters, bots, tests):
+  """If only one Rev is given, defaults to Rev - beginning of next milestone."""
+  if 'ClankInternal' in masters:
+    milestone_dict = CLANK_MILESTONES
+  else:
+    milestone_dict = CHROUMIUM_MILESTONES
+  #pylint: disable=unused-variable
+  for key, value in milestone_dict.iteritems():

[sullivan, 2017/02/08 22:19:53]

To get rid of the pylint warning:

for _, value

+    if value > int(rev):
+      return value
+  return _GetNewestRev(bots, tests, masters)
+
+def _GetDisplayRev(bots, tests, rev):
+  """Creates a user friendly commit position to display.
+
+  For V8 and ChromiumPerf masters, this will just be the passed in rev.
+  """
+  if bots and tests:
+    test_path = bots[0] + '/' + tests[0]
+    test_key = utils.TestKey(test_path)
+    row_key = utils.GetRowKey(test_key, rev)
+    row = row_key.get()
+    if row and hasattr(row, 'r_commit_pos'): # Rule out masters like V8
+      if rev != row.r_commit_pos: # Rule out ChromiumPerf
+        if hasattr(row, 'a_default_rev') and hasattr(row, row.a_default_rev):
+          return row.r_commit_pos + '-' + getattr(row, row.a_default_rev)[:3]
+  return rev
+
+def _GetDefaultRev(rev_a, rev_b, masters, bots, tests):

[sullivan, 2017/02/08 22:19:53]

This function should be named better, something like
GetMilestoneRevisionsForRevs? I am actually not 100% sure what it's supposed to
do, and why we snap revisions to milestones from the url parameters. Wouldn't we
want to just grab the data between the revisions exactly if they are specified?
And use milestones if milestones are specified?

+  """If one/both of the revisions are None, change accordingly.
+
+  If both are None, return most recent milestone, present.
+  If one is None, return the other, present.
+  """
+  if not rev_a and not rev_b:
+    return  _GetMilestone(masters, 0), _GetNewestRev(bots, tests, masters)
+  return ((rev_a or rev_b),
+          _GetMilestoneRevAfter((rev_a or rev_b), masters, bots, tests))
+
 def _CheckRevisions(rev_a, rev_b):
-  """Checks to ensure the revisions are valid."""
+  """Checks to ensure the revisions are in the correct order."""
   rev_a = int(rev_a)
   rev_b = int(rev_b)
   if rev_b < rev_a:
     rev_a, rev_b = rev_b, rev_a
-  # TODO(jessimb): Check if r_commit_pos (if clank), if so return revision.
   return rev_a, rev_b
 
 def _GetCategoryCounts(layout):