Calling WKBundlePageSetFooterBanner() API to set a banner to null does not actually hide the banner

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ada Chan]

Calling WKBundlePageSetFooterBanner() API to set a banner to null does not actually hide the banner.  

Looks like we need to call Page::addFooterWithHeight(0) in that case to properly remove the parent layer that hosts the banner.  Ditto for the header banner.

Patch coming soon.

This is needed for <rdar://problem/13917901>.

[Ada Chan]

Created attachment 205775 [details]
Patch

[EFL EWS Bot]

Comment on attachment 205775 [details]
Patch

Attachment 205775 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/995956

[Early Warning System Bot]

Comment on attachment 205775 [details]
Patch

Attachment 205775 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/927397

[Ada Chan]

Created attachment 205776 [details]
Patch

Replaced m_page with corePage().

[Early Warning System Bot]

Comment on attachment 205776 [details]
Patch

Attachment 205776 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/919584

[EFL EWS Bot]

Comment on attachment 205776 [details]
Patch

Attachment 205776 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/943800

[Ada Chan]

Created attachment 205777 [details]
Patch

Wrap the calls with #if ENABLE(RUBBER_BANDING).

[Sam Weinig]

Ideally this would all be in the PageBanner class.  Any reason we can't do this in PageBanner::detachFromPage()?

[Ada Chan]

(In reply to comment #8)
> Ideally this would all be in the PageBanner class.  Any reason we can't do this in PageBanner::detachFromPage()?

PageBanner::detachFromPage() is also called on the header and footer banners in WebPage's destructor. I don't know if it's necessary to hide the banners also for that case, and if there would be issues trying to hide the banners at that point.

Another thing I can do is this:

void WebPage::setHeaderPageBanner(PassRefPtr<PageBanner> pageBanner)
{
    if (m_headerBanner) {
        m_headerBanner->hide();
        m_headerBanner->detachFromPage();
    }

    m_headerBanner = pageBanner;

    if (m_headerBanner)
        m_headerBanner->addToPage(PageBanner::Header, this);
}

In the common case where we are setting the page banner to another instance, the above code would remove the parent layer that hosts the banner and then recreate it again when we add in the new banner. What I had earlier in my patch would avoid that. I'm not sure if that optimization is worth the trouble though. If it's really worth not removing the parent layer that hosts the banner unless it's necessary, I could do this:

void WebPage::setHeaderPageBanner(PassRefPtr<PageBanner> pageBanner)
{
    if (m_headerBanner) {
        if (!pageBanner)
            m_headerBanner->hide();
        m_headerBanner->detachFromPage();
    }

    m_headerBanner = pageBanner;

    if (m_headerBanner)
        m_headerBanner->addToPage(PageBanner::Header, this);
}

Please let me know what you think.  By the way, should we add a check in the beginning of the method to see if pageBanner is the same as m_headerBanner and bail early if it is?

[Ada Chan]

Created attachment 205831 [details]
Patch

Fix this in PageBanner::detachFromPage() as suggested by Sam.

[Ada Chan]

Committed:
https://trac.webkit.org/changeset/152249