Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
187263002:
Refactor Authenticated Server Requests to use Promises (Closed)
|
Created:
6 years, 9 months ago by robliao Modified:
6 years, 9 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor Authenticated Server Requests to use Promises
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256902
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix Bug #
Total comments: 6
Patch Set 3 : Clarify the scoping #
Total comments: 2
Patch Set 4 : CR Feedback #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:253: * @param {string} handlerName Server handler to send the request to. This is more than the handler. It includes params and such in some cases. I'm kinda stumped on what to name it though. https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:263: request.addEventListener('loadend', function() { any difference between this and doing request.onloadend =? https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:265: }, false); This parameter is optional and looks like it defaults to false. Would have saved me a trip to documentation.
https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:253: * @param {string} handlerName Server handler to send the request to. Unfortunately there is no real name for the path+query. We can ponder this later. On 2014/03/07 00:53:06, rgustafson wrote: > This is more than the handler. It includes params and such in some cases. I'm > kinda stumped on what to name it though. https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:263: request.addEventListener('loadend', function() { It doesn't overwrite any existing onloadend that was set. On 2014/03/07 00:53:06, rgustafson wrote: > any difference between this and doing request.onloadend =? https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:265: }, false); Given that the default is not consistent (false in Chrome, true in other browsers), I'm keeping it here for clarity. The web examples also use this. On 2014/03/07 00:53:06, rgustafson wrote: > This parameter is optional and looks like it defaults to false. Would have saved > me a trip to documentation.
https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:265: }, false); On 2014/03/07 01:39:40, robliao wrote: > Given that the default is not consistent (false in Chrome, true in other > browsers), I'm keeping it here for clarity. The web examples also use this. > On 2014/03/07 00:53:06, rgustafson wrote: > > This parameter is optional and looks like it defaults to false. Would have > saved > > me a trip to documentation. > It's false for Firefox: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener The IE docs aren't very clear, but link to the w3 spec (http://www.w3.org/TR/DOM-Level-3-Events/#widl-EventTarget-addEventListener) which says: void addEventListener (DOMString type, EventListener? listener, optional boolean useCapture = false); so I really really really hope none of the browsers default to true. Web examples use it since it used to be required, but I believe removing it will make it easier to comprehend without wondering what that false is. https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:268: requestPromise.then(function(request) { Just to make sure I understand what is going on here: Is adding the request parameter here what makes the next 'then' receive the request? Or why did this have to be added?
https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:265: }, false); Instead we'll wonder what the default is :-) for those aware that the default isn't always false. On 2014/03/11 21:29:30, rgustafson wrote: > On 2014/03/07 01:39:40, robliao wrote: > > Given that the default is not consistent (false in Chrome, true in other > > browsers), I'm keeping it here for clarity. The web examples also use this. > > On 2014/03/07 00:53:06, rgustafson wrote: > > > This parameter is optional and looks like it defaults to false. Would have > > saved > > > me a trip to documentation. > > > > It's false for Firefox: > https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener > The IE docs aren't very clear, but link to the w3 spec > (http://www.w3.org/TR/DOM-Level-3-Events/#widl-EventTarget-addEventListener) > which says: > void addEventListener (DOMString type, EventListener? listener, optional > boolean useCapture = false); > so I really really really hope none of the browsers default to true. > > Web examples use it since it used to be required, but I believe removing it will > make it easier to comprehend without wondering what that false is. https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:268: requestPromise.then(function(request) { JS is lexically scoped. Since there is no request variable outside of the declaring environment, it defaults to undefined. On 2014/03/11 21:29:31, rgustafson wrote: > Just to make sure I understand what is going on here: Is adding the request > parameter here what makes the next 'then' receive the request? Or why did this > have to be added?
https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:268: requestPromise.then(function(request) { On 2014/03/12 00:13:11, robliao wrote: > JS is lexically scoped. Since there is no request variable outside of the > declaring environment, it defaults to undefined. > On 2014/03/11 21:29:31, rgustafson wrote: > > Just to make sure I understand what is going on here: Is adding the request > > parameter here what makes the next 'then' receive the request? Or why did this > > have to be added? > maybe use a different name if this shadows the outer one?
https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:268: requestPromise.then(function(request) { There's no shadowing. They're actually in different scopes. On 2014/03/12 19:01:34, Travis Skare wrote: > On 2014/03/12 00:13:11, robliao wrote: > > JS is lexically scoped. Since there is no request variable outside of the > > declaring environment, it defaults to undefined. > > On 2014/03/11 21:29:31, rgustafson wrote: > > > Just to make sure I understand what is going on here: Is adding the request > > > parameter here what makes the next 'then' receive the request? Or why did > this > > > have to be added? > > > > maybe use a different name if this shadows the outer one?
https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:268: requestPromise.then(function(request) { Clarifying the scoping... On 2014/03/12 19:49:53, robliao wrote: > There's no shadowing. They're actually in different scopes. > On 2014/03/12 19:01:34, Travis Skare wrote: > > On 2014/03/12 00:13:11, robliao wrote: > > > JS is lexically scoped. Since there is no request variable outside of the > > > declaring environment, it defaults to undefined. > > > On 2014/03/11 21:29:31, rgustafson wrote: > > > > Just to make sure I understand what is going on here: Is adding the > request > > > > parameter here what makes the next 'then' receive the request? Or why did > > this > > > > have to be added? > > > > > > > maybe use a different name if this shadows the outer one? >
lgtm https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:268: requestPromise.then(function(request) { yep this solves it. I was talking about the shadowing of the closure over the above 'request' in diff 1. same behavior, just a readability thing. thanks! On 2014/03/12 20:29:25, robliao wrote: > Clarifying the scoping... > On 2014/03/12 19:49:53, robliao wrote: > > There's no shadowing. They're actually in different scopes. > > On 2014/03/12 19:01:34, Travis Skare wrote: > > > On 2014/03/12 00:13:11, robliao wrote: > > > > JS is lexically scoped. Since there is no request variable outside of the > > > > declaring environment, it defaults to undefined. > > > > On 2014/03/11 21:29:31, rgustafson wrote: > > > > > Just to make sure I understand what is going on here: Is adding the > > request > > > > > parameter here what makes the next 'then' receive the request? Or why > did > > > this > > > > > have to be added? > > > > > > > > > > maybe use a different name if this shadows the outer one? > > >
lgtm https://codereview.chromium.org/187263002/diff/40001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/40001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:253: * @param {XMLHttpRequest} request XMLHTTPRequest that sent the authenticated Nitpicky: match capitalization on the second usage
vadimt: Please provide owner approval of this CL. https://codereview.chromium.org/187263002/diff/40001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/187263002/diff/40001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:253: * @param {XMLHttpRequest} request XMLHTTPRequest that sent the authenticated On 2014/03/12 21:26:28, rgustafson wrote: > Nitpicky: match capitalization on the second usage Done.
lgtm
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/187263002/40002
Message was sent while issue was closed.
Change committed as 256902 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
