Last modified: 2014-10-09 19:03:58 UTC
Hi, I would like to ask for a full code review of trunk/extensions/OnlineStatusBar I believe there is a lot to fix or improve, especially concerning its performance, so any feedback would be really appreciated, thanks!
Think "Extension requests" is a better place for this request. Also, it may be more effective to find someone in #mediawiki on IRC and ask there.
That's the place where they sent me here :)
Also related - bug 26246. Also, if the proposal on wikipedia turns out to be succesful, you'll want to mark this as blocking bug 31235. ------- I happened to take a superficial look at the extension today (Saw it mentioned in the signpost, and wondered what it was about). One thing you might consider doing is combinding some of the CSS files, so that the stuff that is the same in all of them goes in a common css file, and the one or two lines that differ go in the skin specific css files. Another thing I noticed is underscores in some variable names (but that's a stylistic thing that doesn't matter). Also, I'm not sure how well this extension would play with squid caching. Logged out users will probably see outdated statuses as far as i can tell. Cheers
Other things I notice: *The wording on the options in the preferences is kind of weird (Do you want X? where most other preferences are not worded as a question) *"Purge user page everytime when you login or logout" is an implentation detail, and should not be a preference *flash of semi-unstyled content. The css for this extension should be loaded at the top *It's weird to be able to have different statuses like busy, on-line etc, but not be able to set them (you can set the default certainly on special:preferences, but the user shouldn't have to be constantly going to special:prefs to change their status, preferences should be used for rarely changed options. There should be some other method (perhaps, if user is viewing own user page, have a drop down box where the status is displayed) * <b>Notice</b>: Undefined variable: w_time in <b>/var/www/w/extensions/OnlineStatusBar/OnlineStatusBar.status.php</b> on line <b>99</b><br /> Displayed directly after i made an edit to my user page *Note, my comment before about squid might be incorrect, the purge i see in the code should take care of that I think.
Thank you, CSS: I don't know if it would have some effect if I split it to more files, because now only one file needs to be loaded when page is opened, if I split it, I would need to load more files, so it would have probably negative effect on loading of page. Regarding java script - it would be cool if there was option to change it directly but since I don't understand js, it's not possible for me to implement it, but if anyone could do that, it would be great. I believe that most of other issues you mentioned were fixed during weekend, and if not I am still working on it.
So the discussion on english wp was closed in favor of installation of this: http://en.wikipedia.org/wiki/Wikipedia:Village_pump_(proposals)#Online_Status Therefore I would like to ask for more input concerning this extension, what all should be implemented or fixed or what do you think that isn't correct. I tried to reword some of options according to feedback of english wikipedia users (native english speakers), so I hope it should be correct now. In case that there were no more concerns, it would be nice if someone could do full code review or help me to finish it so that it could be deployed to english wp. Thank you
I would like to remind that extension is now installed here http://hub.tm-irc.org/test/wiki/User_talk:Petrb (example of status) and that anyone can create as many user accounts as they need to test it, thank you for your feedback
(In reply to comment #5) > Thank you, > > CSS: > I don't know if it would have some effect if I split it to more files, because > now only one file needs to be loaded when page is opened, if I split it, I > would need to load more files, so it would have probably negative effect on > loading of page. > Nope, the ResourceLoader will combine them all before sending them to the user. Only one css file will be downloaded in either case.
Why's it been added against bug 31235? I've not seen a request for it to be enabled on WMF wikis
(In reply to comment #9) > Why's it been added against bug 31235? > > I've not seen a request for it to be enabled on WMF wikis Comment 6: http://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28proposals%29#Online_Status (perma-link http://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_%28proposals%29&oldid=462882512#Online_Status )
Right, I will try to split them, but how can I make it load on top? Is it possible to make it load the css before other parts of the page? Concerning option to purge user page, I think it should remain in options, because there may be users who: 1 - want to show status bar / not using magic word - in this case it should not purge 2 - want to hide status bar / using magic word - in this case it should purge 3 - want to use both - in this case it should purge Reedy: sorry I am sort of new to this, I didn't know there is a queue you mentioned, I will try to insert it there soon
(In reply to comment #11) > Right, I will try to split them, but how can I make it load on top? Is it > possible to make it load the css before other parts of the page? Use the OutputPage method addModuleStyles instead of addModules. > Concerning option to purge user page, I think it should remain in options, > because there may be users who: > 1 - want to show status bar / not using magic word - in this case it should not > purge [...] Even in that case, you'd want to purge the page. (The status bar gets cached too)
(In reply to comment #12) > > Concerning option to purge user page, I think it should remain in options, > > because there may be users who: > > 1 - want to show status bar / not using magic word - in this case it should not > > purge > [...] > Even in that case, you'd want to purge the page. (The status bar gets cached > too) For some reason it works even without purge (at least on my test wiki, while magic word needs purge), why? If it's necessary I will do that but I wanted to avoid purge if possible Thanks for your help
> > For some reason it works even without purge (at least on my test wiki, while > magic word needs purge), why? If it's necessary I will do that but I wanted to > avoid purge if possible > > Thanks for your help You're right, that isn't in the parser cache. (I wonder why it wasn't being cleared away for me in my testing). However, it would still be cached by squids/file cache if those caches are in use [but could probably use something that clears them without doing a full purge if overly concerned]. In any case, if purge is not desired in those cases, should be detected based on other options, not something user specifiable.
(In reply to comment #14) > > > > For some reason it works even without purge (at least on my test wiki, while > > magic word needs purge), why? If it's necessary I will do that but I wanted to > > avoid purge if possible > > > > Thanks for your help > You're right, that isn't in the parser cache. (I wonder why it wasn't being > cleared away for me in my testing). However, it would still be cached by > squids/file cache if those caches are in use [but could probably use something > that clears them without doing a full purge if overly concerned]. In any case, > if purge is not desired in those cases, should be detected based on other > options, not something user specifiable. Problem is that you can't always detect if user wants this or not, they can for instance change backgroud color of their user page, using magic word while having the status bar as well (that's why there is 'for advanced users') If it's problem it can be removed, however I think that giving people more options to customize it would be better, or at least should be overridable in LS.php?
Althought both caches are installed on test wiki where I am testing it, I have no problems with that, you noticed that there could be problem with squid cache, but unless I have different configuration on server than is on production I think it should be ok, because when status expires, user is flagged offline even without purge, so maybe it's not needed, I added some code which should handle cache expiry times for this reason, I hope it helps a bit, do you see any other problem?
Hm, there is probably bug in core, but I would like to check before I report it, setCacheExpiry purge all caches excepting browser cache in firefox, so actually it has troubles in firefox (user status doesn't update, unless browser cache is turned off), is here any workaround to flush browser cache which works for all browsers?
(In reply to comment #17) > Hm, there is probably bug in core, but I would like to check before I report > it, setCacheExpiry purge all caches excepting browser cache in firefox, so > actually it has troubles in firefox (user status doesn't update, unless browser > cache is turned off), is here any workaround to flush browser cache which works > for all browsers? I assume you mean updateCacheExpiry (method of ParserOutput), in which case yes this is normal. It will only update the time the parser cache expires. Squid still needs to be explicitly purged. Browsers should not be caching user pages when (Or more specificly, they should do if-modified-since checks every time). However, ParserOutput::updateCacheExpiry doesn't update the last modified times, so server's would still send HTTP 304 not modified in those cases.
A rough review / suggestion: -- Purging a user page (and any other place it will potentially be shown on) because the user online status changes seems wrong. I haven't looked at the extension deeply yet to know wether or not and if so how it does this, but if it does, it shouldn't. I suggest solving this with AJAX instead. * users can set a preference to join/leave exposing their status * users can set a preference to do or don't show other ppls statuses By default we will probably want to make the first preference false and the second one true. Then the extension would have a scripts&style module, with position=>top, loaded through addModules instead of addModuleStyles. * a stylesheet + extra stylesheets for per-skin perfection * one or more javascript files that will load this information from the API and display in on the appropiate pages The module would load dependent on the user preference (use the BeforePageDisplay hook, then conditionally $out->addModules). And, as noted, the information would have to be made available through an API module. While taking a quick look at the sql file, I noticed it uses column username (VARCHAR 255) and timestamp (char 14). I suggest user IDs instead and timestamps per MediaWiki convention (and columnames prefixed) * os_user int unsigned NOT NULL * os_timestamp binary(14) NOT NULL default '19700101000000' (mysql types based on user.user_id and logging.log_timestamp [1]) -- Krinkle [1] http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/maintenance/tables.sql?view=markup
> * users can set a preference to join/leave exposing their status rephrase, * users can set a preference to opt-in/out exposing their status
Thank you for review: SQL: it uses char's because it allows even tracking of IP users (if user allow that in LS.php) it's disabled by default, however if I changed it to user ID it would not be possible (but since it was developed based on request on enwp where tracking of IP users would be disabled, I don't think it's important) + also using user name seems to me easier since I do not need to translate it to user ID and back Timestamp will be updated asap AJAX: I don't know much about ajax but I believe you are correct :) The default options are already in configuration (if users have it enabled as default etc.) API: I will try to implement it there soon Regarding cache - I don't know how different it would be when AJAX would be used, so maybe the current problems with cache flushing wouldn't occur using that, so I won't care about that for now.
I implemented api so that now you can do api.php?action=query&prop=onlinestatus&onlinestatususer=user to get status of user, unfortunately I still don't know how to implement AJAX part I will try to find it out :)
thanks to Brion it's updated so you can review it now design is now little bit different, the online bar doesn't even query db, but it retrieve it using api every two minutes (when user wants to display it)
is there any progress in review of the source?
Assigned to Krinkle since he said he is going to review it
OK. Lots of progress has been made so I'll give it a full extension review this time (on a wiki page). Note that I'm going to FOSDEM this weekend and after that I'll be offline for 2 weeks. I'll be back February 20 and this'll be reviewed that week :)
Krinkle: ping. If you can't do this within the next month, please say so and I'll try to find someone else to do so.
(In reply to comment #27) > Krinkle: ping. If you can't do this within the next month, please say so and > I'll try to find someone else to do so. I won't have time to do this in my spare time as I have other things I plan to work on in the coming weeks/months. As foundation work, I basically do whatever I'm assigned to. So the regular channels apply here. This is currently not on my agenda and I'm currently assigned to 2 medium-long term projects that are currently taking up all my available hours per week (ResourceLoader and Continuous integration) and that likely won't change for at least another month.
Adding the design keyword - Brandon, could you take a look at the UI and give some feedback here?
I thought I had given some feedback about this extension time ago (perhaps through irc, I don't seem to have edited this bug). I think it would be better to use memcached for this, as it's not something we want to keep or replicate. Additionally, it seems to support Online/Away/Offline, with Away being calculated automatically. Seems desirable that it allowed to mark yourself as being away or offline without having to change your preferences. Also, some user scripts with this kind of goal, support a Busy status, so this should probably allow that, too.
It uses memcached
Trevor is taking over code review for this extension as part of his 20% community service time per https://www.mediawiki.org/wiki/Wikimedia_engineering_20%25_policy .
Thanks Trevor! Let me know if you found any issue
Trevor began this review last week and it is continuing. <TrevorParscal> yeah, so i'm reviewing en masse because there are so many deferred revs and things need one good pass <TrevorParscal> also, i'm doing a design review at the same time Assigning to Trevor. Trevor will communicate with Peter to request fixes.
https://www.mediawiki.org/w/index.php?path=%2Ftrunk%2Fextensions%2FOnlineStatusBar&title=Special%3ACode%2FMediaWiki has the updates and code review Krinkle has done, and we'll be moving the extension to Gerrit this week. Useful bits of IRC conversation from today: <TrevorParscal> I just OK'd Timo's whitespace fixes <TrevorParscal> I think this extension is ready to be moved to gerrit personally <Krinkle> yeah, we both did a review and a small pass with fixes over the code. afaik it is fully reviewed and fixed up. I did open 1 or 2 bugs in bugzilla that I think have to be fixed before deployment on production wikis <Krinkle> they block deployment of any kind imho, but moving the repo into gerrit is fine <TrevorParscal> it's gone through UI review (by me) and UI fixup (by petr), JS/CSS rewrite (by me( and JS/CSS review (by Timo) and PHP review (by me and Timo) and fix up (by Petr and Timo) and finally more review (by me) <^demon> Let's get it on the list so it'll get done this week. <Krinkle> TrevorParscal: I didn't put this in review or in bugzilla, but something else I dislike about it is how it displays the information "John Doe is now <blib> offline" or "John Doe is now <blib> hidden" or "John Doe is now <blib> status is unknown" <TrevorParscal> Krinkle: yeah, I suggested that the <blip> be moved to the far right and the sentence be more consistent
It's now in Git/Gerrit: https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/extensions/OnlineStatusBar.git;a=summary Timo, Trevor: do you approve this for deployment?
Deployment where for which community/communities? If this is deployment beyond testwiki, then the design is imho a requirement and a blocker for deployment (both usability wise, as well as technically speaking due to the implementation with the problematic absolute-position which conflicts with many other components and gadgets). I don't know the design has been improved on either front since I last reviewed it. The blocker bug as dependency on this one for it is still open at this time.
-design: there is no design needed to deploy this extension (which is what this bug is for). The blocking bug, bug 37132, is design related and already has the design keyword.
And the "John Doe is now status is unknown" issue has been fixed as well (by Krinkle) in https://gerrit.wikimedia.org/r/#/c/22180/.
http://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_%28proposals%29&oldid=462882512#Online_Status is at least the beginnings of consensus to add something like OnlineStatusBar to English Wikipedia, and it looks like nearly all the blockers and critiques have been resolved. Trevor and Timo, tell me if I'm wrong. I want to ensure the design department has looked at this and given a thumbs-up, per https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment , so I've added the extension to https://www.mediawiki.org/wiki/User_experience_review_queue#Pending and am cc'ing the Editor Engagement product manager and Jorm from design. We've already gotten an OK from tech on the deployability of the code, so once design & product management gives OnlineStatusBar the OK, we can use that existing en.wp consensus to move forward and ask Sam Reed to put OSB on the deploy schedule. (All of that is In My Opinion -- tell me if I'm wrong.)
On the tech side it is okay now indeed. Though I believe there are various concerns still from the design, usability and/or accessibility perspective. It could be sufficient for a trial deployment, but there's certainly room for improvement.
(In reply to comment #40) > We've already gotten an OK from tech on the deployability of the code, so > once design & product management gives OnlineStatusBar the OK It looks like there hasn't been any progress here for the last five months - CC'ing some more design people, asking for an extension review.
Hello, this is a quasi-automated-but-not-really message: I am reviewing all tracking bugs for extensions to review and deploy to WMF servers. See the list here: https://bugzilla.wikimedia.org/showdependencytree.cgi?id=31235&hide_resolved=1 The [[mw:Review queue]] page lists the steps necessary to complete the review. I have copied them below and done some initial filling out based on what I can easily gleen from this bug and any linked to sources that are obvious. If I miss something/state something false, please do correct me. Also, if you haven't yet done so, please review the information on and linked to from: https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment == TODO/Check list == Extension page on mediawiki.org: https://www.mediawiki.org/wiki/Extension:OnlineStatusBar Bugzilla component: Yes Extension in Gerrit: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/OnlineStatusBar Design Review: not done Archeticecture/Performance Review: maybe (per Krinkle)? Security Review: not done (right?) Screencast (if applicable): not that I can find. Community support: Looks to be for enwp. Suggestions for next steps: Please email the Design mailing list at https://lists.wikimedia.org/mailman/listinfo/design asking for an explicit design review. Also, ask on wikitech-l for a security review. Thanks.
