Bug 74188 – Web Inspector: Generated HAR is missing pages.startedDateTime

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Libo Song]

The getHAR returns the HAR object, but the har.log.pages.startedDateTime is not valid. I also noticed that the title filed is empty. See the attached screen shot.

[Libo Song]

Created attachment 118596 [details]
Screen-shot to show the missing field.

attach the screen-shot again.

[Andrey Kosyakov]

This was originally caused by r101126: http://trac.webkit.org/changeset/101126

[Andrey Kosyakov]

Created attachment 119041 [details]
Patch

[Pavel Feldman]

Comment on attachment 119041 [details]
Patch

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

> LayoutTests/http/tests/inspector/inspector-test.js:117
> +    return 0 <= delta && delta < 30 * 1000 ? "<plausible>" : value;

We should not rely upon current time in the tests.

> LayoutTests/http/tests/inspector/inspector-test.js:130
> +        var formatter = nondeterministicProps && nondeterministicProps[prop];

It is hard for compiler to infer the type of this variable.

> LayoutTests/http/tests/inspector/inspector-test.js:133
> +            InspectorTest.addResult(prefixWithName + InspectorTest[formatter].call(InspectorTest, propValue));

Non-deterministic means that only the type should be printed.

> LayoutTests/http/tests/inspector/resources-test.js:12
> +    startedDateTime: "formatRecentTime",

This is a strange change to the non-deterministic properties. Should you test for instanceof Date instead?

[Andrey Kosyakov]

Comment on attachment 119041 [details]
Patch

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

>> LayoutTests/http/tests/inspector/inspector-test.js:117
>> +    return 0 <= delta && delta < 30 * 1000 ? "<plausible>" : value;
> 
> We should not rely upon current time in the tests.

We do not. This rather removes the dependency on current time. We compare the timestamp produced through the test execution against current time -- the difference between these should never exceed test timeout. Do we have a  better option for assuring sanity of time values?

>> LayoutTests/http/tests/inspector/inspector-test.js:130
>> +        var formatter = nondeterministicProps && nondeterministicProps[prop];
> 
> It is hard for compiler to infer the type of this variable.

We do not compile the tests. We use eval() all over the test suite, so I doubt it will be practical to make it compilable without major rework. I can change this to a ternary operator if you feel storngly about it.

>> LayoutTests/http/tests/inspector/inspector-test.js:133
>> +            InspectorTest.addResult(prefixWithName + InspectorTest[formatter].call(InspectorTest, propValue));
> 
> Non-deterministic means that only the type should be printed.

I would not regard it as a dogma: non-deterministic here means the value varies, but it does not mean it's completely random. If we can narrow the expected range of results down in some way, we should aim at doing this. The original regression that this patch fixes was masked exactly because we took "non-deterministic" too wide. So I'm introducing a generic way here to have "formatters" that will allow us to assure that variable values are within the reasonable limites. Other possible uses for this include time intervals (that must be within test execution time for the resource timings) and file: URLs that need to be trimmed to skip the root path, to pick a few.

>> LayoutTests/http/tests/inspector/resources-test.js:12
>> +    startedDateTime: "formatRecentTime",
> 
> This is a strange change to the non-deterministic properties. Should you test for instanceof Date instead?

Because the semantics of the field is better defined by its name rather than type. We may have quite a few dates within the objects that we dump, and only some of them should be checked to be within the test lifetime -- others may be in future, static or totally random.

[Pavel Feldman]

(In reply to comment #5)
> (From update of attachment 119041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119041&action=review
> 
> >> LayoutTests/http/tests/inspector/inspector-test.js:117
> >> +    return 0 <= delta && delta < 30 * 1000 ? "<plausible>" : value;
> > 
> > We should not rely upon current time in the tests.
> 
> We do not. This rather removes the dependency on current time. We compare the timestamp produced through the test execution against current time -- the difference between these should never exceed test timeout. Do we have a  better option for assuring sanity of time values?

This 30 seconds threshold is what I don't like. Since we don't have a way to mock Date here, we could simply check whether the year is like >2000 or something similar.

> 
> >> LayoutTests/http/tests/inspector/inspector-test.js:130
> >> +        var formatter = nondeterministicProps && nondeterministicProps[prop];
> > 
> > It is hard for compiler to infer the type of this variable.
> 
> We do not compile the tests. We use eval() all over the test suite, so I doubt it will be practical to make it compilable without major rework. I can change this to a ternary operator if you feel storngly about it.
> 

Yeah, you are right. Still, using the same slot as a flag and a formatter does not sound right.

[Andrey Kosyakov]

Created attachment 119236 [details]
Patch

[Andrey Kosyakov]

I renamed nondeterministicProperties to customFormatters and added formatAsTypeName that provides "old" behavior.

WRT using current time, a search for Date.now() would reveal quite a few places where this is being done. I think checking delta time is much better than a check against a constant date -- we do a bit of arithmetic over the date values and could end up with the date in the future, for example. I'm bumping the tolerance to 30m, though, to provide additional guard against things like NTP clock adjustments and debugging.

[Pavel Feldman]

Comment on attachment 119236 [details]
Patch

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

> LayoutTests/http/tests/inspector/inspector-test.js:112
> +InspectorTest.formatAsTypeName = function(value)

I don't think firnatters should be defined on the InspectorTest object - use a nested object / namespace instead.

> LayoutTests/http/tests/inspector/inspector-test.js:135
> +        if (customFormatters && typeof customFormatters[prop] === "string") {

Why do we have this distinction now that there is a default formatting?

[Andrey Kosyakov]

Committed r102916: <http://trac.webkit.org/changeset/102916>

[Andrey Kosyakov]

(In reply to comment #9)
> I don't think firnatters should be defined on the InspectorTest object - use a nested object / namespace instead.

done.

> 
> > LayoutTests/http/tests/inspector/inspector-test.js:135
> > +        if (customFormatters && typeof customFormatters[prop] === "string") {
> Why do we have this distinction now that there is a default formatting?

yep, this is redundant now -- dropped.