Ensure a good experience for ARES-6 error reporting
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jun 2017 05:55:28 +0000 (05:55 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jun 2017 05:55:28 +0000 (05:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171699

Reviewed by Filip Pizlo and Jon Davis.

This patch fixes a bug where we would silently fail running ARES-6. The bug
was that we were calling reportError with the wrong |this| value.
I also cleaned up a bit of the code around error reporting. We
now indicate which test failed, and update the status to reflect
that a failure happened.

This patch also modifies the CSS a bit to work better on smaller
screened devices. The CSS prevents the status from having a line
break both when an error is reported and when we're running the
benchmark.

* ARES-6/driver.js:
(Driver):
(Driver.prototype.reportError):
* ARES-6/results.js:
(Results.prototype.reportError):
(Results):
* ARES-6/styles.css:
(.start):
(#status):
(.failed):
(#status.failed):
(.test .failed:before):
(#magic):
(@media only screen and (max-width: 784px)):
(.test):
(p):
(@media only screen and (max-width: 320px)):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@217701 268f45cc-cd09-0410-ab3c-d52691b4dbfc

PerformanceTests/ARES-6/driver.js
PerformanceTests/ARES-6/results.js
PerformanceTests/ARES-6/styles.css
PerformanceTests/ChangeLog
Websites/browserbench.org/ARES-6/driver.js
Websites/browserbench.org/ARES-6/results.js
Websites/browserbench.org/ARES-6/styles.css
Websites/browserbench.org/ChangeLog

index 2639409..2ef7d63 100644 (file)
@@ -34,7 +34,6 @@ class Driver {
         this._magicCell = magicCell;
         this._summary = new Stats(summaryCell, "summary");
         this._key = key;
-        this._hadErrors = false;
         if (isInBrowser)
             window[key] = this;
     }
@@ -83,10 +82,16 @@ class Driver {
     
     reportError(...args)
     {
+        if (isInBrowser)
+            console.log("Error encountered: ", ...args);
+
         this._benchmarks.get(this._benchmark).reportError(...args);
-        this._recomputeSummary();
-        this._hadErrors = true;
-        this._iterate();
+
+        if (isInBrowser) {
+            this._statusCell.innerHTML = "Test failure \u2014 error in console";
+            this._statusCell.classList.add("failed");
+        } else
+            print("Test failure");
     }
     
     _recomputeSummary()
@@ -137,11 +142,10 @@ class Driver {
         if (!this._benchmark) {
             if (!this._numIterations) {
                 if (isInBrowser) {
-                    this._statusCell.innerHTML =
-                        (this._hadErrors ? "Failures encountered!" : "Restart");
+                    this._statusCell.innerHTML = "Restart";
                     this.readyTrigger();
                 } else
-                    print(this._hadErrors ? "Failures encountered!" : "Success! Benchmark is now finished.");
+                    print("Success! Benchmark is now finished.");
                 return;
             }
             this._numIterations--;
@@ -165,7 +169,7 @@ class Driver {
                 magicFrame.contentDocument.open();
                 magicFrame.contentDocument.write(
                     `<!DOCTYPE html><head><title>benchmark payload</title></head><body><script>` +
-                    `window.onerror = top.${this._key}.reportError;\n` +
+                    `window.onerror = top.${this._key}.reportError.bind(top.${this._key});\n` +
                     `function reportResult()\n` +
                     `{\n` +
                     `    var driver = top.${this._key};\n` +
index 5edaf87..28563cd 100644 (file)
@@ -66,10 +66,11 @@ class Results {
     
     reportError(message, url, lineNumber)
     {
-        for (let subResult of Results.subResults)
-            this[subResult].reportResult(Stats.error);
-        if (isInBrowser)
-            this._benchmark.cells.message.innerHTML = url + ":" + lineNumber + ": " + message;
+        if (isInBrowser) {
+            this._benchmark.cells.message.classList.remove('running');
+            this._benchmark.cells.message.classList.add('failed');
+        } else
+            print("Failed running benchmark");
     }
 }
 
index 8b09b8a..2c9910f 100644 (file)
@@ -91,7 +91,6 @@ p {
     background-color: transparent;
     border: 5px solid #E7B135;
     font-size: 2.4rem;
-    line-height: 5.4rem;
     font-weight: 600;
     text-transform: uppercase;
     color: #E7B135;
@@ -159,6 +158,22 @@ p {
     color: #ffffff;
 }
 
+#status {
+    line-height: 5.4rem;
+}
+
+.failed {
+    color: #ff5744;
+}
+
+#status.failed {
+    font-size: 1.5rem;
+}
+
+.test .failed:before {
+    content: '\2716';
+}
+
 .test .running:before {
     content: '\25b8';
 }
@@ -249,6 +264,10 @@ p {
     border-left: 0 solid transparent;
 }
 
+#magic {
+    opacity: 0;
+}
+
 @keyframes fade-in {
   0%   { opacity: 0; }
   100% { opacity: 1; }
@@ -284,10 +303,35 @@ p {
     .start {
         width: 100%;
     }
-
     .test {
         flex: none;
-
         width: 100%;
     }
+    #status {
+        line-height: 4.3rem;
+    }
+    .start {
+        font-size: 2.1rem;
+    }
+    #status.failed {
+        font-size: 1.4rem;
+    }
+    p {
+        font-size: 1.6rem;
+    }
+}
+
+@media only screen and (max-width: 320px) {
+    #status {
+        line-height: 3.8rem;
+    }
+    .start {
+        font-size: 1.9rem;
+    }
+    #status.failed {
+        font-size: 1.1rem;
+    }
+    p {
+        font-size: 1.4rem;
+    }
 }
index 91baae2..df16b8a 100644 (file)
@@ -1,3 +1,39 @@
+2017-06-01  Saam Barati  <sbarati@apple.com>
+
+        Ensure a good experience for ARES-6 error reporting
+        https://bugs.webkit.org/show_bug.cgi?id=171699
+
+        Reviewed by Filip Pizlo and Jon Davis.
+
+        This patch fixes a bug where we would silently fail running ARES-6. The bug
+        was that we were calling reportError with the wrong |this| value.
+        I also cleaned up a bit of the code around error reporting. We
+        now indicate which test failed, and update the status to reflect
+        that a failure happened.
+        
+        This patch also modifies the CSS a bit to work better on smaller
+        screened devices. The CSS prevents the status from having a line
+        break both when an error is reported and when we're running the 
+        benchmark.
+
+        * ARES-6/driver.js:
+        (Driver):
+        (Driver.prototype.reportError):
+        * ARES-6/results.js:
+        (Results.prototype.reportError):
+        (Results):
+        * ARES-6/styles.css:
+        (.start):
+        (#status):
+        (.failed):
+        (#status.failed):
+        (.test .failed:before):
+        (#magic):
+        (@media only screen and (max-width: 784px)):
+        (.test):
+        (p):
+        (@media only screen and (max-width: 320px)):
+
 2017-05-19  Ryosuke Niwa  <rniwa@webkit.org>
 
         REGRESSION(r217118): Speedometer 2.0: Flight.js test is broken
index 2639409..2ef7d63 100644 (file)
@@ -34,7 +34,6 @@ class Driver {
         this._magicCell = magicCell;
         this._summary = new Stats(summaryCell, "summary");
         this._key = key;
-        this._hadErrors = false;
         if (isInBrowser)
             window[key] = this;
     }
@@ -83,10 +82,16 @@ class Driver {
     
     reportError(...args)
     {
+        if (isInBrowser)
+            console.log("Error encountered: ", ...args);
+
         this._benchmarks.get(this._benchmark).reportError(...args);
-        this._recomputeSummary();
-        this._hadErrors = true;
-        this._iterate();
+
+        if (isInBrowser) {
+            this._statusCell.innerHTML = "Test failure \u2014 error in console";
+            this._statusCell.classList.add("failed");
+        } else
+            print("Test failure");
     }
     
     _recomputeSummary()
@@ -137,11 +142,10 @@ class Driver {
         if (!this._benchmark) {
             if (!this._numIterations) {
                 if (isInBrowser) {
-                    this._statusCell.innerHTML =
-                        (this._hadErrors ? "Failures encountered!" : "Restart");
+                    this._statusCell.innerHTML = "Restart";
                     this.readyTrigger();
                 } else
-                    print(this._hadErrors ? "Failures encountered!" : "Success! Benchmark is now finished.");
+                    print("Success! Benchmark is now finished.");
                 return;
             }
             this._numIterations--;
@@ -165,7 +169,7 @@ class Driver {
                 magicFrame.contentDocument.open();
                 magicFrame.contentDocument.write(
                     `<!DOCTYPE html><head><title>benchmark payload</title></head><body><script>` +
-                    `window.onerror = top.${this._key}.reportError;\n` +
+                    `window.onerror = top.${this._key}.reportError.bind(top.${this._key});\n` +
                     `function reportResult()\n` +
                     `{\n` +
                     `    var driver = top.${this._key};\n` +
index 5edaf87..28563cd 100644 (file)
@@ -66,10 +66,11 @@ class Results {
     
     reportError(message, url, lineNumber)
     {
-        for (let subResult of Results.subResults)
-            this[subResult].reportResult(Stats.error);
-        if (isInBrowser)
-            this._benchmark.cells.message.innerHTML = url + ":" + lineNumber + ": " + message;
+        if (isInBrowser) {
+            this._benchmark.cells.message.classList.remove('running');
+            this._benchmark.cells.message.classList.add('failed');
+        } else
+            print("Failed running benchmark");
     }
 }
 
index 8b09b8a..2c9910f 100644 (file)
@@ -91,7 +91,6 @@ p {
     background-color: transparent;
     border: 5px solid #E7B135;
     font-size: 2.4rem;
-    line-height: 5.4rem;
     font-weight: 600;
     text-transform: uppercase;
     color: #E7B135;
@@ -159,6 +158,22 @@ p {
     color: #ffffff;
 }
 
+#status {
+    line-height: 5.4rem;
+}
+
+.failed {
+    color: #ff5744;
+}
+
+#status.failed {
+    font-size: 1.5rem;
+}
+
+.test .failed:before {
+    content: '\2716';
+}
+
 .test .running:before {
     content: '\25b8';
 }
@@ -249,6 +264,10 @@ p {
     border-left: 0 solid transparent;
 }
 
+#magic {
+    opacity: 0;
+}
+
 @keyframes fade-in {
   0%   { opacity: 0; }
   100% { opacity: 1; }
@@ -284,10 +303,35 @@ p {
     .start {
         width: 100%;
     }
-
     .test {
         flex: none;
-
         width: 100%;
     }
+    #status {
+        line-height: 4.3rem;
+    }
+    .start {
+        font-size: 2.1rem;
+    }
+    #status.failed {
+        font-size: 1.4rem;
+    }
+    p {
+        font-size: 1.6rem;
+    }
+}
+
+@media only screen and (max-width: 320px) {
+    #status {
+        line-height: 3.8rem;
+    }
+    .start {
+        font-size: 1.9rem;
+    }
+    #status.failed {
+        font-size: 1.1rem;
+    }
+    p {
+        font-size: 1.4rem;
+    }
 }
index 86faad9..f739b62 100644 (file)
@@ -1,3 +1,39 @@
+2017-06-01  Saam Barati  <sbarati@apple.com>
+
+        Ensure a good experience for ARES-6 error reporting
+        https://bugs.webkit.org/show_bug.cgi?id=171699
+
+        Reviewed by Filip Pizlo and Jon Davis.
+
+        This patch fixes a bug where we would silently fail running ARES-6. The bug
+        was that we were calling reportError with the wrong |this| value.
+        I also cleaned up a bit of the code around error reporting. We
+        now indicate which test failed, and update the status to reflect
+        that a failure happened.
+        
+        This patch also modifies the CSS a bit to work better on smaller
+        screened devices. The CSS prevents the status from having a line
+        break both when an error is reported and when we're running the 
+        benchmark.
+
+        * ARES-6/driver.js:
+        (Driver):
+        (Driver.prototype.reportError):
+        * ARES-6/results.js:
+        (Results.prototype.reportError):
+        (Results):
+        * ARES-6/styles.css:
+        (.start):
+        (#status):
+        (.failed):
+        (#status.failed):
+        (.test .failed:before):
+        (#magic):
+        (@media only screen and (max-width: 784px)):
+        (.test):
+        (p):
+        (@media only screen and (max-width: 320px)):
+
 2017-05-19  Ryosuke Niwa  <rniwa@webkit.org>
 
         REGRESSION(r217118): Speedometer 2.0: Flight.js test is broken