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 <[email protected]> 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 <[email protected]> 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 > > [email protected] > > https://lists.wikimedia.org/mailman/listinfo/mobile-l > > > > > > -- > Jon Robson > http://jonrobson.me.uk > @rakugojon >
_______________________________________________ Mobile-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mobile-l
