Page MenuHomePhabricator

Toolbar API: Unnecessary cookies are stored when inserting a booklet
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Delete all wikiEditor-* cookies.
  2. Run the test script (./js/tests/wikiEditor.toolbar.js).
  3. Look at the created cookies:

Two cookies are created, wikiEditor-0-booklet-colors-page and wikiEditor-0-booklet-removeme-page, both with value "colors" (which is the first page in the booklet containing the pages "colors" and "removeme").

Expected behavior:

Only one cookie for the opened section (wikiEditor-0-toolbar-section), and one for each booklet (wikiEditor-0-booklet-info-page, in this case) should be created, when the user opens something (this works as expected), but not one cookie for each page.


Version: unspecified
Severity: minor

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:20 PM
bzimport added a project: WikiEditor.
bzimport set Reference to bz25184.
bzimport added a subscriber: Unknown Object (MLST).

Created attachment 8782
Proposed patch

In two places updateBookletSelection was called with the name of the page instead of the name of the section. The patch fixes this.
Additionaly, when only the default (i. e. the first) page is selected, storing this in a cookie isn't necessary, so I removed this, too.

Attached:

sumanah wrote:

Michael, thank you for the patch, and my apologies that no one has reviewed it and responded yet. I'm marking this bug with "need-review" as a keyword, to signal that this patch is awaiting code review. Thanks again. If you want to talk about your patch and get it reviewed faster, please feel free to visit MediaWiki-General in freenode IRC -- http://webchat.freenode.net/?channels=#mediawiki .

sumanah wrote:

Michael: Thanks again for the patch. Are you interested in using developer access to directly suggest it into our Git source control system?

https://www.mediawiki.org/wiki/Developer_access

https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch

This is still reproducible, but since the original test file was removed, you have to use some other script that modifies the booklets. For example, you can go to de.wikipedia and execute

importScript('Benutzer:Schnark/js/wikieditor.js');

in the console. This script will add some additional booklets. Note that a lot of stupid cookies are stored, and that reloading the page (and re-executing the script) will not keep the booklet open, but will fall back to the first one.

My patch from above still fixes the issues, I'll upload an updated version using Gerrit Patch Uploader.

Change 225014 had a related patch set uploaded (by Gerrit Patch Uploader):
WikiEditor shouldn't store wrong cookies for booklets

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

Curious; I'm wondering if the 'deferLoad': true, line -- found in the parts that build the character & help booklets in jquery.wikiEditor.toolbar.config.js -- is somehow screwing-up or superseding the cookie assignment thing as well as retarding the manual addition/removal of individual booklet Indexes &/or Pages (see T70791)?

That you can't modify things that are marked as deferLoad is a different issue (and by the way, there is also T25479 for this).

Change 225014 merged by jenkins-bot:
WikiEditor shouldn't store wrong cookies for booklets

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

Schnark claimed this task.