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

Tools/GardeningServer/scripts/ui.js

Index: Tools/GardeningServer/scripts/ui.js
diff --git a/Tools/GardeningServer/scripts/ui.js b/Tools/GardeningServer/scripts/ui.js
index 600e09ddbf9350967ec76ec3300d9f78f6c04a8e..ec28a4ea7c8783323a7a1eccb98586138fb9b602 100644
--- a/Tools/GardeningServer/scripts/ui.js
+++ b/Tools/GardeningServer/scripts/ui.js
@@ -34,6 +34,8 @@ ui.displayURLForBuilder = function(builderName)
     });
 }
 
+ui.kUseNewWindowForLinksSetting = 'gardenomatic.use-new-window-for-links';
+
 ui.displayNameForBuilder = function(builderName)
 {
     return builderName.replace(/Webkit /, '');
@@ -67,6 +69,42 @@ ui.rolloutReasonForTestNameList = function(testNameList)
     }).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()
     {
@@ -77,6 +115,7 @@ ui.onebar = base.extends('div', {
                 '<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>';
@@ -90,6 +129,10 @@ ui.onebar = base.extends('div', {
         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() {
@@ -108,15 +151,6 @@ ui.onebar = base.extends('div', {
         }
 
         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);
@@ -129,9 +163,17 @@ ui.onebar = base.extends('div', {
             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)
@@ -295,7 +337,7 @@ ui.revisionDetails = base.extends('span', {
         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');
@@ -303,7 +345,7 @@ ui.revisionDetails = base.extends('span', {
             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');
@@ -337,18 +379,18 @@ ui.revisionDetails = base.extends('span', {
 
         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 {