Bug 87337 – [Blackberry] WebKit's fullscreen mode needs to notify page client.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Chris.Guan]

No fullscreen video widget has been made available by the Browser chrome, or It is not a video element. The webkitRequestFullScreen Javascript call is often made on a div element. we would call client's full screen APIs.

[Chris.Guan]

Created attachment 143732 [details]
Patch

[Antonio Gomes]

Comment on attachment 143732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143732&action=review

Some suggestions:

> Source/WebKit/blackberry/ChangeLog:9
> +        chrom or it is not a video element. The webkitRequestFullScreen Javascript 

chrom*

period should be a comma

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

is not it clear to add another method with parameters? adding 6 default values seems too much to me.

Then we can name one deprecatedFullScreenStart, and keep using the other.

[Chris.Guan]

(In reply to comment #2)
> (From update of attachment 143732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143732&action=review
> 
> Some suggestions:
> 
> > Source/WebKit/blackberry/ChangeLog:9
> > +        chrom or it is not a video element. The webkitRequestFullScreen Javascript 
> 
> chrom*
> 
> period should be a comma
> 
> > Source/WebKit/blackberry/Api/WebPageClient.h:239
> > +    virtual int fullscreenStart(const char* contextName = 0, Platform::Graphics::Window* = 0, unsigned x = 0, unsigned y = 0, unsigned width = 0, unsigned height = 0) = 0;
> 
> is not it clear to add another method with parameters? adding 6 default values seems too much to me.
> 
> Then we can name one deprecatedFullScreenStart, and keep using the other.

Thanks, Antonio, I will fix them next patch, but we have some secure and UX issues now, I cancelled reveiew first, see internal PR for more please. Thanks.

[Rob Buis]

Comment on attachment 143732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143732&action=review

Looks good, but could be improved more.

> Source/WebKit/blackberry/ChangeLog:9
> +        chrom or it is not a video element. The webkitRequestFullScreen Javascript 

chrome

> Source/WebKit/blackberry/ChangeLog:11
> +        to enter or exit full screen mode.

You probably want to reword the ChangeLog a bit:

Cover the case where no no fullscreen video widget has been made available by the Browser Chrome, or it is not a video element. In this case we call client's full screen APIs to enter or exit full screen mode.

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

Do we care for the parameters at all if they are not handed over? Or is there some other place not in this patch that states these parameters?

[Chris.Guan]

(In reply to comment #4)
> (From update of attachment 143732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143732&action=review
> 
> Looks good, but could be improved more.
> 
> > Source/WebKit/blackberry/ChangeLog:9
> > +        chrom or it is not a video element. The webkitRequestFullScreen Javascript 
> 
> chrome
> 
> > Source/WebKit/blackberry/ChangeLog:11
> > +        to enter or exit full screen mode.
> 
> You probably want to reword the ChangeLog a bit:
> 
> Cover the case where no no fullscreen video widget has been made available by the Browser Chrome, or it is not a video element. In this case we call client's full screen APIs to enter or exit full screen mode.
> 
> > Source/WebKit/blackberry/Api/WebPageClient.h:239
> > +    virtual int fullscreenStart(const char* contextName = 0, Platform::Graphics::Window* = 0, unsigned x = 0, unsigned y = 0, unsigned width = 0, unsigned height = 0) = 0;
> 
> Do we care for the parameters at all if they are not handed over? Or is there some other place not in this patch that states these parameters?

Thanks, Rob. I should cancel r? earlier. see internal PR for more.

[Chris.Guan]

Created attachment 144780 [details]
Patch

[Antonio Gomes]

Comment on attachment 144780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144780&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:6061
> +    // Search for a video element in this document.
> +    Document* document = frame->document();
> +    for (Node* node = document->firstChild(); node; node = node->traverseNextNode(document)) {
> +        if (!node->isElementNode())
> +            continue;
> +        if (node->hasTagName(HTMLNames::videoTag))
> +            return true;
> +    }
> +
> +    // Do the same for the nested frames.
> +    for (Frame* child = frame->tree()->firstChild(); child; child = child->tree()->nextSibling()) {
> +        if (containsVideoTags(child))
> +            return true;

that can be VERY slow. r- due to that.

> Source/WebKit/blackberry/Api/WebPage.cpp:6082
> +#if ENABLE(VIDEO)

can you have #fullscreen_api enabled and #video disabled?

[Chris.Guan]

Created attachment 144786 [details]
Patch

[Chris.Guan]

(In reply to comment #7)
> (From update of attachment 144780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144780&action=review
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:6061
> > +    // Search for a video element in this document.
> > +    Document* document = frame->document();
> > +    for (Node* node = document->firstChild(); node; node = node->traverseNextNode(document)) {
> > +        if (!node->isElementNode())
> > +            continue;
> > +        if (node->hasTagName(HTMLNames::videoTag))
> > +            return true;
> > +    }
> > +
> > +    // Do the same for the nested frames.
> > +    for (Frame* child = frame->tree()->firstChild(); child; child = child->tree()->nextSibling()) {
> > +        if (containsVideoTags(child))
> > +            return true;
> 
> that can be VERY slow. r- due to that.
yes, I think so. we do not need to check frame probably, Thanks.
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:6082
> > +#if ENABLE(VIDEO)
> 
> can you have #fullscreen_api enabled and #video disabled?
I think it is possible, fullscreen_api is for all elements. But for some secure concerns, we only apply fullscreen mode to video tags or those elements containing video tags.

[Max Feil]

Comment on attachment 144786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144786&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:5487
> +        static_cast<Element*>(d->m_fullscreenVideoNode.get())->document()->webkitCancelFullScreen();

This line is not correct. The m_fullscreenVideoNode is only used by the enterFullScreenForNode path (when fullScreenVideoCapable is true). So it will always be zero here. Was this code path tested?

[Chris.Guan]

(In reply to comment #10)
> (From update of attachment 144786 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144786&action=review
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:5487
> > +        static_cast<Element*>(d->m_fullscreenVideoNode.get())->document()->webkitCancelFullScreen();
> 
> This line is not correct. The m_fullscreenVideoNode is only used by the enterFullScreenForNode path (when fullScreenVideoCapable is true). So it will always be zero here. Was this code path tested?

Max, I reuse m_fullscreenVideoNode in this patch, I assigned/cleared the node in enter/exitFullScreenForElement functions. Chrome should not call "notifyFullScreenVideoExited" when it is not in full screen mode, should it? Yes, anyway, I should have null pointer check or Assert. Thanks.

[Chris.Guan]

Created attachment 144992 [details]
Patch

[Antonio Gomes]

Comment on attachment 144992 [details]
Patch

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.

[Max Feil]

(In reply to comment #11)
> Max, I reuse m_fullscreenVideoNode in this patch, I assigned/cleared the node in enter/exitFullScreenForElement functions. Chrome should not call "notifyFullScreenVideoExited" when it is not in full screen mode, should it? Yes, anyway, I should have null pointer check or Assert. Thanks.

I didn't notice your reuse of m_fullscreenVideoNode, sorry. In that case just doing a null pointer check is fine.

[Max Feil]

(In reply to comment #13)
> why do you need containsVideoTags at all? is not checking for ::videoTag enough, since you have the element?

The "contains" check is needed because the element will often be a container like a DIV. In fact that is one of the main points of PR158257 which this webkit bug is designed to address.

> I would add a comment saying that this method is deprecated.
> in fact I would add comments deprecating the whole old codepath.

The previous fullscreen codepath is not deprecated yet. It needs to be maintained to support AIR and native fullscreen widgets in general. Once AIR is no longer supported in BlackBerry OS we can revisit this.

[Antonio Gomes]

Comment on attachment 144992 [details]
Patch

s/todo/fixme won't hold it from landing...

[WebKit Review Bot]

Comment on attachment 144992 [details]
Patch

Clearing flags on attachment: 144992

Committed r119119: <http://trac.webkit.org/changeset/119119>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Feil]

This patch has broken fullscreen video for AIR and WebWorks apps on some sites like YouTube. I am investigating a fix.