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

[bugzilla-daemon at webkit.org]

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





--- Comment #21 from Tony Gentilcore <tonyg at chromium.org>  2011-05-02 03:04:24 PST ---
(From update of attachment 91388)
View in context: https://bugs.webkit.org/attachment.cgi?id=91388&action=review

> LayoutTests/fast/events/page-visibility-iframe-test.html:10
> +if (window.accessibilityController)

Is the accessibilityController guard here and in the other test intentional?

> LayoutTests/fast/events/page-visibility-iframe-test.html:48
> +        debug("Too many visibility transitions");  

Looks like a testFailed() would be better than debug().

> LayoutTests/fast/events/page-visibility-iframe-test.html:51
> +    testPassed("TEST COMPLETE");

This and the notifyDone() call below shouldn't be necessary. You should just need a "var successfullyParsed = true" at the end of the script and call finishJSTest(). Some good background here:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

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

Still a stray character after the "1"

> LayoutTests/fast/events/page-visibility-transition-test.html:62
> +    }

Should this have a testFailed for >3 changes like the other test?

> LayoutTests/fast/events/resources/page-visibility-iframe-test-subframe.html:35
> +    }

All this work in the subframe makes the test harder to understand and leads to code duplication. Is it possible to structure the test so that the subframe is just a generic empty page and the parent always reaches down through the contentDocument? e.g.

<script>
function startTest()
{
    frames[0].contentDocument.addEventListener("webkitvisibilitystatechange", onChildVisibilityChange, false);
}
</script>
<iframe onload=startTest()>

> LayoutTests/platform/gtk/Skipped:1400
> +media/adopt-node-crash.html

Stray diff?

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

s/invisible/visible/

> Source/WebCore/dom/Document.cpp:1336
> +    // seems to happen in cases such as thumbnails which we consider as

The thumbnails you're mentioning are chromium-port specific, right? Perhaps a better justification is that visible is considered the default unless the embedder has explicitly indicated that it is hidden. No frame/page means no indication from the embedder.

> Source/WebCore/page/Frame.cpp:709
> +        child->dispatchVisibilityStateChangeEvent();

I'd still like to see some testing of how this API behaves when documents are moved between frames. For instance, it is interesting to verify things like:
- The properties are correct when a document is moved from a visible to hidden frame
- Subsequent child dispatching works properly if a child frame is added or removed during a sibling's visibilitystatechange event.

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