PERF: Move processing of inline onebox out of V8 context. (#6658)

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) {
+      $(`a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`, $preview).each((index, link) => {
-        let $link = $(link);
+        const $link = $(link);
-        $link.removeClass("inline-onebox-loading");
+        $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
-        let text = $link.text();
+        const text = $link.text();
         inline[text] = inline[text] || [];
         inline[text].push($link);
       });

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) {
+          } else if (mode === INLINE && !isTopLevel(href)) {
-            if (!isTopLevel(href)) {
+            const onebox = cachedInlineOnebox(href);
-              let onebox = cachedInlineOnebox(href);
-              let options = state.md.options.discourse;
 
-              if (options.lookupInlineOnebox) {
+            if (onebox && onebox.title) {
-                onebox = options.lookupInlineOnebox(
+              text.content = onebox.title;
-                  href,
+            } else {
-                  options.invalidateOneboxes
+              attrs.push(["class", INLINE_ONEBOX_LOADING_CSS_CLASS]);
-                );
-              }
-
-              if (onebox && onebox.title) {
-                text.content = onebox.title;
-              } else if (state.md.options.discourse.previewing && !onebox) {
-                attrs.push(["class", "inline-onebox-loading"]);
-              }
             }
           }
         }

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

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,
+    censoredWords
-    invalidateOneboxes
   } = 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,
+    previewing
-    invalidateOneboxes
   };
 
   // note, this will mutate options due to the way the API is designed

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]",

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

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

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

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);
 }

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: "<html><head><title>#{title}</title></head></html>"
+            )
+          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("<div>GANGNAM STYLE</div>")
+
       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) }

spec/components/pretty_text_spec.rb

@@ -1020,42 +1020,6 @@ HTML
     expect(PrettyText.cook("<img src='a'>\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: "<html><head><title>news</title></head></html>")
-
-    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: "<html><head><title>news</title></head></html>")
-
-    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: "<html><head><title>news</title></head></html>")
-
-    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('<p>a<span class="bbcode-b">b</span>c</p>')
     expect(PrettyText.cook("a[i]b[/i]c")).to eq('<p>a<span class="bbcode-i">b</span>c</p>')
@@ -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
-        <p>Hello <a href="#{topic.url}">#{topic.title}</a></p>
-      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
-        <p>Hello <a href="#{url}">#{topic.title}</a></p>
-      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

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",
-    '<p>Youtube: <a href="http://www.youtube.com/watch?v=1MrpeBRkM5A">http://www.youtube.com/watch?v=1MrpeBRkM5A</a></p>',
+    `<p>Youtube: <a href="http://www.youtube.com/watch?v=1MrpeBRkM5A" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://www.youtube.com/watch?v=1MrpeBRkM5A</a></p>`,
     "allows links to contain query params"
   );
 
   assert.cooked(
     "Derpy: http://derp.com?__test=1",
-    '<p>Derpy: <a href="http://derp.com?__test=1">http://derp.com?__test=1</a></p>',
+    `<p>Derpy: <a href="http://derp.com?__test=1" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://derp.com?__test=1</a></p>`,
     "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)",
-    '<p>Batman: <a href="http://en.wikipedia.org/wiki/The_Dark_Knight_(film)">http://en.wikipedia.org/wiki/The_Dark_Knight_(film)</a></p>',
+    `<p>Batman: <a href="http://en.wikipedia.org/wiki/The_Dark_Knight_(film)" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://en.wikipedia.org/wiki/The_Dark_Knight_(film)</a></p>`,
     "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<br/>next line.",
-    '<ol>\n<li>View <span class="mention">@eviltrout</span>\'s profile here: <a href="http://meta.discourse.org/u/eviltrout/activity">http://meta.discourse.org/u/eviltrout/activity</a><br>next line.</li>\n</ol>',
+    `<ol>\n<li>View <span class="mention">@eviltrout</span>\'s profile here: <a href="http://meta.discourse.org/u/eviltrout/activity" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://meta.discourse.org/u/eviltrout/activity</a><br>next line.</li>\n</ol>`,
     "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",
     '<p><a href="http://discourse.org">http://discourse.org</a> and ' +
-      '<a href="http://discourse.org/another_url">http://discourse.org/another_url</a> and ' +
+      `<a href="http://discourse.org/another_url" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://discourse.org/another_url</a> and ` +
-      '<a href="http://www.imdb.com/name/nm2225369">http://www.imdb.com/name/nm2225369</a></p>',
+      `<a href="http://www.imdb.com/name/nm2225369" class="${INLINE_ONEBOX_LOADING_CSS_CLASS}">http://www.imdb.com/name/nm2225369</a></p>`,
     "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(