Bug 190440 – WebKit Inspector: Expose Server Timing Response Headers in Network Tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

Expose Server Timing Response Headers in WebKit Inspector

Created attachment 351984 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351984&action=review

This is great! Don't be alarmed by all of my comments, they are just on style and minor nits. Rebaseline this, give it tests and include a screenshot and it should be good to review!

> Source/WebInspectorUI/UserInterface/Models/Resource.js:600
> +            this._serverTimingEntries =
> +                WI.ServerTimingEntry.parseHeaders(this._responseHeaders.valueForCaseInsensitiveKey("Server-Timing"));

Style: Keep this one line. We don't worry about line width in these cases its much much easier to read this as a single line.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:27
> +WI.ServerTimingEntry = class ServerTimingEntry

We will definitely want to include layout tests for the parsing here. I'd recommend:

    LayoutTests/inspector/unit-tests/server-timing-entry.js

You can model it after something like string-utilities.js. A sync test that tests the static methods. Something that looks like:

    function test()
    {
        let suite = InspectorTest.createSyncSuite("ServerTimingEntry");

        suite.addTestCase({
            name: "ServerTimingEntry.parseHeaders",
            description: "Parsing of Server-Timing headers."
            test() {
                InspectorTest.expectThat(...);
                ...
                return true;
            }
        });

        suite.runTestCasesAndFinish();
    }

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:40
> +    static parseHeaders(valueString = "") {

Style: Put method opening/closing braces on their own line. So this would be:
https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide

    static parseHeaders(valueString = "")
    {
        ...
    }

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:43
> +        function trimLeadingWhiteSpace() {
> +            valueString = valueString.replace(/^\s*/, "");
> +        }

We have String.prototype.trimStart which would do same thing.

    valueString = valueString.trimStart();

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:47
> +            if (valueString.charAt(0) !== char) {

Style: No braces on a single line conditionals:
https://webkit.org/code-style-guidelines/#braces

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:54
> +        function consumeToken() {

Style: Put a newline between these inline functions to make them read as their own paragraph / concrete block of code.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:80
> +                // split into two parts:
> +                //  -everything before the first " or \
> +                //  -everything else

Style: For comments we prefer complete sentences with capitals and periods if applicable.
https://webkit.org/code-style-guidelines/#comments

So something like:

    // Split into two parts:
    //   - Everything before the first " or \
    //   - Everything else

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:87
> +                if (result[2].charAt(0) === "\"") {
> +                    // we have found our closing "
> +                    valueString = result[2].substring(1);  // strip off everything after the closing "
> +                    return value;                          // we are done here
> +                }

Style: For comments we prefer complete sentences and annotations above the line instead of on the same line. So something like:

    // Found the closing quote.
    if (result[2].charAt(0) === "\"") {
        // Strip off everything after the closing quote.
        ...
    }

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:98
> +            const result = /([,;].*)/.exec(valueString);

Style: We tend to use `let` for results of computations and `const` for anything that you would traditionally think of as a "compile time constant". So all of these `const` would become `let`.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:141
> +        switch (paramName) {
> +            case "dur":

Style: Case indentation should match the switch:
https://webkit.org/code-style-guidelines/#indentation-case-label

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:169
> +    set duration(duration) {

Style: Braces on their own line for these multi-line getters as well. Include a newline between them as well (compare to other Model classes).

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:108
> +        if (typeof duration !== "undefined") {

Nit: Since we don't need to worry about backwards compatibility this can be:

    if (duration !== undefined) {
        ...
    }

I'm assuming this is to allow zero as a valid value. If not, then you could just do:

    if (duration) {
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:113
> +            block.classList.add("block", "response"); // TODO (cvazac) color

Style: Avoid "TODO" and instead have a FIXME near by that is more descriptive, or includes a bugzilla link.

For example:

    // FIXME: Provide unique colors for the different ServerTiming rows based on the label.

Or:

    // FIXME: <https://webkit.org/b/123456> Give ServerTiming entries colors

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:119
> +            timeCell.textContent = WI.UIString("%.2fms").format(duration);

Style: We should prefer `Number.secondsToMillisecondsString(duration)` over the "%.2fms" formatting unless there is an explicit reason this needs to be different.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:166
> +            var emptyCell = this._appendEmptyRow().appendChild(document.createElement("td"));

Style: `let` where possible (everywhere expect case blocks).

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:175
> +            let maxDuration = 0;
> +            serverTiming.forEach(({duration = 0}) => {
> +                maxDuration = Math.max(maxDuration, duration);
> +            });

Style: We tend to prefer a for..of loop for loops of this nature, maybe even Array.prototype.reduce.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:177
> +            serverTiming.forEach(({name, duration, description}) => {

Style: Same, a for..of loop would read a bit easier.

Created attachment 351989 [details]
screenshot

In the left column, we show the description (falling back to name). The right column is the duration (if specified) in ms.

Created attachment 351992 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351992&action=review

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:176
> +            this._appendHeaderRow(WI.UIString("Server Timing"));

In the screenshot, each of the others have a colon. So I'd expect:

    WI.UIString("Server Timing:")

Created attachment 352052 [details]
Patch

Thx for the review! This patch (https://bugs.webkit.org/attachment.cgi?id=352052&action=diff) addresses your feedback. 

Remaining TODOs:
- tests
- colors

[Joseph Pecoraro]

Comment on attachment 352052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352052&action=review

Nice! This parsing looks great to me. I provided a few feedback comments but nothing serious.

r- only because it just needs tests.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:40
> +    static parseHeaders(valueString = "")

Lets put this somewhere nearby:

    // https://w3c.github.io/server-timing/#the-server-timing-header-field

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:74
> +        function consumeQuotedString() {

I found this pretty confusing to follow with the splits. This appears to be doing 2 things:

    1. Loop along a string to find a trailing DQUOTE that was not escaped.
    2. Unescaping characters preceded by an escape character.

I think it would be clearer written as a loop that does those two things:

    function consumeQuotedString() {
        // https://tools.ietf.org/html/rfc7230#section-3.2.6
        console.assert(valueString.charAt(0) === "\"");

        // Consume the leading quote.
        valueString = valueString.substring(1);

        let unescapedValueString = "";
        for (let i = 0; i < valueString.length; ++i) {
            let char = valueString.charAt(i);
            switch (char) {
            case "\\":
                // Escaped character.
                ++i;
                if (i < valueString.length)
                    break;
                unescapedValueString += valueString.charAt(i);
                break;
            case "\"":
                // Trailing quote.
                valueString = valueString.substring(i + 1);
                return unescapedValueString;
            default:
                unescapedValueString += char;
                break;
            }
        }

        // No trailing quote found. Consume the entire string to complete parsing.
        valueString = "";
        return null;
    }

The efficiency of this is a little worse, but negligibly so for the strings we are likely to have.  I find this much easier to follow. What do you think?

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:94
> +                console.assert(result[2].charAt(0) === "\\");

As written, this would assert if given an invalid string since result[2] could be the empty string. For example:

    valueString = '"te\\"st';
    consumeQuotedString();

In the second loop value string is "st" which would result in, result[2] being the empty string. This would behave just fine it would just inadvertently assert.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:135
> +                if (parseParameter) {
> +                    // paramName is valid.
> +                    parseParameter.call(this, entry, paramValue);
> +                }

It could be that this is a new parameter we don't know about yet, but I like that this code is trying to be future proof/compatible. Also the Function.prototype.call is unnecessary here since we don't use `this`. How about:

    if (parseParameter)
        parseParameter(entry, paramValue);
    else
        console.warn("Unknown Server-Timing parameter:", paramName, paramValue)

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:146
> +    static getParserForParameter(paramName) {

Lets make this another inline function inside of parseHeaders. It doesn't seem to warrant being another static function.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:149
> +            return function(entry, paramValue) {

Nit: I'd probably swap these parameters to be `function(value, entry)`. Since this primarily handles the value, and optionally updates the entry.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:189
> +

Style: Remove this extraneous line.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:180
> +            this._appendHeaderRow(WI.UIString("Server Timing"));

Nit: Add a colon to this UIString => "Server Timing:"

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:185
> +            for (let entry of serverTiming) {
> +              let {name, duration, description} = entry;

Style: Indentation should be 4 spaces.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:191
> +

Style: Remove this extraneous line.

[Joseph Pecoraro]

Comment on attachment 352052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352052&action=review

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:178
> +            let emptyCell = this._appendEmptyRow().appendChild(document.createElement("td"));
> +            emptyCell.colSpan = 3;
> +            emptyCell.appendChild(document.createElement("hr"));

Oh, and it might be worth pulling this out into a helper. How about `this._appendDividerRow()`?

Comment on attachment 351984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351984&action=review

>> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:108
>> +        if (typeof duration !== "undefined") {
> 
> Nit: Since we don't need to worry about backwards compatibility this can be:
> 
>     if (duration !== undefined) {
>         ...
>     }
> 
> I'm assuming this is to allow zero as a valid value. If not, then you could just do:
> 
>     if (duration) {
>         ...
>     }

That's right, we need to support zeroes, I added a comment.

Created attachment 352123 [details]
Patch

Comment on attachment 352052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352052&action=review

>> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:135
>> +                }
> 
> It could be that this is a new parameter we don't know about yet, but I like that this code is trying to be future proof/compatible. Also the Function.prototype.call is unnecessary here since we don't use `this`. How about:
> 
>     if (parseParameter)
>         parseParameter(entry, paramValue);
>     else
>         console.warn("Unknown Server-Timing parameter:", paramName, paramValue)

AKAICT, `console.warn`s from here don't make it to the actual browser console, only to the command line. Is that what you want?

[Joseph Pecoraro]

(In reply to cvazac from comment #12)
> Comment on attachment 352052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352052&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:135
> >> +                }
> > 
> > It could be that this is a new parameter we don't know about yet, but I like that this code is trying to be future proof/compatible. Also the Function.prototype.call is unnecessary here since we don't use `this`. How about:
> > 
> >     if (parseParameter)
> >         parseParameter(entry, paramValue);
> >     else
> >         console.warn("Unknown Server-Timing parameter:", paramName, paramValue)
> 
> AKAICT, `console.warn`s from here don't make it to the actual browser
> console, only to the command line. Is that what you want?

Yep that is what we want. The web content is not necessarily in error, it may be making use of a feature the frontend is not yet aware of. The console.warn can go to the system console or a inspector², which will be great because those of us developing on it might see it and then decide to add support for a possibly new Server-Timing feature.

It may also be possible that we could turn this into a warning indicator on the Server-Timing row in the popover. That could have a tooltip or some description that says:

    ⚠️ Unknown Parameter: 'dur'

Which would be useful to the developer to show they mis-typed 'dur' as 'duration' and that is why it is not exposed in the tools.

Created attachment 352135 [details]
Patch

[EWS Watchlist]

Comment on attachment 352135 [details]
Patch

Attachment 352135 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9547126

New failing tests:
inspector/unit-tests/server-timing-entry.html

[EWS Watchlist]

Created attachment 352138 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352135 [details]
Patch

Attachment 352135 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9547157

New failing tests:
inspector/unit-tests/server-timing-entry.html

[EWS Watchlist]

Created attachment 352141 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352135 [details]
Patch

Attachment 352135 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9547332

New failing tests:
inspector/unit-tests/server-timing-entry.html

[EWS Watchlist]

Created attachment 352143 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

Created attachment 352169 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 352169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352169&action=review

Awesome tests! Just rebaseline the results (the console.warn output shows up in it)

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:87
> +                    // Backslash character found, ignore it

Style: Comments are complete sentences that end in a period.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:89
> +                    if (i < valueString.length)

Style: Given the comment inside of this there should be braces.

> LayoutTests/inspector/unit-tests/server-timing-entry-expected.txt:221
> +Testing response header: -->metric;desc=\"\<--
> +PASS: Parsed ServerTimingEntry count ?== expected results count.
> +PASS: expectEqual("metric", "metric")
> +PASS: expectEqual(undefined, undefined)
> +PASS: expectEqual("", "")

These tests are great but the output is a bit unfortunate!

> LayoutTests/inspector/unit-tests/server-timing-entry.html:20
> +                InspectorTest.expectEqual(results.length, expectedResults.length,
> +                    `Parsed ServerTimingEntry count ?== expected results count.`);

This could provide the expected results count and it might be easier to read immediately. For example:

    PASS: Parsed ServerTimingEntry count has expected results count of 0.
    PASS: Parsed ServerTimingEntry count has expected results count of 1.
    PASS: Parsed ServerTimingEntry count has expected results count of 2.

Instead of all having ?== and no known value.

> LayoutTests/inspector/unit-tests/server-timing-entry.html:31
> +                    InspectorTest.expectEqual(serverTimingEntry.name, expectedResult.name);
> +                    InspectorTest.expectEqual(serverTimingEntry.duration, expectedResult.dur);
> +                    InspectorTest.expectEqual(serverTimingEntry.description, expectedResult.desc);

If you add a third parameter the output will be better. Something like:

    InspectorTest.expectEqual(serverTimingEntry.name, expectedResult.name, `name is ${JSON.stringify(expectedResult.name)}`);
    InspectorTest.expectEqual(serverTimingEntry.duration, expectedResult.dur, `duration is ${JSON.stringify(expectedResult.dur)}`);
    InspectorTest.expectEqual(serverTimingEntry.description, expectedResult.desc, `description is ${JSON.stringify(expectedResult.desc)}`);

Should produce something slightly easier to read then things like "equals(42, 42)" above.

> LayoutTests/inspector/unit-tests/server-timing-entry.html:50
> +            testServerTimingHeader("metric;desc=\"description\"", [{"name":"metric","desc":"description"}]);

It may be easier for you to use template strings so that you don't have to escape double quotes all the time:

    testServerTimingHeader(`metric;desc="description"`, [{"name":"metric","desc":"description"}]);

This has the added bonus of syntax highlighting the test strings slightly differently if your text editor supports template strings =).

> LayoutTests/inspector/unit-tests/server-timing-entry.html:91
> +            testServerTimingHeader("metric;desc=\\\\", [{"name":"metric","desc":""}]);

Should the expected description here be two slashes?

Comment on attachment 352169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352169&action=review

>> LayoutTests/inspector/unit-tests/server-timing-entry.html:91
>> +            testServerTimingHeader("metric;desc=\\\\", [{"name":"metric","desc":""}]);
> 
> Should the expected description here be two slashes?

Were it double-quoted, yes. But the \ character isn't valid as a token, because it's not a valid tchar.
Relevant spec bits from https://tools.ietf.org/html/rfc7230:
   token = 1*tchar
   tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
    "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

Created attachment 352214 [details]
Patch

Sites in the wild with server-timing entries:
https://www.bbc.co.uk/iplayer
https://ma.ttias.be/server-timings-chrome-devtools/
https://www.scalemates.com/
https://www.kohls.com/

My test site:
https://server-timing.netlify.com/

[Joseph Pecoraro]

Comment on attachment 352214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review

r=me!

> Source/WebInspectorUI/ChangeLog:9
> +        * UserInterface/Main.html:

You should regenerate the ChangeLog, this now has edits to new files like Test.html.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:184
> +            this._appendHeaderRow(WI.UIString("Server Timing"));

This lost the colon. Should we add it back? I only suggested it because it matches the other header rows.

> LayoutTests/inspector/unit-tests/server-timing-entry.html:63
> +            // spaces
> +            testServerTimingHeader(`metric ; `, [{"name":"metric"}]);

Maybe have a test for leading whitespace. Seems like it would be stripped via `consumeToken()` and work just fine.

[Joseph Pecoraro]

(In reply to cvazac from comment #23)
> Comment on attachment 352169 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352169&action=review
> 
> >> LayoutTests/inspector/unit-tests/server-timing-entry.html:91
> >> +            testServerTimingHeader("metric;desc=\\\\", [{"name":"metric","desc":""}]);
> > 
> > Should the expected description here be two slashes?
> 
> Were it double-quoted, yes. But the \ character isn't valid as a token,
> because it's not a valid tchar.
> Relevant spec bits from https://tools.ietf.org/html/rfc7230:
>    token = 1*tchar
>    tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
>     "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

Oh nice, that makes sense. Thanks!

[Devin Rousso]

Comment on attachment 352214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review

This is awesome!  One thing to note is that you're missing a ChangeLog for the LayoutTests changes :(

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:180
> +        if (!this._durationSet) {

Is it necessary to prevent modification of this value once it's set?  Typically, our style would be to `console.assert` for this, as it's more of a programmer's responsibility to control this.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:188
> +        if (!this._descriptionSet) {

Ditto (180).

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:140
> +        let {

I would prefer if you separated this onto two lines.  The reason we destructured `timingData` is because it's just a plain object, whereas `timingData` is an actual getter.  I don't think we should be destructuring our model objects.

    let {startTime, redirectStart, redirectEnd, fetchStart, domainLookupStart, domainLookupEnd, connectStart, connectEnd, secureConnectionStart, requestStart, responseStart, responseEnd} = this._resource.timingData;
    let serverTiming = this._resource.serverTiming;

Comment on attachment 352214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review

>> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:180
>> +        if (!this._durationSet) {
> 
> Is it necessary to prevent modification of this value once it's set?  Typically, our style would be to `console.assert` for this, as it's more of a programmer's responsibility to control this.

Per spec (https://w3c.github.io/server-timing/) "first-man" in wins, relevant bits:
To avoid any possible ambiguity, individual server-timing-param-names SHOULD NOT appear multiple times within a server-timing-metric. If any server-timing-param-name is specified more than once, only the first instance is to be considered, even if the server-timing-param is incomplete or invalid. All subsequent occurrences MUST be ignored without signaling an error or otherwise altering the processing of the server-timing-metric. This is the only case in which the ordering of parameters within a server-timing-metric is considered to be significant.

[Devin Rousso]

Comment on attachment 352214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review

>>> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:180
>>> +        if (!this._durationSet) {
>> 
>> Is it necessary to prevent modification of this value once it's set?  Typically, our style would be to `console.assert` for this, as it's more of a programmer's responsibility to control this.
> 
> Per spec (https://w3c.github.io/server-timing/) "first-man" in wins, relevant bits:
> To avoid any possible ambiguity, individual server-timing-param-names SHOULD NOT appear multiple times within a server-timing-metric. If any server-timing-param-name is specified more than once, only the first instance is to be considered, even if the server-timing-param is incomplete or invalid. All subsequent occurrences MUST be ignored without signaling an error or otherwise altering the processing of the server-timing-metric. This is the only case in which the ordering of parameters within a server-timing-metric is considered to be significant.

In that case, use an early return:

set duration(duration)
{
    if (this._duration !== undefined)
        return;

    this._duration = duration;
}

You might also want to synthesize a console error to warn developers that "hey you included the same name twice".

Created attachment 352243 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 352243 [details]
Patch

r=me!

Do you need me to commit-queue this?

[WebKit Commit Bot]

Comment on attachment 352243 [details]
Patch

Clearing flags on attachment: 352243

Committed r237151: <https://trac.webkit.org/changeset/237151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45288297>