On Tue, Aug 16, 2011 at 3:25 AM, Tomasz Finc tfinc@wikimedia.org wrote:
I'll let Patrick respond to the more technical questions but Brion Vibber has been doing the majority of the reviews for this. He's been helped by Reedy and Aaron for some of the revisions as well.
--tomasz
On Mon, Aug 15, 2011 at 8:15 PM, MZMcBride z@mzmcbride.com wrote:
Patrick Reilly wrote:
On Aug 15, 2011, at 6:27 PM, MZMcBride z@mzmcbride.com wrote:
Is the MobileFrontend extension development being monitored or reviewed by any senior developers?
Yes, it has been reviewed.
By whom?
Please see Tomasz's comment.
I took a look at the code today and it seemed incredibly strange.
Could you send along some examples please.
The use of a views directory containing .html.php files stuck out to me. As far as I know, this is the only extension in Wikimedia's SVN repo using this type of code structure. It looks like a holdover from the old Ruby code. Is that right? Is it going to be rewritten?
Yes, you are correct it is a holdover from the old Ruby code. It could be rewritten to be more in-line with the standards.
Some of its layout and architecture doesn't seem to be consistent with MediaWiki conventions/style and a lot of code (particularly variable assignments) looked duplicative.
Can you please provide an example.
Variables such as $randomButton are defined four times in the same file (extensions/MobileFrontend/MobileFrontend.php).
Yes, those view related variables could be created as class level variables instead avoiding the duplication.
I don't know too much about
PHP, but it seemed very strange. Is there a reason for the duplication?
Not really any good technical reason. It was mostly due to the views being slightly refactored.
Could it be reduced?
Yes.
In general, the URL parameters are odd. I'm not sure if the English Wikipedia is running the newest code, but if you visit a URL such as http://en.wikipedia.org/w/index.php?title=Main_Page&useFormat=mobile, none of the links within it maintain the &useFormat parameter, including and especially the links at the bottom.
This is by design. The useFormat parameter is used only to force the mobile view. The main way that the extension is invoked is via the X-Device header.
If you click one of the links at the
bottom such as "enable images on the mobile site," it removes all parameters including the ?title= parameter.
This could be easily changed. Thanks, for the feedback.
Rather than something sane and predictable for the URL parameter to view the standard (non-mobile) site (such as &useFormat=standard), the code uses ?mAction=view_normal_site.
This could also be easily changed for consistency sake.
As I commented in CodeReview today, a boolean parameter (?disableImages=1) has now been complemented with a completely separate parameter (?enableImages=1) rather than using ?disableImages=0, which I find to be very strange.
Okay, duly noted.
The HTML at the bottom of the page at http://en.wikipedia.org/w/index.php?title=Main_Page&useFormat=mobile appears to also be completely invalid (pasted below):
<div id='copyright'>Text is available under the <a rel="license" href="http://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attrib ution-ShareAlike_3.0_Unported_License">Creative Commons Attribution-ShareAlike License</a><a rel="license" href="http://creativecommons.org/licenses/by-sa/3.0/" style="display:none;"></a>; additional terms may apply. See <a href="http://wikimediafoundation.org/wiki/Terms_of_use">Terms of use</a> for details.<br/> Wikipedia® is a registered trademark of the <a href="http://www.wikimediafoundation.org/">Wikimedia Foundation, Inc.</a>, a non-profit organization.<br /></li><li class="noprint"><a class='internal' href="http://en.wikipedia.org/wiki/Wikipedia:Contact_us">Contact us</a></div> </div> ----
The copyright is not part of the mobile extension it is core to media wiki.
As far as I can see, the code is defining list items without specifying a
<ul> or <ol> pair. One of the <li> elements is unclosed. And there's also some very strange display:none; code in this snippet.
All of this seems very odd and quirky. I believe some of this quirkiness would be mitigated by using pre-built functions and following established coding styles, rather than using DIY templates.
This is NOT part of the template.
Again, I'm wondering which senior developer (or even non-senior developer) is reviewing this code. Some clarification on that would be appreciated.
It has been provided.
I've probably written about ten lines of PHP in my life, so feel free to tell me off if these comments are out of left field and/or don't make any sense.
They make sense and are appreciated.
These were just my observations from reading through some of the code
today.
I appreciate you taking the time to do that.
Some revisions are also apparently being pushed live without any outside review.
Yes, this is known. Every effort has been made to get eyes on every change, but some things have been pushed quickly for testing and to meet our schedule for the opt-in launch.
It's incredibly dangerous to push out code without at least cursory outside review. It's an enormous risk you're taking, though it's apparently yours to take. It's a very bad situation, in my opinion.
Duly noted.
MZMcBride
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l