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

[bugzilla-daemon at webkit.org]

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


Shishir Agrawal <shishir at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #90455|1                           |0
        is obsolete|                            |




--- Comment #18 from Shishir Agrawal <shishir at chromium.org>  2011-04-27 17:17:06 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.

Done.

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

Done

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

Changed this to visible. Right now I am not sure what the right value for this is. I have only seen this happen in case of thumbnails and ideally they should have a visibility state of their own.

>> Source/WebCore/dom/Document.h:381
>> +    PageVisibilityState webkitVisibilityStateEnum() const;
> 
> This should be private, and I'd recommend just renaming it to visibilityState() or something.

Done.

>> Source/WebCore/page/Page.cpp:955
>> +void Page::setInitialVisibilityState(PageVisibilityState visibilityState)
> 
> This is never used. Is that in a subsequent patch?

Aah thanks for catching this. Comitted to wrong branch. Brought the changes in.

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

I see roughly 225 failures with and without my changes. How do I tell if my change is responsible. There are a lot of SVG failures too. Should I make sure tests in some directory pass?

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