WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
118219
Calling WKBundlePageSetFooterBanner() API to set a banner to null does not actually hide the banner
https://bugs.webkit.org/show_bug.cgi?id=118219
Summary
Calling WKBundlePageSetFooterBanner() API to set a banner to null does not ac...
Ada Chan
Reported
2013-06-30 08:53:41 PDT
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
>.
Attachments
Patch
(2.52 KB, patch)
2013-06-30 09:12 PDT
,
Ada Chan
eflews.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch
(2.52 KB, patch)
2013-06-30 09:29 PDT
,
Ada Chan
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
Patch
(2.60 KB, patch)
2013-06-30 10:16 PDT
,
Ada Chan
no flags
Details
Formatted Diff
Diff
Patch
(2.20 KB, patch)
2013-07-01 13:09 PDT
,
Ada Chan
bdakin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Ada Chan
Comment 1
2013-06-30 09:12:01 PDT
Created
attachment 205775
[details]
Patch
EFL EWS Bot
Comment 2
2013-06-30 09:16:55 PDT
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 3
2013-06-30 09:17:27 PDT
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
Comment 4
2013-06-30 09:29:15 PDT
Created
attachment 205776
[details]
Patch Replaced m_page with corePage().
Early Warning System Bot
Comment 5
2013-06-30 09:37:15 PDT
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 6
2013-06-30 09:38:06 PDT
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
Comment 7
2013-06-30 10:16:15 PDT
Created
attachment 205777
[details]
Patch Wrap the calls with #if ENABLE(RUBBER_BANDING).
Sam Weinig
Comment 8
2013-06-30 14:28:01 PDT
Ideally this would all be in the PageBanner class. Any reason we can't do this in PageBanner::detachFromPage()?
Ada Chan
Comment 9
2013-06-30 22:28:06 PDT
(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
Comment 10
2013-07-01 13:09:21 PDT
Created
attachment 205831
[details]
Patch Fix this in PageBanner::detachFromPage() as suggested by Sam.
Ada Chan
Comment 11
2013-07-01 14:58:59 PDT
Committed:
https://trac.webkit.org/changeset/152249
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug