Skip to content

multiPass Resize + bugfixes #163

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

Closed
wants to merge 1 commit into from
Closed

Conversation

shtyftu
Copy link

@shtyftu shtyftu commented Nov 29, 2013

No description provided.

@RubaXa
Copy link
Contributor

RubaXa commented Nov 29, 2013

Я не могу принять этот pull request, очень "грязный", нужно привести в порядок.

Основное

Во первых, перед отправкой pull request нужно запустить
npm install
grunt
— все тесты должны быть пройдены

Во вторых, в описании указать какие проблемы были решены, что добавлено.

В третьих, не нужно вносить изменения, которые ничего не меняют:
https://github.com/mailru/FileAPI/pull/163/files#diff-d66e3e8af67800c2b7d1a9461b6eba57R340


По коду

Не нужно оборачивать в try catch код внутри методов, т.к. весь метод не будет оптимизирован (JIT):
https://github.com/mailru/FileAPI/pull/163/files#diff-3499afafd0bf1c1dd2fb817e99c7b32cR135

Этого try catch будет достаточно, но нужно добавить api.log('[err] FileAPI.Image.fn._apply: '+err.toString());
https://github.com/mailru/FileAPI/pull/163/files#diff-3499afafd0bf1c1dd2fb817e99c7b32cR286

Зачем это? Чем не угодил POST?
https://github.com/mailru/FileAPI/pull/163/files#diff-cc3a3bb86697dbc9c83edcf01eccea36R91

Зачем это всё, если в iframe нужно просто вызвать callback, название которого было переданно через POST
https://github.com/mailru/FileAPI/pull/163/files#diff-cc3a3bb86697dbc9c83edcf01eccea36R135
https://github.com/mailru/FileAPI/pull/163/files#diff-cc3a3bb86697dbc9c83edcf01eccea36R141
https://github.com/mailru/FileAPI/pull/163/files#diff-cc3a3bb86697dbc9c83edcf01eccea36R147

А вот это уже баг, window.FileAPI может быть не определен:
https://github.com/mailru/FileAPI/pull/163/files#diff-d2288ee75713db667ef315dc6bd31027R27

P.S. Друзья и перед тем как вводить какие-то новые параметры или методы, давайте их обсуждать, ведь мы это делаем не только для себя.

@mbezoyan
Copy link

mbezoyan commented Dec 3, 2013

  • try{} catch - ок
  • POST не угодил тем что парсить на сервере сложнее multipart (в нашем конктертном случае)
  • в некоторых браузерах при cross domain редиректах вызов коллбэка из iframe не работает
  • window.FileAPI - согласен, не успел выпилить

@RubaXa
Copy link
Contributor

RubaXa commented Dec 3, 2013

POST не угодил тем что парсить на сервере сложнее multipart (в нашем конктертном случае)

Это совсем не повод вводить новый параметр.

Предлагаю добавить опцию jsonp: "callback", которая будет отвечать за название POST-параметра. Если она false, вешаемся на событие onLoad у iframe и парсим полученный контент.

https://github.com/mailru/FileAPI/pull/163/files#diff-cc3a3bb86697dbc9c83edcf01eccea36R147
— такая реализация никуда не годиться, для этого есть onLoad:

// jsonp-callack
var complete = window[uid] = function (status, statusText, response){
    _this.readyState    = 4;
    _this.responseText  = response;
    _this.end(status, statusText);
    window[uid] = xhr = null;
};

if( !jsonp ){
    iframe.onload = function (){
        try {
            var
                  transport = xhr.getElementsByTagName('iframe')[0]
                , win = transport.contentWindow
                , doc = win.document
                , result = win.result || new Function('return ('+doc.body.innerHTML+')')()
            ;
            complete(result.status, result.statusText, result.response);
        } catch (e){}
    };
}

@mbezoyan
Copy link

mbezoyan commented Dec 3, 2013

Это совсем не повод вводить новый параметр.

Почему не повод? очень даже повод.

такая реализация никуда не годиться, для этого есть onLoad

onLoad не годица, там есть редиректы и могут быть внутренние сабмиты.

@RubaXa
Copy link
Contributor

RubaXa commented Dec 3, 2013

onLoad не годица, там есть редиректы и могут быть внутренние сабмиты.

http://jsfiddle.net/jEr9v/show/ — пример со всем видами редиректов, где именно у вас не работает?
http://jsfiddle.net/jEr9v

<?
    if( $_GET['step'] == 2 ){
        header('location: http://jsfiddle.net/kv2tg/show/');
    }
    else {
        header('location: http://jsfiddle.net/2ZLXs/1/show/');
    }

http://jsfiddle.net/2ZLXs/1/
http://jsfiddle.net/a7MR2/1/

@RubaXa
Copy link
Contributor

RubaXa commented Dec 4, 2013

Вот образцово-показательный pull request: #164

Но это не просто pull request, но и улучшенная работа с iframe:

// default callback
FileAPI.upload({
   jsonp: 'callback' 
});

// callback in url
FileAPI.upload({
   url: 'ctrl.php?callback=?'  // jQuery style
});

// without callback
FileAPI.upload({
   jsonp: false
});

@RubaXa
Copy link
Contributor

RubaXa commented Dec 4, 2013

#164 — iframe
#165 — try catch и вернул утерянные изменения при merge #140

@RubaXa RubaXa closed this Dec 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants