Page MenuHomePhabricator

Page Previews should only process links that the user dwells on
Closed, ResolvedPublic3 Story Points

Description

Background

Paraphrasing T164256#3243278:

When PP boots, it processes every link in the page's content by:

  1. Getting the list of whitelisted links in the page's content;
  2. Filtering the links by namespace (by creating an instance of mw.Title via mw.Title.newFromText); and
  3. Storing the associated title using jQuery#data for retrieval later.

The result is that we do a lot of processing for links that the user is unlikely to interact with [0]. Moreover, because the filtering is done upfront, we have to create event handlers for each filtered element, rather than one top-level event handler bound to the container.

Value statement

After this change, PP will boot faster on longer pages. Indeed, PP boot time will be constant across all pages.

AC

  1. A top-level event handler is used to handle dwell and click events.
  2. The "dynamic" filtering is done on a per-interaction basis.
  3. We no longer store mw.Title objects using jQuery#data.

Psuedocode

src/index.js
const CSS_BLACKLIST = [];

mw.hook( 'wikipage.content', ( $container ) => {
  // "Static" filtering
  $container.on( 'hover', 'a:not(' + CSS_BLACKLIST.join(',') + ')', ( event ) => {

    // "Dynamic" filtering
    const el = event.target,
      title = getLinkTitle( el );

   if ( title && canHandleTitle( title ) ) {
     boundActions.linkDwell( el, title );
   }
  } );
} );

[0]
select
  avg(nInteractionsPerPage) as 'IPP'
from
(
  select
    count(*) as nInteractionsPerPage
  from log.Popups_16364296
  where
    timestamp like '20170516%'
    and event_linkInteractionToken is not null
  group by event_pageToken
) as interactionsPerPage;

+--------+
| IPP    |
+--------+
| 9.9246 |
+--------+

Event Timeline

phuedx created this task.May 17 2017, 10:00 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 17 2017, 10:00 AM
phuedx updated the task description. (Show Details)May 17 2017, 10:12 AM

Note that from the pseudocode canHandle would be changing processLinks.js and the tests to something like canHandle or shouldShowPreview.js, and make it a function that gets a title or element and returns true or false by running the conditions on the .filter().

LGTM

And getLinkTitle already exists as ./src/getTitle.js.

Jdlrobson added subscribers: ovasileva, Jdlrobson.

@phuedx can you add a value statement that will help @ovasileva understand the impact here and set a priority for this?

ovasileva triaged this task as High priority.May 21 2017, 11:28 AM

@phuedx can you add a value statement that will help @ovasileva understand the impact here and set a priority for this?

@Jdlrobson, @ovasileva: I see that this task was marked as high priority soon after you asked for a value statement (T165572#3281881). I'm not sure what changed in the interim…

Anyway: at the very least, this'll make PP boot faster on longer pages. Indeed, it'll make boot time the same across all pages.

Jdlrobson updated the task description. (Show Details)May 22 2017, 6:51 AM
phuedx updated the task description. (Show Details)May 22 2017, 6:51 AM
ovasileva set the point value for this task to 3.May 23 2017, 4:53 PM

Change 357831 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Rename getTitle.js to title.js

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

Change 357832 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Add title#isValid

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

Change 357833 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Use delegated events in container

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

Change 357834 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Remove unused files processLinks{,.test}.js

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

Change 357831 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Rename getTitle.js to title.js

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

@Jhernandez I've left some questions on your patches.

Change 357832 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Add title#isValid

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

Change 357833 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Use delegated events in container

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

Change 357834 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Remove unused files processLinks{,.test}.js

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

@bmansurov Re: no dist files on rEPOPb16a6fe7358f: Remove unused files processLinks{,.test}.js the processLinks.js file stopped being required on the previous commit, so when running building on it it removed the file from the bundle. So by this commit, it was already removed from the compiled bundle.

I submitted the deletion of the files as a separate commit because I didn't want to bloat more the previous diff and I like smaller commits.

Thanks, makes sense.

phuedx closed this task as Resolved.Jun 12 2017, 11:11 AM

💥💥💥