Garden-o-matic: Introduce an option to open links in a new window

Tools/GardeningServer/scripts/ui.js

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 16 matching lines @@
 
 (function () {
 
 ui.displayURLForBuilder = function(builderName)
 {
     return config.waterfallURL + '?' + $.param({
         'builder': builderName
     });
 }
 
+ui.kUseNewWindowForLinksSetting = 'gardenomatic.use-new-window-for-links';
+
 ui.displayNameForBuilder = function(builderName)
 {
     return builderName.replace(/Webkit /, '');
 }
 
 ui.urlForTest = function(testName)
 {
     return 'http://trac.webkit.org/browser/trunk/LayoutTests/' + testName;
 }
 
@@ skipping 13 matching lines @@
     return ui.urlForFlakinessDashboard(opt_testNameList) + '&showChrome=false';
 }
 
 ui.rolloutReasonForTestNameList = function(testNameList)
 {
     return 'Broke:\n' + testNameList.map(function(testName) {
         return '* ' + testName;
     }).join('\n');
 }
 
+ui.setTargetForLink = function(anchor)
+{
+    if (!anchor.href.indexOf('#'))

[pdr., 2014/03/04 23:12:09]

Can you help me understand these checks for fragments?

[apavlov, 2014/03/05 00:23:56]

This is a catch-all check for the app navigation, which uses fragments (e.g. for
switching tabs, like "#unexpected"), as opposed to all other links (to crbugs,
commits, etc.) We don't want the app navigation links to open in new windows,
hence this check (if href starts with "#", then return immediately).

[alecflett, 2014/03/05 01:21:30]

I've found the test !foo.indexOf() as a constant source for confusion and future
bugs since indexOf() returns -1 for not found and "!" doesn't accurately convey
this to the reader.

how about if (anchor.href.indexOf('#') == 0)

[apavlov, 2014/03/05 06:56:21]

Hey Alec,

You are absolutely right. In our DevTools code, we have used explicit
comparisons to 0 for some time, as this is far more readable. I even wrote this
line that way, but then recollected the Blink rule :) Fixed.

+        return;
+    if (ui.useNewWindowForLinks)
+        anchor.target = '_blank';
+    else
+        anchor.removeAttribute('target');
+}
+
+ui.setUseNewWindowForLinks = function(enabled)

[apavlov, 2014/03/04 18:46:54]

Not sure how to organize this best location-wise. Thoughts?

[ojan, 2014/03/04 22:49:02]

Yeah...putting localStorage queries in the ui module seems a bit wrong to me. I
don't immediately have a better suggestion though.

[apavlov, 2014/03/05 00:23:56]

Could it be "base" instead? localStorage is unrelated to ui, even though this
particular setting is :)

[ojan, 2014/03/05 01:04:20]

I suppose you could add base.setUseNewWindowsForLinks that this code
ui.setUseNewWindowsForLinks calls. I'm not sure it's any better though. I
wouldn't bother.

[apavlov, 2014/03/05 06:56:21]

Okay, let's leave it as is.

+{
+    ui.useNewWindowForLinks = enabled;
+    if (enabled)

[apavlov, 2014/03/04 18:46:54]

Do we want to use localStorage? Or just go ahead with a non-persistent setting
(and make the "open in new window" the default)? One pro for persistence is that
there is a lesser chance to inadvertently open something instead of GoM,
especially when it is processing a zillion of revisions for long-standing
failures (I saw something on the order of 10 minutes today).

[ojan, 2014/03/04 22:49:02]

I think using localStorage makes sense. It's not a lot of code complexity and
saves a lot of user annoyance.

+        localStorage[ui.kUseNewWindowForLinksSetting] = 'true';
+    else
+        delete localStorage[ui.kUseNewWindowForLinksSetting];
+
+    var anchors = $('a').filter(function() {

[ojan, 2014/03/05 01:04:20]

Now that I look a bit more closely, you don't need this filter at all, do you
since setTargetForLink early returns?

[apavlov, 2014/03/05 06:56:21]

Correct, this is now not required.

+        return this.href.indexOf('#') !== 0;
+    });
+    $.each(anchors, function(index, anchor) {
+        ui.setTargetForLink(anchor);
+    });
+}
+ui.setUseNewWindowForLinks(!!localStorage[ui.kUseNewWindowForLinksSetting]);
+
+ui.createLinkNode = function(url, textContent)
+{
+    var link = document.createElement('a');
+    link.href = url;
+    ui.setTargetForLink(link);
+    link.appendChild(document.createTextNode(textContent));
+    return link;
+}
+
 ui.onebar = base.extends('div', {
     init: function()
     {
         this.id = 'onebar';
         this.innerHTML =
             '<ul>' +
                 '<li><a href="#unexpected">Unexpected Failures</a></li>' +
                 '<li><a href="#expected">Expected Failures</a></li>' +
                 '<li><a href="#results">Results</a></li>' +
             '</ul>' +
+            '<div id="link-handling"><input type="checkbox" id="new-window-for-links"><label for="new-window-for-links">Open links in new window</label></div>' +
             '<div id="unexpected"></div>' +
             '<div id="expected"></div>' +
             '<div id="results"></div>';
         this._tabNames = [
             'unexpected',
             'expected',
             'results',
         ]
 
         this._tabIndexToSavedScrollOffset = {};
         this._tabs = $(this).tabs({
             disabled: [2],
             show: function(event, ui) { this._restoreScrollOffset(ui.index); },
+            select: function(event, ui) {
+                this._saveScrollOffset();
+                window.location.hash = ui.tab.hash;
+            }.bind(this)
         });
     },
     _saveScrollOffset: function() {
         var tabIndex = this._tabs.tabs('option', 'selected');
         this._tabIndexToSavedScrollOffset[tabIndex] = document.body.scrollTop;
     },
     _restoreScrollOffset: function(tabIndex)
     {
         document.body.scrollTop = this._tabIndexToSavedScrollOffset[tabIndex] || 0;
     },
     _setupHistoryHandlers: function()
     {
         function currentHash() {
             var hash = window.location.hash;
             return (!hash || hash == '#') ? '#unexpected' : hash;
         }
 
         var self = this;
-        $('.ui-tabs-nav a').bind('mouseup', function(event) {

[apavlov, 2014/03/04 18:46:54]

Drive-by: fixed clicking the tabs. With this code in place, the disabled
"Results" tab would get enabled and activated when clicked. This is now handled
by the |select| function in the this._tabs initialization above.

-            var href = event.target.getAttribute('href');
-            var hash = currentHash();
-            if (href != hash) {
-                self._saveScrollOffset();
-                window.location = href
-            }
-        });
-
         window.onhashchange = function(event) {
             var tabName = currentHash().substring(1);
             self._selectInternal(tabName);
         };
 
         // When navigating from the browser chrome, we'll
         // scroll to the #tabname contents. popstate fires before
         // we scroll, so we can save the scroll offset first.
         window.onpopstate = function() {
             self._saveScrollOffset();
         };
     },
+    _setupLinkSettingHandler: function()
+    {
+        $('#new-window-for-links').attr('checked', ui.useNewWindowForLinks);
+        $('#new-window-for-links').change(function(event) {
+            ui.setUseNewWindowForLinks(this.checked);
+        });
+    },
     attach: function()
     {
         document.body.insertBefore(this, document.body.firstChild);
+        this._setupLinkSettingHandler();
         this._setupHistoryHandlers();
     },
     tabNamed: function(tabName)
     {
         if (this._tabNames.indexOf(tabName) == -1)
             return null;
         tab = document.getElementById(tabName);
         // We perform this sanity check below to make sure getElementById
         // hasn't given us a node in some other unrelated part of the document.
         // that shouldn't happen normally, but it could happen if an attacker
@@ skipping 143 matching lines @@
         theSpan.appendChild(document.createTextNode('Latest revision processed by every bot: '));
 
         var latestRevision = model.latestRevisionWithNoBuildersInFlight();
         var latestRevisions = model.latestRevisionByBuilder();
 
         // Get the list of builders sorted with the most recent one first.
         var builders = Object.keys(latestRevisions);
         builders.sort(function (a, b) { return parseInt(latestRevisions[b]) - parseInt(latestRevisions[a]);});
 
         var summaryNode = document.createElement('summary');
-        var summaryLinkNode = base.createLinkNode(trac.changesetURL(latestRevision), latestRevision);
+        var summaryLinkNode = ui.createLinkNode(trac.changesetURL(latestRevision), latestRevision);
         summaryNode.appendChild(summaryLinkNode);
 
         var revisionsTableNode = document.createElement('table');
         builders.forEach(function(builderName) {
             var trNode = document.createElement('tr');
 
             var tdNode = document.createElement('td');
-            tdNode.appendChild(base.createLinkNode(ui.displayURLForBuilder(builderName), builderName.replace('WebKit ', '')));
+            tdNode.appendChild(ui.createLinkNode(ui.displayURLForBuilder(builderName), builderName.replace('WebKit ', '')));
             trNode.appendChild(tdNode);
 
             var tdNode = document.createElement('td');
             tdNode.appendChild(document.createTextNode(latestRevisions[builderName]));
             trNode.appendChild(tdNode);
 
             revisionsTableNode.appendChild(trNode);
         });
 
         var revisionsNode = document.createElement('details');
@@ skipping 13 matching lines @@
             }
         });
         $(summaryLinkNode).mouseout(function(ev) {
             if (!revisionsNode.open) {
                 $(revisionsPopUp).removeClass("active");
             }
         });
 
         var totRevision = model.latestRevision();
         theSpan.appendChild(document.createTextNode(', trunk is at '));
-        theSpan.appendChild(base.createLinkNode(trac.changesetURL(totRevision), totRevision));
+        theSpan.appendChild(ui.createLinkNode(trac.changesetURL(totRevision), totRevision));
 
         checkout.lastBlinkRollRevision().then(function(revision) {
             theSpan.appendChild(document.createTextNode(', last roll is to '));
-            theSpan.appendChild(base.createLinkNode(trac.changesetURL(revision), revision));
+            theSpan.appendChild(ui.createLinkNode(trac.changesetURL(revision), revision));
         }, function() {});
 
         rollbot.fetchCurrentRoll().then(function(roll) {
             theSpan.appendChild(document.createTextNode(', current autoroll '));
             if (roll) {
                 var linkText = "" + roll.fromRevision + ":" + roll.toRevision;
-                theSpan.appendChild(base.createLinkNode(roll.url, linkText));
+                theSpan.appendChild(ui.createLinkNode(roll.url, linkText));
                 if (roll.isStopped)
                     theSpan.appendChild(document.createTextNode(' (STOPPED) '));
             } else {
                 theSpan.appendChild(document.createTextNode(' None'));
             }
         });
     }
 });
 
 })();