Page MenuHomePhabricator

Make MobileContext modular by creating the DeviceDetector service
Closed, ResolvedPublic5 Story Points

Description

DeviceDetector is a provisional name.


The MobileContext#getDevice, #isMobileDevice, #getAMF, and the DeviceDetection, DeviceProperties, and HtmlDeviceProperties classes are the part of the MobileFrontend codebase that guess a device's form factor.

Briefly, the Device* classes are used to guess the form factor given the User-Agent header and the Accept header. The HtmlDeviceProperties is stub class that represents a mobile device. The MobileContext methods share some of the responsibility of guessing at the device's form factor, which stops us from refining (if possible) the design of the Device* classes.

AC

  • The MobileContext class no longer bears responsibility for guessing at the device's form factor
  • There exists a MobileFrontend.DeviceDetector service that is responsible for guessing at the device's form factor

Proposed Plan (YMMV)

  • Create the MobileFrontend.DeviceDetector service and corresponding class ("service class"), which answers the following questions:
    • does the device have a mobile form factor?
    • does the device have a tablet form factor?
  • Review the design of the Device* classes and see whether they answer those questions directly
  • Integrate MobileContext#getDevice and #getAMF with the service class, removing MobileContext#getDevice and #getAMF
  • Remove MobileContext#isMobileDevice
  • Make MobileContext depend on the service class and delegate to it when guessing at the device's form factor

Test Plan

Either on our staging server or on your development wiki:

  • Enable device detection, i.e. add $wgMFAutodetectMobileView = true; to LocalSetttings.php and:
    • Browse to any page on the wiki using a desktop UA and you shouldn't see the Minerva skin.
    • Browse to a page on the wiki using a mobile UA and you should see the Minerva skin.
    • Browse to a page on the wiki using a tablet UA and you should see the Minerva skin.
    • Browse to a page with the X-Subdomain header set to M and you should see the Minerva skin.
  • Disable device detection, i.e. add $wgMFAutodetectMobileView = false; to LocalSettings.php and:
    • Browse to any page on the wiki using any UA and you shouldn't see the Minerva skin.
    • Browser to a page with the X-Subdomain header set to M and you shouldn't see the Minerva skin.

Event Timeline

phuedx created this task.Aug 25 2016, 1:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 25 2016, 1:40 PM
Jdlrobson triaged this task as Normal priority.Aug 25 2016, 5:14 PM
Jdlrobson added a project: Readers-Web-Backlog.
MBinder_WMF renamed this task from Create the DeviceFormFactorDetector service to Make MobileContext modular by creating the DeviceFormFactorDetector service.Aug 25 2016, 8:19 PM
MBinder_WMF moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
phuedx updated the task description. (Show Details)Aug 27 2016, 10:34 AM
phuedx updated the task description. (Show Details)
phuedx renamed this task from Make MobileContext modular by creating the DeviceFormFactorDetector service to Make MobileContext modular by creating the DeviceDetector service.Sep 12 2016, 10:12 AM
phuedx updated the task description. (Show Details)
MBinder_WMF set the point value for this task to 5.Sep 19 2016, 4:38 PM
phuedx claimed this task.Sep 27 2016, 2:10 PM
phuedx moved this task from To Do to Doing on the Reading-Web-Sprint-82-Xpect-Rspec board.

Change 313381 had a related patch set uploaded (by Phuedx):
[WIP] Extract all device detection from MobileContext

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

Change 314000 had a related patch set uploaded (by Phuedx):
Add MobileFrontend.DeviceDetector service

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

Change 314001 had a related patch set uploaded (by Phuedx):
Hygiene: Remove superseded device detection code

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

@jhobs has asked good questions during code review, to which I've yet to respond.

Change 315240 had a related patch set uploaded (by Phuedx):
Hygiene: Update $wgMFMobileHeader default value

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

Change 315499 had a related patch set uploaded (by Phuedx):
Fully test MobileContext#shouldDisplayMobileView

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

phuedx added subscribers: bmansurov, Jdlrobson.

Thanks for your review @bmansurov, @jhobs, and @Jdlrobson.

Change 315499 merged by jenkins-bot:
Fully test MobileContext#shouldDisplayMobileView

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

Change 313381 merged by jenkins-bot:
Extract extra device detection from MobileContext

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

Change 314000 merged by jenkins-bot:
Add DeviceDetectorService class

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

Change 314001 merged by jenkins-bot:
Hygiene: Remove superseded device detection code

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

Change 315240 merged by jenkins-bot:
Hygiene: Update $wgMFMobileHeader default value

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

A/C seems met @phuedx - is it worth adding a test plan or should this go straight for developer sign off?

phuedx updated the task description. (Show Details)Oct 17 2016, 9:10 AM

@Jdlrobson: I've added a test plan for a developer to run through.

bmansurov closed this task as Resolved.Oct 18 2016, 7:27 PM

Followed the test plan - everything worked as described.