Bug 87337 - [Blackberry] WebKit's fullscreen mode needs to notify page client.
Summary: [Blackberry] WebKit's fullscreen mode needs to notify page client.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-23 20:30 PDT by Chris.Guan
Modified: 2012-10-04 14:37 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.21 KB, patch)
2012-05-23 22:36 PDT, Chris.Guan
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2012-05-30 04:46 PDT, Chris.Guan
no flags Details | Formatted Diff | Diff
Patch (9.38 KB, patch)
2012-05-30 05:22 PDT, Chris.Guan
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2012-05-30 22:46 PDT, Chris.Guan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris.Guan 2012-05-23 20:30:35 PDT
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.
Comment 1 Chris.Guan 2012-05-23 22:36:14 PDT
Created attachment 143732 [details]
Patch
Comment 2 Antonio Gomes 2012-05-24 07:01:28 PDT
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.
Comment 3 Chris.Guan 2012-05-24 07:07:04 PDT
(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.
Comment 4 Rob Buis 2012-05-24 07:10:54 PDT
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?
Comment 5 Chris.Guan 2012-05-24 07:17:56 PDT
(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.
Comment 6 Chris.Guan 2012-05-30 04:46:16 PDT
Created attachment 144780 [details]
Patch
Comment 7 Antonio Gomes 2012-05-30 04:50:10 PDT
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?
Comment 8 Chris.Guan 2012-05-30 05:22:32 PDT
Created attachment 144786 [details]
Patch
Comment 9 Chris.Guan 2012-05-30 05:33:33 PDT
(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.
Comment 10 Max Feil 2012-05-30 12:25:51 PDT
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?
Comment 11 Chris.Guan 2012-05-30 19:46:42 PDT
(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.
Comment 12 Chris.Guan 2012-05-30 22:46:53 PDT
Created attachment 144992 [details]
Patch
Comment 13 Antonio Gomes 2012-05-31 04:52:28 PDT
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.
Comment 14 Max Feil 2012-05-31 09:06:01 PDT
(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.
Comment 15 Max Feil 2012-05-31 09:14:28 PDT
(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.
Comment 16 Antonio Gomes 2012-05-31 10:16:11 PDT
Comment on attachment 144992 [details]
Patch

s/todo/fixme won't hold it from landing...
Comment 17 WebKit Review Bot 2012-05-31 10:33:55 PDT
Comment on attachment 144992 [details]
Patch

Clearing flags on attachment: 144992

Committed r119119: <http://trac.webkit.org/changeset/119119>
Comment 18 WebKit Review Bot 2012-05-31 10:34:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Max Feil 2012-10-04 14:37:41 PDT
This patch has broken fullscreen video for AIR and WebWorks apps on some sites like YouTube. I am investigating a fix.