Page MenuHomePhabricator

Remove the Disable images functionality
Closed, ResolvedPublic2 Story Points

Description

Image disabling statistics for July:

mysql:research@analytics-store.eqiad.wmnet [log]> select count(*), event_images from MobileOptionsTracking_8101982 where timestamp like '201507%' group by event_images;
+----------+--------------+
| count(*) | event_images |
+----------+--------------+
|   737033 | nochange     |
|      113 | off          |
|      131 | on           |
+----------+--------------+
3 rows in set (5.81 sec)

This means that this functionality is barely ever used and maintaining it (cf T107368 and T109286) takes more effort than the good it brings. Therefore, it makes sense to remove it. Ping Zero just in case (it shouldn't affect ZeroBanner that removes images on its own, but oh well).

Acceptance criteria

  • When I visit Special:MobileOptions I do not see the "disable images" feature.
  • Drop any code relating to the feature in MobileFrontend
  • Drop any code relating to the feature in WikidataPageBanner
  • Drop any code relating to the feature in Zero

^ There are no other extensions that rely on or set wgDisableImages: https://github.com/search?q=org%3Awikimedia+wgImagesDisabled&type=Code

Sign off steps

  • Ensure all subtasks are resolved
  • Decline any subtasks implicated by this decision

Post-deploy steps (Thursday, 22nd June)

Note:

If for some reason we decide to keep this feature, we will need to revisit the decision in T122267. N/A

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bmansurov raised the priority of this task from Lowest to Normal.Dec 1 2016, 9:54 PM
bmansurov added a subscriber: bmansurov.

@Jdlrobson should we consider removing the feature now that the lazy-loading of images is in place?

Yes, it's my opinion we should remove this option, but we'd need to talk to the partnership team about how Zero uses it first.

bmansurov renamed this task from Either encourage use of or remove the Disable images functionality to Remove the Disable images functionality.Dec 1 2016, 10:56 PM
Jdlrobson added a subscriber: ovasileva.

@ovasileva you would need to talk to partnerships team. I believe the underlying functionality is used by Zero so we wouldn't be able to remove the PHP code, but we can certainly remove the option from Special:MobileOptions

ovasileva added a subscriber: DFoy.Apr 17 2017, 5:46 PM

@Jdlrobson - sorry, I didn't submit my comment from the last change. After discussing shortly with @DFoy, it seems Zero does not use this. We can go ahead and remove it.

Jdlrobson added a comment.EditedApr 17 2017, 10:09 PM

The correct question to ask @DFoy is probably.. do any Zero providers provide Wikipedia on the expectation that we strip images from page views?

We can remove the setting from mobile options easily, but I'm not sure about whether the underlying technology needs to stay here (we might want to move that to Zero if so)

ovasileva set the point value for this task to 2.Jun 7 2017, 4:48 PM
Jdlrobson updated the task description. (Show Details)Jun 7 2017, 6:56 PM

Change 359226 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/WikidataPageBanner@master] MobileFrontend is removing the disabled images feature

https://gerrit.wikimedia.org/r/359226

Change 359229 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop the disable images feature

https://gerrit.wikimedia.org/r/359229

Change 359226 merged by jenkins-bot:
[mediawiki/extensions/WikidataPageBanner@master] MobileFrontend is removing the disabled images feature

https://gerrit.wikimedia.org/r/359226

Change 359417 had a related patch set uploaded (by Ema; owner: Ema):
[operations/puppet@production] VCL: remove disableImages handling

https://gerrit.wikimedia.org/r/359417

Change 359229 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop the disable images feature

https://gerrit.wikimedia.org/r/359229

@ema: rEMFRc89f371ea9e7: Drop the disable images feature has been merged and will ride this week's change. We'll be able to merge rOPUP332d2ca8abac: VCL: remove disableImages handling after the MediaWiki train deployment on Thursday. 22nd.

Also, thanks for submitting that change @ema!

phuedx updated the task description. (Show Details)Jun 20 2017, 9:29 AM
phuedx updated the task description. (Show Details)Jun 20 2017, 9:32 AM
phuedx updated the task description. (Show Details)Jun 20 2017, 10:25 AM
phuedx updated the task description. (Show Details)Jun 20 2017, 10:28 AM
phuedx added a comment.EditedJun 20 2017, 10:37 AM

I've updated MobileOptionsTracking schema to reflect that the images property is no longer required.

After @Jdlrobson's changes will all be fully deployed on Thursday, 22nd June, we can remove the property from the schema.

This can be moved to "todo" around 3pm PST tomorrow.

Change 361643 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/MobileFrontend@master] i13n: Update MobileOptionsTracking schema rev

https://gerrit.wikimedia.org/r/361643

Merging and deploying @ema's change is tracked in T168013: Remove disableImages handling from VCL so this can be moved to Needs Signoff after rEMFRd02db5547c4e: i13n: Update MobileOptionsTracking schema rev has been merged.

Change 361643 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] i13n: Update MobileOptionsTracking schema rev

https://gerrit.wikimedia.org/r/361643

phuedx removed phuedx as the assignee of this task.Jun 29 2017, 8:30 AM
ovasileva closed this task as Resolved.Jun 29 2017, 9:41 AM
ovasileva claimed this task.

Testing criteria

  1. go to: https://en.m.wikipedia.org/w/index.php?title=Main_Page&mobileaction=toggle_view_mobile
  2. select "settings" from menu
  3. ensure disable images is not available (only avaialble feature is beta)

confirmed.

Change 359417 merged by Ema:
[operations/puppet@production] VCL: remove disableImages handling

https://gerrit.wikimedia.org/r/359417

TheDJ added a subscriber: TheDJ.Jul 4 2017, 7:25 PM

BTW. If we ever want this feature to return, we could maybe do it client side with a service worker. I know @Jdlrobson looked at that for lazy loaded images at some point, that's basically the same usecase of course. Or we can just make it a client side option that influences the current JS lazy loader.