Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework: Optimize decodeEntities to skip unnecessary decoding #9725

Merged
merged 2 commits into from
Nov 30, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Nov 30, 2016

This pull request seeks to replace the current decodeEntities implementation with a much simpler alternative, with the hopes of improving performance. decodeEntities is a top culprit in profiling page transitions in Calypso, often the top time-consuming function in Chrome Timeline reports:

Before After
decodeEntities before decodeEntities after

(See earlier implementation results)

Implementation notes:

With these changes, decodeEntities will no longer decode all entities, only those which we expect to have been encoded by the WordPress _wp_specialchars method. In other words, it should now mimic the behavior of wp_specialchars_decode.

There was previously a test that expected entities like Ñ (Ñ) to have been decoded for plugin authors, but in my real-world testing, I find that these entities are never encoded to begin with and continue to display correctly. You can verify this yourself with an example plugin which specifies an author name including both "&" and "Ñ" characters. On the plugin detail screen, the author name shows correctly as "Andrew Duthie Ñáð & Friends". cc @enejb re: 6123-gh-calypso-pre-oss

Another minor optimization inspired by the wp_specialchars_decode implementation is to skip replacement attempts if there are no entities in the text argument, detected by presence of a & character.

Edit: After further review, it was determined that we rely on full entity decoding in many places (Reader, post titles with user-entered entities such as •). The changes here are limited to the above-noted inspired & check optimization, and memoization of the title-generating selector which makes use of decodeEntities on every screen.

Testing instructions:

Verify that wherever content is rendered using REST API data where text would be encoded (e.g. post titles, assigned categories), that the text continues to display as decoded. This should include expected encoded values & < > ' " as well as other special characters that are never encoded Ñ á ð.

  1. Navigate to the post editor
  2. Select a site, if prompted
  3. Enter title "Ñáð & Friends"
  4. Save post
  5. Return to posts listing
  6. Note that draft title is rendered exactly as "Ñáð & Friends"

Open questions:

Is there anywhere in the codebase that we're expecting decodeEntities to decode more than just the entities that would have been encoded by WordPress core logic?

cc @blowery

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 30, 2016
@matticbot
Copy link
Contributor

@aduth aduth changed the title Framework: Simplify decodeEntities to replace only WordPress-encoded entities Framework: Optimize decodeEntities to skip unnecessary decoding Nov 30, 2016
@aduth
Copy link
Contributor Author

aduth commented Nov 30, 2016

Noting reference to original implementation, since I'll plan to rebase it to the void: f4161484955c363c1e1219320bb09727de265b6d

@blowery
Copy link
Contributor

blowery commented Nov 30, 2016

Beautiful. 🚢

@blowery blowery added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants