機能追加(4/4): 短縮URLの展開機能を追加 #223

awazi · 2016-11-18T15:08:53Z

短縮URLの展開機能の追加です。
対象となるサイトは、t.coを除きヘッダのリクエストでLocation:が返ってくるものを対象にしています。
($ curl -I -i URL (linuxの場合) にて確認)
ただし、youtu.beのようなサムネイルの取得で対応可能と思われるものは含まれていません。

よろしくお願いします。

eru · 2016-11-20T00:44:02Z

😻

eru · 2016-11-20T02:29:14Z

src/ui/ThreadContent.coffee

+          xhr = new XMLHttpRequest()
+          if /// //t\.co/ ///.test(targetUrl)
+            xhr.open("GET", targetUrl)
+            xhr.responseType = "document"


t.coの場合でも、HEADリクエストでlocation取得できるようですが、敢えて分けている理由を教えてください。

$ curl --head -I https://t.co/9fGZDZz3P2 HTTP/1.1 301 Moved Permanently cache-control: private,max-age=300 content-length: 0 date: Sun, 20 Nov 2016 02:26:54 GMT expires: Sun, 20 Nov 2016 02:31:54 GMT location: https://readcrx-2.github.io/read.crx-2/ server: tsa_m set-cookie: muc=63353035-57cf-4443-b3de-4f8f7cfc891a; Expires=Fri, 02 Nov 2018 02:26:54 UTC; Domain=t.co strict-transport-security: max-age=0 x-connection-hash: 4cc500c45ecc8f6263fe0a890c43d822 x-response-time: 102

確認します。

もしHEADだけになるならapp.HTMLを利用してください

t.coの例外処理についてですが、XMLHttpRequest()でHEADリクエストをした場合は、何故かstatus=200でrespons headにlocationがありません。詳しい理由がわからないのでGETリクエストを使用しています。

リクエスト時のUserAgentヘッダーを消すと301が戻ってきました

情報ありがとうございます。
curlコマンドとのヘッダーの違いはUser-AgentとCookieくらいだったので、実験して試そうと思っていました。

eru · 2016-11-20T02:56:35Z

src/ui/ThreadContent.coffee

+        asyncProcessDfds = []
+
+        # 短縮URLの展開と追加
+        expandShortURL = (aSrc, targetUrl) ->


基本的には、スレッドの表示以外では使う機会がなさそうですが、
短縮URLの展開機能として独立させて、core/URL.tsか、core/util.coffeeへ移動したほうがいいと思います。

@S--Minecraft ↑どう（どっちがいい）でしょう？

そうですね
core/URL.tsがいいかなと

eru · 2016-11-20T03:10:24Z

src/ui/ThreadContent.coffee

+        checkShortURL = (tUrl) ->
+          if tmp = /// ^h?ttps?://([\w\-\.]+)/.+ ///.exec(tUrl)
+            return true if shortUrlSites.indexOf(tmp[1]) >= 0
+          return false


可読性が若干損なわれますが、速度優先で正規表現判定一発にしちゃいましょう。
core/URL.ts に移動をお願いします。

了解しました。

S--Minecraft · 2016-11-20T15:58:10Z

src/ui/ThreadContent.coffee

+            expandedURL.setAttribute("short-url", sourceA.href)
+
+          if finalUrl is ""
+            expandedURL.appendChild(document.createTextNode("展開できませんでした。"))


クラスを追加してcssで::afterをつけるほうがよいのでは？

やってみます。

S--Minecraft · 2016-11-20T16:00:16Z

src/ui/ThreadContent.coffee

+            expandedURL.appendChild(document.createTextNode("展開できませんでした。"))
+          else
+            expandedURLLink = document.createElement("a")
+            expandedURLLink.appendChild(document.createTextNode(finalUrl))


expandedURLLink.textContent = finalUrlで

S--Minecraft · 2016-11-20T16:03:40Z

src/ui/ThreadContent.coffee

+          xhr = new XMLHttpRequest()
+          if /// //t\.co/ ///.test(targetUrl)
+            xhr.open("GET", targetUrl)
+            xhr.responseType = "document"


もしHEADだけになるならapp.HTMLを利用してください

S--Minecraft · 2016-11-20T16:09:24Z

src/ui/ThreadContent.coffee

+          )
+        else
+          return d.resolve()
+


app.util.concurrentではだめですか？

既存の方法ですっきりさせたいのですが、短縮URLの展開後のサムネイルの確認などがあり、それぞれの終了時の処理内容など、うまい方法がないか検討中です。

checkUrl = (a) -> d = new $.Deffered() if app.config.get("expand_short_url") isnt "none" if app.url.checkShortURL(a.href) app.url.expandShortURL(a, a.href).done( (a, finalUrl) -> newLink = addExpandedURL(a, finalUrl) if finalUrl d.resolve(a, newLink) else d.resolve(a, a.href) return ) else d.resolve(a, a.href) else d.resolve(a, a.href) return d.promise() app.util.concurrent(@container.querySelectorAll(".message > a:not(.thumbnail):not(.has_thumbnail)"), (a) -> checkUrl(a).done( (a, link) -> return app.ImageReplaceDat.do(a, link) ).done( (a, res, err) -> # addThumbnail関連 ) )

これでどうでしょう？

提案ありがとうございます。
非同期プログラミングに不慣れな身としては何がなんだか。と言った感じですが試してみます。

S--Minecraft · 2016-11-20T16:10:44Z

src/view/config.haml

+          %br
+          %label
+            タイムアウト:
+            %input.direct(type="number" name="expand_short_url_timeout" min="0" max="5000")ms


設定につくらずに固定値でいいのでは？

元々は特にタイムアウトは指定していなかったのですが、時々異常に時間のかかるものがあることからタイムアウトの必要性を感じ追加しました。
正常に終了するまでの時間のばらつきが大きく、適正と思われる値を固定化するのが難しいと感じ設定可能としました。

awazi · 2016-11-23T14:55:56Z

修正しました。

変更箇所
・checkShortURL()とexpandShortURL()をcore/URL.tsに移動
・短縮URLのチェックを正規表現で行うように変更
・XMLHttpRequestを直接呼び出さずにapp.HTTPを使用するように変更
・t.coに対する例外を廃止
・view/index.coffee内にRequestHeaderの監視を追加
・展開に失敗した場合のメッセージをCSSで表示するように変更
・タイムアウトの初期値を変更、及び上限の廃止
・app.HTTPにresponseURLを追加
・1行に複数の短縮URLが存在する場合に展開済みURLの追加順が保証されないため、短縮URLを併記するように変更

補足
・view/index.coffeeにリクエスト・ヘッダーの監視を追加しましたが、他の処理でも使用する可能性を考慮し、<all_urls>にしてあります。問題があれば変更します。
・app.HTTPにresponseURLを追加しましたが、コンパイラがresponseURLを認識しないため代用変数を使用しています。他に方法があれば修正します。
・core/URL.tsに新たに追加した関数をapp.URL.hoge()として呼び出すと実行時エラーが発生するため、app.url.hoge()として呼び出しをしています。
・app.util.concurrentを使用する方法については、サムネイルの追加と短縮URLの展開のどちらかを呼び出すようにすれば大丈夫な気もしますが、展開後のURLについてのサムネイルの追加を考えると難しい気がします。
仮にapp.util.concurrentをクラス化するなどして、処理対象となる配列要素または監視対象となるDefferedオブジェクトを後から追加可能とすればいけそうな気もしますが、追加前に処理が終了してしまった場合に全ての処理が終了したことを保証できなくなってしまいます。何か良いアイデアはありますか？

eru · 2016-11-24T02:54:22Z

src/core/URL.ts

+            "t\.co|tiny\.cc|tinyurl\.com|tl\.gd|tr\.im|trib\.al|" +
+            "url\.ie|urx\.nu|urx2\.nu|urx3\.nu|ur0\.pw|" +
+            "wk\.tk|xrl\.us" +
+          ")\/.+");


これだと毎回正規表現コンパイルされるので、constとしてSHORT_URL_REG = /.../としてください。
57-63行目の様な形で。

あと、.testしか使わないのであれば、関数化せずにそのままtestでもいいかと。

処理効率を優先すれば定数化するのは良い選択だと思いますが、設定画面からの追加・変更の可能性は閉ざされます。よろしいですか？

t.coみたいに特殊な例もあるので、短縮URLは定数でいいと思います。
追加要望あれば都度変更でも対応可能ですし（少なくとも一般的によく使われる短縮URLサービス程度であれば）

了解しました。

S--Minecraft · 2016-11-24T07:47:05Z

src/core/HTTP.ts


        resonseHeaders = Request.parseHTTPHeader(xhr.getAllResponseHeaders());

-        callback(new Response(xhr.status, resonseHeaders, xhr.responseText));
+        callback(new Response(xhr.status, resonseHeaders, xhr.responseText, xhr2.responseURL));


Typescriptのバグっぽいですね microsoft/TypeScript#11510
2.1.3で修正されるみたいなのでとりあえずのところ
一行目に///<reference path="../global.d.ts" />と書いて
global.d.tsに以下の記述をするとコンパイルを通せると思います

namespace XMLHttpRequest { declare var responseURL: string; }

上記の修正をしましたが、以下のエラーが発生します。

src/global.d.ts(7,11): error TS2300: Duplicate identifier 'XMLHttpRequest'.

あ、namespaceじゃなくてinterfaceでした
すいません…

S--Minecraft · 2016-11-24T07:57:09Z

src/view/index.coffee

+
+    return {requestHeaders: details.requestHeaders}
+  ,{
+    urls: ["<all_urls>"],


追加するときに追加すればよいのでは…？

では現状ではt.coのみにします。

S--Minecraft · 2016-11-24T07:58:40Z

src/view/index.coffee

+      return
+
+    # 短縮URLの展開でのt.coに対する例外
+    if details.method is "HEAD" and /// //t\.co/ ///.test(details.url)


app.URL.getDomain(details.url) is "t.co"で

了解しました。

S--Minecraft · 2016-11-24T08:05:24Z

src/ui/ThreadContent.coffee

          )
-        ).always(=>
-          d.resolve()
+          # Audioの確認


元々Audio/VideoがImageReplaceDatの処理後だったのがそうでなくなってます
おそらく817行目のあとにくるべきかと

S--Minecraft · 2016-11-24T08:14:00Z

src/ui/ThreadContent.coffee

+          )
+        else
+          return d.resolve()
+


checkUrl = (a) -> d = new $.Deffered() if app.config.get("expand_short_url") isnt "none" if app.url.checkShortURL(a.href) app.url.expandShortURL(a, a.href).done( (a, finalUrl) -> newLink = addExpandedURL(a, finalUrl) if finalUrl d.resolve(a, newLink) else d.resolve(a, a.href) return ) else d.resolve(a, a.href) else d.resolve(a, a.href) return d.promise() app.util.concurrent(@container.querySelectorAll(".message > a:not(.thumbnail):not(.has_thumbnail)"), (a) -> checkUrl(a).done( (a, link) -> return app.ImageReplaceDat.do(a, link) ).done( (a, res, err) -> # addThumbnail関連 ) )

これでどうでしょう？

awazi · 2016-11-24T12:57:01Z

指摘のあった箇所を修正しました。
ただし、core/HTTP.tsについては別のエラーが発生するため変更していません。
また、提案していただいたapp.util.concurrentの処理については一部修正をしました。

S--Minecraft

app.util.concurrentの件一応テストだけお願いします(パパっと書いたものなので…)

S--Minecraft · 2016-11-24T13:04:24Z

src/core/HTTP.ts


        resonseHeaders = Request.parseHTTPHeader(xhr.getAllResponseHeaders());

-        callback(new Response(xhr.status, resonseHeaders, xhr.responseText));
+        callback(new Response(xhr.status, resonseHeaders, xhr.responseText, xhr2.responseURL));


あ、namespaceじゃなくてinterfaceでした
すいません…

awazi · 2016-11-24T14:40:03Z

コンパイル出来るようになりました。ありがとうございます。
app.util.concurrentについては、動作確認をした際に判明した不具合を修正しています。

S--Minecraft · 2016-11-24T23:31:21Z

src/ui/ThreadContent.coffee

+          checkUrl(a).done( (a, link) ->
+            return app.ImageReplaceDat.do(a, link).done( (a, res, err) ->
+              # サムネイルの追加
+              addThumbnail(a, res.text, "image", res.referrer, res.cookie) unless err?


unless err?はaudio,videoの処理にもかけないといけないのでは？

audioとvideoは、ImageViewURLReplace.datに依存しなため不要と考えます。

ImageViewURLReplace.datでurlをaudio/videoのmp4などへのurlに置き換えることもあるのでは？

S--Minecraft · 2016-11-24T23:35:50Z

src/ui/ThreadContent.coffee

+          ".message > a:not(.thumbnail):not(.has_thumbnail):not(.expandedURL):not(.has_expandedURL)"
+          ), (a) ->
+          checkUrl(a).done( (a, link) ->
+            return app.ImageReplaceDat.do(a, link).done( (a, res, err) ->


checkUrl(a).done( (a, link) -> return app.ImageReplaceDat(a,link) ).done( (a,res,err) -> 〜 )

よくかんがえたらこうかけました
インデント減るのでこっちで…

元々そのように書かれていましたが、サムネイルが表示されないため修正しました。
(aは受け取れてもresを受け取れないため？)

すいませんちょっといろんな仕様を混同してました
doneを両方ともthenにするといけるはずです

thenに変更して動くようになりました。

awazi · 2016-11-25T16:54:51Z

変更箇所
・app.util.concurrent周りの処理
・app.ImageReplaceDatでのaudio/videoへの置き換えに対応 (これにより、oggファイルの設定をaudio/videoの両方に設定されていた場合はaudioとみなされます。また、audio/videoの使用設定をせずに置き換えが行われた場合はimageとして扱われるため正しく表示されません)

awazi · 2016-11-26T04:44:34Z

変更箇所
・audio/videoの使用設定をせずにImageViewURLRepace.datでの置き換えが行われた場合は無視するように変更 (oggファイルの設定をaudio/videoの両方に設定されていた場合はvideoとみなされます)

eru

eru added the 機能追加 label Nov 18, 2016

eru requested changes Nov 20, 2016

View reviewed changes

S--Minecraft requested changes Nov 21, 2016

View reviewed changes

awazi force-pushed the dev3-4 branch from 278c814 to da4ca05 Compare November 23, 2016 14:19

eru requested changes Nov 24, 2016

View reviewed changes

S--Minecraft requested changes Nov 24, 2016

View reviewed changes

awazi force-pushed the dev3-4 branch from da4ca05 to b922f4c Compare November 24, 2016 12:46

S--Minecraft requested changes Nov 24, 2016

View reviewed changes

awazi force-pushed the dev3-4 branch from b922f4c to a1c5bae Compare November 24, 2016 14:35

S--Minecraft requested changes Nov 24, 2016

View reviewed changes

awazi force-pushed the dev3-4 branch from a1c5bae to 933e5eb Compare November 25, 2016 16:47

Update: 短縮URLの展開機能を追加

26858f5

awazi force-pushed the dev3-4 branch from 933e5eb to 26858f5 Compare November 26, 2016 04:38

S--Minecraft approved these changes Nov 26, 2016

View reviewed changes

eru approved these changes Nov 27, 2016

View reviewed changes

eru merged commit bac17b7 into readcrx-2:develop Nov 27, 2016

awazi deleted the dev3-4 branch December 15, 2016 10:37

機能追加(4/4): 短縮URLの展開機能を追加 #223

機能追加(4/4): 短縮URLの展開機能を追加 #223

Conversation

awazi commented Nov 18, 2016

eru commented Nov 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awazi commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awazi commented Nov 24, 2016

S--Minecraft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awazi commented Nov 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awazi commented Nov 25, 2016 • edited Loading

awazi commented Nov 26, 2016

eru left a comment

Choose a reason for hiding this comment

awazi commented Nov 25, 2016 •

edited

Loading