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

Remove needless access to 'arguments' #590

Merged
merged 3 commits into from Nov 9, 2013

Conversation

Projects
None yet
2 participants
@saneyuki
Copy link
Contributor

commented Nov 6, 2013

  • This requests to pull to nightly branch.
  • On ES6, we can use Rest parameters instead of arguments.
  • We should pass arguments to other methods if it's available.
@saneyuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2013

@piroor ping?

@piroor

This comment has been minimized.

Copy link
Owner

commented Nov 8, 2013

Sorry for this delay. Thanks a lot! Now I'm studying about rest parameters...

@saneyuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2013

Sorry for this delay.

No problem :)

@@ -216,7 +216,7 @@ var TreeStyleTabBase = {
let Scriptish_openInTab = tabModule.Scriptish_openInTab;
tabModule.Scriptish_openInTab = function(aURL, aLoadInBackground, aReuse, aChromeWin) {
aChromeWin.TreeStyleTabService.readyToOpenChildTabNow(aChromeWin.gBrowser);
return Scriptish_openInTab.apply(this, arguments);
return Scriptish_openInTab.apply(this, aArgs);

This comment has been minimized.

Copy link
@piroor

piroor Nov 9, 2013

Owner

ここは間違いです。このargumentsは「tabModule.Scriptish_openInTab」に渡されたすべての引数を元の関数に引き渡すための物です。
また、元の「tabModule.Scriptish_openInTab」のarity(length)を変えない方がよいのではないかという事で、ここでは敢えて「aURL, aLoadInBackground, aReuse, aChromeWin」と定義した上で、且つこちらで把握していない間に引数が増えた場合のためということで、argumentsすべてをapplyで引き渡すという戦略を採ったという経緯になります。

This comment has been minimized.

Copy link
@saneyuki

saneyuki Nov 10, 2013

Author Contributor

この変更が単純に間違えていたのはわかりました。

単純に引数が末尾に追加される形で増えるのであれば、argumentsを用いずに直接tabModule.Scriptish_openInTabの引数を渡した場合でも結果は変わりません。仮に2番目に引数が挿入された、もしくは引数を丸々オブジェクトバッグを渡す形式に変更されたのであれば、argumentsを渡した所で結局動作しなくなりますので、tabModule.Scriptish_openInTabの引数をargumentsで渡すのは本質的な解決では無いように思います。

This comment has been minimized.

Copy link
@piroor

piroor Nov 10, 2013

Owner

なるほど、確かにその指摘は尤もです。「動作が壊れないように」という所を意識して、せめて元の動作は保証するようにしてみました。
765b0d2

@@ -1570,7 +1570,7 @@ var TreeStyleTabBase = {
*/
readyToOpenChildTabNow : function TSTBase_readyToOpenChildTabNow(aFrameOrTabBrowser, aMultiple) /* PUBLIC API */
{
if (this.readyToOpenChildTab.apply(this, arguments)) {
if (this.readyToOpenChildTab(aFrameOrTabBrowser, aMultiple, false)) {

This comment has been minimized.

Copy link
@piroor

piroor Nov 9, 2013

Owner

ここは、「readyToOpenChildTabNow」を「readyToOpenChildTabの受け付ける引数すべてを同じ形で受け取る事ができる」という仕様を実現することと、「readyToOpenChildTabの仕様を変えた時にこちらのコードをなるべくいじらなくても済むようにしたい」という考えの2つの理由から、argumentsですべての引数を無条件に引き渡している部分です。なので、これではreadyToOpenChildTabNowの仕様が変わってしまいます。
argumentsを使わないようにするのであれば、TSTBase_readyToOpenChildTabNow(...aArgs)と定義した上でaFrameOrTabBrowser = aArgs[0]とした方がよいと思います。

This comment has been minimized.

Copy link
@saneyuki

saneyuki Nov 10, 2013

Author Contributor

publicなAPIであることを差し置いても、どちらもツリー型タブの用意したメソッドですので互換性についてはコードの変更で十分に担保できる(すべき)内容ですし、そもそもreadyToOpenChildTabの引数の定義が変わってしまうのであれば、それはwrapperであるreadyToOpenChildTabNowの引数の定義も変わるべき状況です

This comment has been minimized.

Copy link
@piroor

piroor Nov 10, 2013

Owner

互換性についてはコードの変更で十分に担保できる(すべき)

それはコード全体に十分にいつでも目が届く&全体に目を配るだけの余力があるという条件が整っている時にだけ言える事だと自分は考えています。

あと、本来であれば、ここは自動テストによってそのような動作が保証されていればどっちの戦略(引数を厳密に定義するか、rest argumentsで大雑把にやるか)を採っても問題のない箇所だと思います。なので、どっちを選ぶかという所に開発者のポリシーが反映されてきます。
僕は、メソッドの定義を見た時に「ああなるほど、全部の引数を渡して欲しいのだな」ということが一目で分かる状態である・コードがそのように語っている「べき」と考えているので、この手のコードを書く時はargumentsを多用していたという次第です。
(コメントを書かずにコードで意図を表現できるならそうする「べき」、というのが自分の考えです。)

@@ -1622,7 +1622,7 @@ var TreeStyleTabBase = {
*/
readyToOpenNextSiblingTabNow : function TSTBase_readyToOpenNextSiblingTabNow(aFrameOrTabBrowser) /* PUBLIC API */
{
if (this.readyToOpenNextSiblingTab.apply(this, arguments)) {
if (this.readyToOpenNextSiblingTab(aFrameOrTabBrowser)) {

This comment has been minimized.

@@ -1660,7 +1660,7 @@ var TreeStyleTabBase = {
readyToOpenNewTabGroupNow : function TSTBase_readyToOpenNewTabGroupNow(aFrameOrTabBrowser) /* PUBLIC API */
{

if (this.readyToOpenNewTabGroup.apply(this, arguments)) {
if (this.readyToOpenNewTabGroup(aFrameOrTabBrowser, null, null)) {

This comment has been minimized.

@@ -978,7 +977,7 @@ TreeStyleTabBrowser.prototype = {
catch(e) {
dump(e+'\n'+e.stack+'\n');
}
return this.__treestyletab__stop.apply(this, arguments);
return this.__treestyletab__stop.apply(this, aTab);

This comment has been minimized.

Copy link
@piroor

piroor Nov 9, 2013

Owner

これは<xul:browser/>のstopメソッドを置き換えた上で元のstopメソッドだった関数を呼んでいる部分で、もし元のstopメソッドが引数を受け取る形に仕様が変わっていたらその時も問題なく動作するようにしたいという事で、argumentsを使ってすべての引数を引き渡しているという事になります。なので、これでは意図通りの挙動になりません。

This comment has been minimized.

Copy link
@saneyuki

saneyuki Nov 10, 2013

Author Contributor

#590 (comment) と同意見です。

piroor added a commit that referenced this pull request Nov 9, 2013

Merge pull request #590 from saneyuki/args
Remove needless access to 'arguments'

@piroor piroor merged commit 1847700 into piroor:nightly Nov 9, 2013

@piroor

This comment has been minimized.

Copy link
Owner

commented Nov 9, 2013

上記の点はこちらで修正しておきました。何にせよ、モダンな書き方に切り替えていく作業、ありがとうございます。

@saneyuki saneyuki deleted the saneyuki:args branch Nov 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.