MobileFrontend peeps:
Is it okay to remove the following line from SkinMobile.php, line 50?
wfRunHooks( 'GetMobileNotice', array( $this, &$notice ) );
Removal would reduce unnecessary double execution of the Wikipedia Zero banner rendering code in HTML contexts.
Note that SkinMobile*WML*.php, line 34, will still call GetMobileNotice, but I don't think we need the invocation from SkinMobile.php, line 50, unless some other component will be using it, such as CentralNotice. I get the impression that CentralNotice doesn't actually use GetMobileNotice, given the following code on line 75 of SkinMinerva.php:
if ( $wgMFEnableSiteNotice ) { $banners[] = '<div id="siteNotice"></div>'; }
My understanding is that Central Notice will update the <div id="siteNotice"> with JavaScript when CentralNotice is turned on, and that Central Notice doesn't actually implement GetMobileNotice (the git history suggests GetMobileNotice was coded for generalized banners, but with a very specific callee, ZeroRatedMobileAccess, last year) so it should be safe to remove from SkinMobile.php:50.
Okay to proceed with removal of the line?
Thanks. -Adam
Personally I don't see a reason. I'm not sure about the history myself but your assessment seems valid that it was probably introduced for Zero. We should remove it from SkinMobileWML as well. If it's not documented kill it ;-)
On Tue, Jul 2, 2013 at 10:32 AM, Adam Baso abaso@wikimedia.org wrote:
MobileFrontend peeps:
Is it okay to remove the following line from SkinMobile.php, line 50?
wfRunHooks( 'GetMobileNotice', array( $this, &$notice ) );Removal would reduce unnecessary double execution of the Wikipedia Zero banner rendering code in HTML contexts.
Note that SkinMobileWML.php, line 34, will still call GetMobileNotice, but I don't think we need the invocation from SkinMobile.php, line 50, unless some other component will be using it, such as CentralNotice. I get the impression that CentralNotice doesn't actually use GetMobileNotice, given the following code on line 75 of SkinMinerva.php:
if ( $wgMFEnableSiteNotice ) { $banners[] = '<div id="siteNotice"></div>'; }
My understanding is that Central Notice will update the <div id="siteNotice"> with JavaScript when CentralNotice is turned on, and that Central Notice doesn't actually implement GetMobileNotice (the git history suggests GetMobileNotice was coded for generalized banners, but with a very specific callee, ZeroRatedMobileAccess, last year) so it should be safe to remove from SkinMobile.php:50.
Okay to proceed with removal of the line?
Thanks. -Adam
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Okay. Change submitted at https://gerrit.wikimedia.org/r/71710. This is an * incremental* step, well aware that discussion on https://gerrit.wikimedia.org/r/#/c/69336/ is still in process. Once change 69336 is wrapped up, please do let us know and we'll modify ZeroRatedMobileAccess accordingly - be that actually using GetMobileNotice exclusively for banners as Max's code would suggest (would require GetMobileNotice to pass additional context in order to support wiping out the banners array to avoid Central Notice donation stuff from creeping back into Wikipedia Zero contexts), continuing the current practice of GetMobileNotice in one place and MinervaPreRender in another, or maybe even having an altogether different hook (again, would need to be coordinated with template backing fields).
-Adam
On Tue, Jul 2, 2013 at 11:05 AM, Jon Robson jdlrobson@gmail.com wrote:
Personally I don't see a reason. I'm not sure about the history myself but your assessment seems valid that it was probably introduced for Zero. We should remove it from SkinMobileWML as well. If it's not documented kill it ;-)
On Tue, Jul 2, 2013 at 10:32 AM, Adam Baso abaso@wikimedia.org wrote:
MobileFrontend peeps:
Is it okay to remove the following line from SkinMobile.php, line 50?
wfRunHooks( 'GetMobileNotice', array( $this, &$notice ) );Removal would reduce unnecessary double execution of the Wikipedia Zero banner rendering code in HTML contexts.
Note that SkinMobileWML.php, line 34, will still call GetMobileNotice,
but I
don't think we need the invocation from SkinMobile.php, line 50, unless
some
other component will be using it, such as CentralNotice. I get the impression that CentralNotice doesn't actually use GetMobileNotice, given the following code on line 75 of SkinMinerva.php:
if ( $wgMFEnableSiteNotice ) { $banners[] = '<div id="siteNotice"></div>'; }
My understanding is that Central Notice will update the <div id="siteNotice"> with JavaScript when CentralNotice is turned on, and
that
Central Notice doesn't actually implement GetMobileNotice (the git
history
suggests GetMobileNotice was coded for generalized banners, but with a
very
specific callee, ZeroRatedMobileAccess, last year) so it should be safe
to
remove from SkinMobile.php:50.
Okay to proceed with removal of the line?
Thanks. -Adam
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
-- Jon Robson http://jonrobson.me.uk @rakugojon