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

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Shishir Agrawal <shishir at chromium.org>  2011-05-04 18:11: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?

Removed.

>> LayoutTests/fast/events/page-visibility-iframe-test.html:48
>> +        debug("Too many visibility transitions");  
> 
> Looks like a testFailed() would be better than debug().

Done.

>> 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

Thanks for the pointer. Fixed all tests.

>> LayoutTests/fast/events/page-visibility-transition-test.html:36
>> +//  1`- hidden (should fire event).
> 
> Still a stray character after the "1"

Removed.

>> LayoutTests/fast/events/page-visibility-transition-test.html:62
>> +    }
> 
> Should this have a testFailed for >3 changes like the other test?

Done.

>> 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()>

Done

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

Removed.

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

Done.

>> 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.

Added a comment about the visiiblity needing to be explicitly set by 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.

Added two new tests.

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