[webkit-reviews] review granted: [Bug 87337] [Blackberry] WebKit's fullscreen mode needs to notify page client. : [Attachment 144992] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 31 04:52:28 PDT 2012


Antonio Gomes <tonikitoo at webkit.org> has granted Chris.Guan
<chris.guan at torchmobile.com.cn>'s request for review:
Bug 87337: [Blackberry] WebKit's fullscreen mode needs to notify page client.
https://bugs.webkit.org/show_bug.cgi?id=87337

Attachment 144992: Patch
https://bugs.webkit.org/attachment.cgi?id=144992&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144992&action=review


> Source/WebKit/blackberry/Api/WebPage.cpp:6048
> +// TODO: We should remove this helper class when we decide to support all
elements.

use FIXME

> Source/WebKit/blackberry/Api/WebPage.cpp:6061
> +    // TODO: We should not check video tag when we decide to support all
elements.

ditto

> Source/WebKit/blackberry/Api/WebPage.cpp:6062
> +    if (!element || (!element->hasTagName(HTMLNames::videoTag) &&
!containsVideoTags(element)))

why do you need containsVideoTags al all? is not checking for ::videoTag
enough, since you have the element?

if it can be removed, lets do it. it is still slow...

> Source/WebKit/blackberry/Api/WebPage.cpp:6085
> +    // TODO: We should not check video tag when we decide to support all
elements.

fixme

> Source/WebKit/blackberry/Api/WebPageClient.h:235
>      virtual int fullscreenStart(const char* contextName,
Platform::Graphics::Window*, unsigned x, unsigned y, unsigned width, unsigned
height) = 0;

I would add a comment saying that this method is deprecated.

in fact I would add comments deprecating the whole old codepath.


More information about the webkit-reviews mailing list