[Webkit-unassigned] [Bug 46301] [Web Timing] Implement dom* timing marks

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46301





--- Comment #3 from Tony Gentilcore <tonyg at chromium.org>  2010-10-04 12:05:26 PST ---
(From update of attachment 69560)
View in context: https://bugs.webkit.org/attachment.cgi?id=69560&action=review

Nice patch!

A few very minor comments. Again, I can't r+ this, so I'm adding abarth.

> LayoutTests/fast/dom/script-tests/webtiming-document-open.js:4
> +var originalTiming = [];

s/[]/{}/

> LayoutTests/fast/dom/script-tests/webtiming-document-open.js:21
> +    shouldBe("timing.navigationStart", "originalTiming.navigationStart");

What do you think about writing this test as a loop so that if new members are added they are automatically tested.

For example:
for (var prop in timing) {
  shouldBe("timing." + prop, "originalTiming." + prop);
}

> LayoutTests/fast/dom/script-tests/webtiming.js:41
> +    shouldBeGreaterThanOrEqual("timing.domLoading", "timing.responseStart");

Perhaps we should use fetchStart instead of responseStart. It is marked in WebKit so it will be accurate here. responseStart is reported by the platform, so I believe it is always zero in the test. Ditto for other responseStarts.

> LayoutTests/fast/dom/script-tests/webtiming.js:77
> +    shouldBeGreaterThanOrEqual("timing.domInteractive", "timing.responseEnd");

We should have a test that verifies at the time we run deferred scripts, both domLoading and domInteractive are set, but domContentLoaded and domComplete are not.

> LayoutTests/fast/dom/script-tests/webtiming.js:78
> +    shouldBeGreaterThanOrEqual("timing.domContentLoaded", "timing.domInteractive");

We should have a test that verifies that at the time a slow async script executes, domLoading, domInteractive, and domContentLoaded are all set, but domComplete is not.

> LayoutTests/fast/dom/webtiming-document-open-expected.txt:1
> +This test verifies that the NavigationTimers don't change after a document.open().

s/NavigationTimers/NavigationTimings/

> WebCore/WebCore.gypi:1178
> +            'dom/DocumentTiming.h',

Unfortunately adding a file in WebCore isn't this easy. This would break other ports.

Here's an example of a patch that modifies the build files necessary to add a new header file:
http://trac.webkit.org/changeset/64051

> WebCore/dom/DocumentTiming.h:31
> +struct DocumentTiming {

I'm not sure if this should subclass FastAllocBase. Sent an email to webkit-dev:
https://lists.webkit.org/pipermail/webkit-dev/2010-October/014603.html

> WebCore/dom/DocumentTiming.h:40
> +    double loadingOccurred;

What do you think about just using the names from the spec? eg. s/loadingOccurred/domLoading/

> WebCore/page/Timing.h:42
> +struct DocumentTiming;

struct forward declarations get alphabetized along with the classes. So this should be below DocumentLoader.

> WebCore/page/Timing.idl:51
> +        readonly attribute unsigned long long domComplete;

This should require an update to fast/dom/Window/window-properties-performance-expected.txt. You may have been running the tests on a port that skips that test.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.