From 57e2f4990d4a422c8ad38b64ed91b53fc92161bc Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 26 Nov 2018 09:21:38 +0800 Subject: [PATCH] PERF: Move processing of inline onebox out of V8 context. (#6658) --- .../components/composer-editor.js.es6 | 10 +- .../engines/discourse-markdown/onebox.js.es6 | 28 ++- ...oxer.js.es6 => inline-oneboxer.js.es6.erb} | 3 + .../pretty-text/pretty-text.js.es6 | 8 +- .../pretty-text/white-lister.js.es6 | 4 +- lib/cooked_post_processor.rb | 18 ++ lib/pretty_text.rb | 6 - lib/pretty_text/helpers.rb | 4 - lib/pretty_text/shims.js | 10 -- spec/components/cooked_post_processor_spec.rb | 166 ++++++++++++++++-- spec/components/pretty_text_spec.rb | 69 -------- test/javascripts/lib/pretty-text-test.js.es6 | 17 +- 12 files changed, 201 insertions(+), 142 deletions(-) rename app/assets/javascripts/pretty-text/{inline-oneboxer.js.es6 => inline-oneboxer.js.es6.erb} (84%) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index ce9b6a5f6e4..090e75746af 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -36,6 +36,8 @@ import { resolveAllShortUrls } from "pretty-text/image-short-url"; +import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer"; + const REBUILD_SCROLL_MAP_EVENTS = ["composer:resized", "composer:typed-reply"]; const uploadHandlers = []; @@ -993,10 +995,10 @@ export default Ember.Component.extend({ resolveAllShortUrls(ajax); let inline = {}; - $("a.inline-onebox-loading", $preview).each(function(index, link) { - let $link = $(link); - $link.removeClass("inline-onebox-loading"); - let text = $link.text(); + $(`a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`, $preview).each((index, link) => { + const $link = $(link); + $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS); + const text = $link.text(); inline[text] = inline[text] || []; inline[text].push($link); }); diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 index 8306026be78..f43798897a4 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 @@ -1,5 +1,9 @@ import { lookupCache } from "pretty-text/oneboxer"; -import { cachedInlineOnebox } from "pretty-text/inline-oneboxer"; + +import { + cachedInlineOnebox, + INLINE_ONEBOX_LOADING_CSS_CLASS +} from "pretty-text/inline-oneboxer"; const ONEBOX = 1; const INLINE = 2; @@ -94,23 +98,13 @@ function applyOnebox(state, silent) { attrs.push(["class", "onebox"]); attrs.push(["target", "_blank"]); } - } else if (mode === INLINE) { - if (!isTopLevel(href)) { - let onebox = cachedInlineOnebox(href); - let options = state.md.options.discourse; + } else if (mode === INLINE && !isTopLevel(href)) { + const onebox = cachedInlineOnebox(href); - if (options.lookupInlineOnebox) { - onebox = options.lookupInlineOnebox( - href, - options.invalidateOneboxes - ); - } - - if (onebox && onebox.title) { - text.content = onebox.title; - } else if (state.md.options.discourse.previewing && !onebox) { - attrs.push(["class", "inline-onebox-loading"]); - } + if (onebox && onebox.title) { + text.content = onebox.title; + } else { + attrs.push(["class", INLINE_ONEBOX_LOADING_CSS_CLASS]); } } } diff --git a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb similarity index 84% rename from app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 rename to app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb index a7b6a99a896..983549dc4e0 100644 --- a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 +++ b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb @@ -1,5 +1,8 @@ let _cache = {}; +export const INLINE_ONEBOX_LOADING_CSS_CLASS = + "<%= CookedPostProcessor::INLINE_ONEBOX_LOADING_CSS_CLASS %>"; + export function applyInlineOneboxes(inline, ajax) { Object.keys(inline).forEach(url => { // cache a blank locally, so we never trigger a lookup diff --git a/app/assets/javascripts/pretty-text/pretty-text.js.es6 b/app/assets/javascripts/pretty-text/pretty-text.js.es6 index 410a47e5a18..da572d891ad 100644 --- a/app/assets/javascripts/pretty-text/pretty-text.js.es6 +++ b/app/assets/javascripts/pretty-text/pretty-text.js.es6 @@ -26,12 +26,10 @@ export function buildOptions(state) { lookupPrimaryUserGroupByPostNumber, formatUsername, emojiUnicodeReplacer, - lookupInlineOnebox, lookupImageUrls, previewing, linkify, - censoredWords, - invalidateOneboxes + censoredWords } = state; let features = { @@ -67,7 +65,6 @@ export function buildOptions(state) { lookupPrimaryUserGroupByPostNumber, formatUsername, emojiUnicodeReplacer, - lookupInlineOnebox, lookupImageUrls, censoredWords, allowedHrefSchemes: siteSettings.allowed_href_schemes @@ -79,8 +76,7 @@ export function buildOptions(state) { markdownIt: true, injectLineNumbersToPreview: siteSettings.enable_advanced_editor_preview_sync, - previewing, - invalidateOneboxes + previewing }; // note, this will mutate options due to the way the API is designed diff --git a/app/assets/javascripts/pretty-text/white-lister.js.es6 b/app/assets/javascripts/pretty-text/white-lister.js.es6 index 0ec00398b06..4e1ed92f01c 100644 --- a/app/assets/javascripts/pretty-text/white-lister.js.es6 +++ b/app/assets/javascripts/pretty-text/white-lister.js.es6 @@ -1,3 +1,5 @@ +import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer"; + // to match: // abcd // abcd[test] @@ -116,7 +118,7 @@ const DEFAULT_LIST = [ "a.mention", "a.mention-group", "a.onebox", - "a.inline-onebox-loading", + `a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`, "a[data-bbcode]", "a[name]", "a[rel=nofollow]", diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 6a343bee753..6a604a32636 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -33,6 +33,7 @@ class CookedPostProcessor DistributedMutex.synchronize("post_process_#{@post.id}") do DiscourseEvent.trigger(:before_post_process_cooked, @doc, @post) post_process_oneboxes + post_process_inline_oneboxes post_process_images post_process_quotes optimize_urls @@ -575,8 +576,25 @@ class CookedPostProcessor @doc.try(:to_html) end + INLINE_ONEBOX_LOADING_CSS_CLASS = "inline-onebox-loading" + private + def post_process_inline_oneboxes + @doc.css(".#{INLINE_ONEBOX_LOADING_CSS_CLASS}").each do |element| + inline_onebox = InlineOneboxer.lookup( + element.attributes["href"].value, + invalidate: !!@opts[:invalidate_oneboxes] + ) + + if title = inline_onebox&.dig(:title) + element.children = title + end + + element.remove_class(INLINE_ONEBOX_LOADING_CSS_CLASS) + end + end + def is_svg?(img) path = begin diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index 22970f8d187..30bb0c0ee35 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -158,7 +158,6 @@ module PrettyText __optInput.categoryHashtagLookup = __categoryLookup; __optInput.customEmoji = #{custom_emoji.to_json}; __optInput.emojiUnicodeReplacer = __emojiUnicodeReplacer; - __optInput.lookupInlineOnebox = __lookupInlineOnebox; __optInput.lookupImageUrls = __lookupImageUrls; __optInput.censoredWords = #{WordWatcher.words_for_action(:censor).join('|').to_json}; JS @@ -171,12 +170,7 @@ module PrettyText buffer << "__optInput.userId = #{opts[:user_id].to_i};\n" end - if opts[:invalidate_oneboxes] - buffer << "__optInput.invalidateOneboxes = true;\n" - end - buffer << "__textOptions = __buildOptions(__optInput);\n" - buffer << ("__pt = new __PrettyText(__textOptions);") # Be careful disabling sanitization. We allow for custom emails diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb index 05df0cc18c8..ecdf284e248 100644 --- a/lib/pretty_text/helpers.rb +++ b/lib/pretty_text/helpers.rb @@ -76,10 +76,6 @@ module PrettyText result end - def lookup_inline_onebox(url, opts = {}) - InlineOneboxer.lookup(url, opts) - end - def get_topic_info(topic_id) return unless topic_id.is_a?(Integer) # TODO this only handles public topics, secured one do not get this diff --git a/lib/pretty_text/shims.js b/lib/pretty_text/shims.js index b2ce2cd7469..649c3e4b39c 100644 --- a/lib/pretty_text/shims.js +++ b/lib/pretty_text/shims.js @@ -64,16 +64,6 @@ function __getURL(url) { return url; } -function __lookupInlineOnebox(url, invalidate = false) { - const opts = {}; - - if (invalidate) { - opts["invalidate"] = true; - } - - return __helpers.lookup_inline_onebox(url, opts); -} - function __lookupImageUrls(urls) { return __helpers.lookup_image_urls(urls); } diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index 98aea3515f0..6ad6b486d3c 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -3,7 +3,7 @@ require "cooked_post_processor" describe CookedPostProcessor do - context ".post_process" do + context "#post_process" do let(:upload) do Fabricate(:upload, url: '/uploads/default/original/1X/1/1234567890123456.jpg' @@ -21,6 +21,7 @@ describe CookedPostProcessor do it "post process in sequence" do cpp.expects(:post_process_oneboxes).in_sequence(post_process) + cpp.expects(:post_process_inline_oneboxes).in_sequence(post_process) cpp.expects(:post_process_images).in_sequence(post_process) cpp.expects(:optimize_urls).in_sequence(post_process) cpp.expects(:pull_hotlinked_images).in_sequence(post_process) @@ -29,6 +30,137 @@ describe CookedPostProcessor do expect(PostUpload.exists?(post: post, upload: upload)).to eq(true) end + describe 'when post contains inline oneboxes' do + let(:loading_css_class) do + described_class::INLINE_ONEBOX_LOADING_CSS_CLASS + end + + before do + SiteSetting.enable_inline_onebox_on_all_domains = true + end + + describe 'internal links' do + let(:topic) { Fabricate(:topic) } + let(:url) { topic.url } + let(:post) { Fabricate(:post, raw: "Hello #{url}") } + + it "includes the topic title" do + cpp.post_process + + expect(cpp.html).to have_tag('a', + with: { + href: UrlHelper.cook_url(url) + }, + without: { + class: loading_css_class + }, + text: topic.title, + count: 1 + ) + + topic.update!(title: "Updated to something else") + cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true) + cpp.post_process + + expect(cpp.html).to have_tag('a', + with: { + href: UrlHelper.cook_url(url) + }, + without: { + class: loading_css_class + }, + text: topic.title, + count: 1 + ) + end + end + + describe 'external links' do + let(:url_with_path) do + 'https://meta.discourse.org/t/mini-inline-onebox-support-rfc/66400' + end + + let(:url_with_query_param) do + 'https://meta.discourse.org?a' + end + + let(:url_no_path) do + 'https://meta.discourse.org/' + end + + let(:urls) do + [ + url_with_path, + url_with_query_param, + url_no_path + ] + end + + let(:title) { 'some title' } + + let(:post) do + Fabricate(:post, raw: <<~RAW) + This is a #{url_with_path} topic + This should not be inline #{url_no_path} oneboxed + + - #{url_with_path} + + + - #{url_with_query_param} + RAW + end + + before do + urls.each do |url| + stub_request(:get, url).to_return( + status: 200, + body: "#{title}" + ) + end + end + + after do + urls.each { |url| InlineOneboxer.purge(url) } + end + + it 'should convert the right links to inline oneboxes' do + cpp.post_process + html = cpp.html + + expect(html).to_not have_tag('a', + with: { + href: url_no_path + }, + without: { + class: loading_css_class + }, + text: title + ) + + expect(html).to have_tag('a', + with: { + href: url_with_path + }, + without: { + class: loading_css_class + }, + text: title, + count: 2 + ) + + expect(html).to have_tag('a', + with: { + href: url_with_query_param + }, + without: { + class: loading_css_class + }, + text: title, + count: 1 + ) + end + end + end end context "cooking options" do @@ -51,7 +183,7 @@ describe CookedPostProcessor do end end - context ".post_process_images" do + context "#post_process_images" do before do SiteSetting.responsive_post_image_sizes = "" @@ -389,7 +521,7 @@ describe CookedPostProcessor do end - context ".extract_images" do + context "#extract_images" do let(:post) { build(:post_with_plenty_of_images) } let(:cpp) { CookedPostProcessor.new(post) } @@ -400,7 +532,7 @@ describe CookedPostProcessor do end - context ".get_size_from_attributes" do + context "#get_size_from_attributes" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } @@ -437,7 +569,7 @@ describe CookedPostProcessor do end - context ".get_size_from_image_sizes" do + context "#get_size_from_image_sizes" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } @@ -449,7 +581,7 @@ describe CookedPostProcessor do end - context ".get_size" do + context "#get_size" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } @@ -500,7 +632,7 @@ describe CookedPostProcessor do end - context ".is_valid_image_url?" do + context "#is_valid_image_url?" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } @@ -523,7 +655,7 @@ describe CookedPostProcessor do end - context ".get_filename" do + context "#get_filename" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } @@ -544,8 +676,7 @@ describe CookedPostProcessor do end - context ".post_process_oneboxes" do - + context "#post_process_oneboxes" do let(:post) { build(:post_with_youtube, id: 123) } let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) } @@ -553,6 +684,7 @@ describe CookedPostProcessor do Oneboxer.expects(:onebox) .with("http://www.youtube.com/watch?v=9bZkp7q19f0", invalidate_oneboxes: true, user_id: nil, category_id: post.topic.category_id) .returns("
GANGNAM STYLE
") + cpp.post_process_oneboxes end @@ -598,7 +730,7 @@ describe CookedPostProcessor do end end - context ".post_process_oneboxes removes nofollow if add_rel_nofollow_to_user_content is disabled" do + context "#post_process_oneboxes removes nofollow if add_rel_nofollow_to_user_content is disabled" do let(:post) { build(:post_with_youtube, id: 123) } let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) } @@ -616,7 +748,7 @@ describe CookedPostProcessor do end end - context ".post_process_oneboxes with square image" do + context "#post_process_oneboxes with square image" do it "generates a onebox-avatar class" do SiteSetting.crawl_images = true @@ -652,7 +784,7 @@ describe CookedPostProcessor do end - context ".optimize_urls" do + context "#optimize_urls" do let(:post) { build(:post_with_uploads_and_links) } let(:cpp) { CookedPostProcessor.new(post) } @@ -729,7 +861,7 @@ describe CookedPostProcessor do end - context ".pull_hotlinked_images" do + context "#pull_hotlinked_images" do let(:post) { build(:post, created_at: 20.days.ago) } let(:cpp) { CookedPostProcessor.new(post) } @@ -784,7 +916,7 @@ describe CookedPostProcessor do end - context ".disable_if_low_on_disk_space" do + context "#disable_if_low_on_disk_space" do let(:post) { build(:post, created_at: 20.days.ago) } let(:cpp) { CookedPostProcessor.new(post) } @@ -812,7 +944,7 @@ describe CookedPostProcessor do end - context ".download_remote_images_max_days_old" do + context "#download_remote_images_max_days_old" do let(:post) { build(:post, created_at: 20.days.ago) } let(:cpp) { CookedPostProcessor.new(post) } @@ -840,7 +972,7 @@ describe CookedPostProcessor do end end - context ".is_a_hyperlink?" do + context "#is_a_hyperlink?" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb index 8a020d6b93d..36358f5e5ea 100644 --- a/spec/components/pretty_text_spec.rb +++ b/spec/components/pretty_text_spec.rb @@ -1020,42 +1020,6 @@ HTML expect(PrettyText.cook("\nhttp://a.com")).to include('onebox') end - it 'handles mini onebox' do - SiteSetting.enable_inline_onebox_on_all_domains = true - InlineOneboxer.purge("http://cnn.com/a") - - stub_request(:get, "http://cnn.com/a"). - to_return(status: 200, body: "news") - - expect(PrettyText.cook("- http://cnn.com/a\n- a http://cnn.com/a").split("news").length).to eq(3) - expect(PrettyText.cook("- http://cnn.com/a\n - a http://cnn.com/a").split("news").length).to eq(3) - end - - it 'handles mini onebox with query param' do - SiteSetting.enable_inline_onebox_on_all_domains = true - InlineOneboxer.purge("http://cnn.com?a") - - stub_request(:get, "http://cnn.com?a"). - to_return(status: 200, body: "news") - - expect(PrettyText.cook("- http://cnn.com?a\n- a http://cnn.com?a").split("news").length).to eq(3) - expect(PrettyText.cook("- http://cnn.com?a\n - a http://cnn.com?a").split("news").length).to eq(3) - end - - it 'skips mini onebox for primary domain' do - - # we only include mini onebox if there is something in path or query params - - SiteSetting.enable_inline_onebox_on_all_domains = true - InlineOneboxer.purge("http://cnn.com/") - - stub_request(:get, "http://cnn.com/"). - to_return(status: 200, body: "news") - - expect(PrettyText.cook("- http://cnn.com/\n- a http://cnn.com/").split("news").length).to eq(1) - expect(PrettyText.cook("- cnn.com\n - a http://cnn.com/").split("news").length).to eq(1) - end - it "can handle bbcode" do expect(PrettyText.cook("a[b]b[/b]c")).to eq('

abc

') expect(PrettyText.cook("a[i]b[/i]c")).to eq('

abc

') @@ -1232,39 +1196,6 @@ HTML end - describe "inline onebox" do - it "includes the topic title" do - topic = Fabricate(:topic) - - raw = "Hello #{topic.url}" - - cooked = <<~HTML -

Hello #{topic.title}

- HTML - - expect(PrettyText.cook(raw)).to eq(cooked.strip) - end - - it "invalidates the onebox url" do - topic = Fabricate(:topic) - url = topic.url - raw = "Hello #{url}" - - PrettyText.cook(raw) - - topic.title = "Updated: #{topic.title}" - topic.save - - cooked = <<~HTML -

Hello #{topic.title}

- HTML - - expect(PrettyText.cook(raw)).not_to eq(cooked.strip) - expect(PrettyText.cook(raw, invalidate_oneboxes: true)).to eq(cooked.strip) - expect(PrettyText.cook(raw)).to eq(cooked.strip) - end - end - describe "image decoding" do it "can decode upload:// for default setup" do diff --git a/test/javascripts/lib/pretty-text-test.js.es6 b/test/javascripts/lib/pretty-text-test.js.es6 index f929945de14..000086955f6 100644 --- a/test/javascripts/lib/pretty-text-test.js.es6 +++ b/test/javascripts/lib/pretty-text-test.js.es6 @@ -2,6 +2,7 @@ import Quote from "discourse/lib/quote"; import Post from "discourse/models/post"; import { default as PrettyText, buildOptions } from "pretty-text/pretty-text"; import { IMAGE_VERSION as v } from "pretty-text/emoji"; +import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer"; QUnit.module("lib:pretty-text"); @@ -197,13 +198,13 @@ QUnit.test("Links", assert => { assert.cooked( "Youtube: http://www.youtube.com/watch?v=1MrpeBRkM5A", - '

Youtube: http://www.youtube.com/watch?v=1MrpeBRkM5A

', + `

Youtube: http://www.youtube.com/watch?v=1MrpeBRkM5A

`, "allows links to contain query params" ); assert.cooked( "Derpy: http://derp.com?__test=1", - '

Derpy: http://derp.com?__test=1

', + `

Derpy: http://derp.com?__test=1

`, "works with double underscores in urls" ); @@ -233,7 +234,7 @@ QUnit.test("Links", assert => { assert.cooked( "Batman: http://en.wikipedia.org/wiki/The_Dark_Knight_(film)", - '

Batman: http://en.wikipedia.org/wiki/The_Dark_Knight_(film)

', + `

Batman: http://en.wikipedia.org/wiki/The_Dark_Knight_(film)

`, "autolinks a URL with parentheses (like Wikipedia)" ); @@ -245,7 +246,7 @@ QUnit.test("Links", assert => { assert.cooked( "1. View @eviltrout's profile here: http://meta.discourse.org/u/eviltrout/activity
next line.", - '
    \n
  1. View @eviltrout\'s profile here: http://meta.discourse.org/u/eviltrout/activity
    next line.
    2. \n
', + `
    \n
  1. View @eviltrout\'s profile here: http://meta.discourse.org/u/eviltrout/activity
    next line.
    2. \n
`, "allows autolinking within a list without inserting a paragraph." ); @@ -270,8 +271,8 @@ QUnit.test("Links", assert => { assert.cooked( "http://discourse.org and http://discourse.org/another_url and http://www.imdb.com/name/nm2225369", '

http://discourse.org and ' + - 'http://discourse.org/another_url and ' + - 'http://www.imdb.com/name/nm2225369

', + `http://discourse.org/another_url and ` + + `http://www.imdb.com/name/nm2225369

`, "allows multiple links on one line" ); @@ -705,13 +706,13 @@ QUnit.test("Oneboxing", assert => { assert.ok( !matches( "- http://www.textfiles.com/bbs/MINDVOX/FORUMS/ethics\n\n- http://drupal.org", - /onebox/ + /class="onebox"/ ), "doesn't onebox a link within a list" ); assert.ok( - matches("http://test.com", /onebox/), + matches("http://test.com", /class="onebox"/), "adds a onebox class to a link on its own line" ); assert.ok(