[Webkit-unassigned] [Bug 54181] Implement Page Visibility API.

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Tony Gentilcore <tonyg at chromium.org>  2011-04-26 04:25:40 PST ---
(From update of attachment 90455)
View in context: https://bugs.webkit.org/attachment.cgi?id=90455&action=review

> LayoutTests/fast/events/page-visibility-iframe-test.html:15
> +function log(string) {

Rather than reimplementing these in tests, it would be better to include ../js/resources/js-test-pre.js and use debug() if you just want to output lines. If possible, it is even better to use shouldBe*() methods.

> LayoutTests/fast/events/page-visibility-iframe-test.html:30
> +    layoutTestController.dumpAsText();

You can also remove this once you include js-test-pre.

Also, usually there is a description() call (or just inline text) which has a sentence about the goal of the test.

> LayoutTests/fast/events/page-visibility-transition-test.html:35
> +//  1`- hidden (should fire event).

Stray tick after the 1 (here and elsewhere)

> LayoutTests/fast/events/page-visibility-transition-test.html:50
> +            log("PASS 1: hidden");

Nowhere is webkitIsVisible() tested. All the existing tests could be updated to test both properties. Something along the lines of:

shouldBe("document.webkitVisibilityState()", "hidden");
shouldBeFalse("document.webkitIsVisible()");

> Source/WebCore/dom/Document.cpp:1338
> +    // with the frame, we will consider the document to be invisible.

This comment should answer "why" rather than just restating the condition below.

> Source/WebCore/dom/Document.h:381
> +    PageVisibilityState webkitVisibilityStateEnum() const;

This should be private, and I'd recommend just renaming it to visibilityState() or something.

> Source/WebCore/page/Page.cpp:955
> +void Page::setInitialVisibilityState(PageVisibilityState visibilityState)

This is never used. Is that in a subsequent patch?

> Source/WebKit/chromium/features.gypi:78
> +        'ENABLE_PAGE_VISIBILITY_API=1',

Have you run all the tests? I'm surprised that the new DOM API doesn't cause an existing test to fail. There are a some tests which just iterate through all the properties on window, etc.

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