Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9460010:
test http auth-basic handling and implement headers.binaryValue for downloads.download() (Closed)
|
Created:
8 years, 10 months ago by benjhayden Modified:
8 years, 9 months ago Reviewers:
cbentzel CC:
chromium-reviews, Aaron Boodman, mihaip+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiontest http auth-basic handling and implement headers.binaryValue for downloads.download()
BUG=115629
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124814
Patch Set 1 #Patch Set 2 : fail/succeed #
Total comments: 10
Patch Set 3 : ... #Patch Set 4 : ... #
Total comments: 2
Patch Set 5 : binaryValue #Patch Set 6 : LOAD_DO_NOT_PROMPT_FOR_LOGIN #Patch Set 7 : merge #Patch Set 8 : merge #Patch Set 9 : include load_flags.h #
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:88: function downloadAuthBasicFail() { What happens in this case? Do you see an auth prompt?
http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:88: function downloadAuthBasicFail() { On 2012/02/24 22:36:07, cbentzel wrote: > What happens in this case? Do you see an auth prompt? Nope, no prompt. The test succeeds. It detects the 427-byte long error html page.
Generally L G T M except for the console.log statements and whether you can detect interrupted with Andy's recent CL. http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:88: function downloadAuthBasicFail() { On 2012/02/27 14:09:55, benjhayden_chromium wrote: > On 2012/02/24 22:36:07, cbentzel wrote: > > What happens in this case? Do you see an auth prompt? > > Nope, no prompt. The test succeeds. It detects the 427-byte long error html > page. I'm still not entirely sure why you wouldn't get a prompt with the current code. I'll patch this in and try it in a debugger. Probably just a hole in my brain. http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:90: console.log(downloadId); Remove console.log Or you could add a debugLog() function that you temporarily override to console.log http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:101: // TODO(benjhayden): Change COMPLETE to INTERRUPTED after This may be fixed - see the most recent CL on http://crbug.com/114020
PTAL http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:88: function downloadAuthBasicFail() { On 2012/02/28 20:23:22, cbentzel wrote: > On 2012/02/27 14:09:55, benjhayden_chromium wrote: > > On 2012/02/24 22:36:07, cbentzel wrote: > > > What happens in this case? Do you see an auth prompt? > > > > Nope, no prompt. The test succeeds. It detects the 427-byte long error html > > page. > > I'm still not entirely sure why you wouldn't get a prompt with the current code. > I'll patch this in and try it in a debugger. Probably just a hole in my brain. Done. http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:90: console.log(downloadId); On 2012/02/28 20:23:22, cbentzel wrote: > Remove console.log > > Or you could add a debugLog() function that you temporarily override to > console.log Done. Also, this test is still DISABLED. http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:101: // TODO(benjhayden): Change COMPLETE to INTERRUPTED after On 2012/02/28 20:23:22, cbentzel wrote: > This may be fixed - see the most recent CL on http://crbug.com/114020 Done.
Looks like you are adding more to this CL? http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:88: function downloadAuthBasicFail() { On 2012/03/01 18:43:18, benjhayden_chromium wrote: > On 2012/02/28 20:23:22, cbentzel wrote: > > On 2012/02/27 14:09:55, benjhayden_chromium wrote: > > > On 2012/02/24 22:36:07, cbentzel wrote: > > > > What happens in this case? Do you see an auth prompt? > > > > > > Nope, no prompt. The test succeeds. It detects the 427-byte long error html > > > page. > > > > I'm still not entirely sure why you wouldn't get a prompt with the current > code. > > I'll patch this in and try it in a debugger. Probably just a hole in my brain. > > Done. Done meaning the hole in my brain has been resolved? http://codereview.chromium.org/9460010/diff/8001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/8001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:9: // console.debug = function() {}; I'd prefer it if you introduced a new function which turns into a no-op initially, than overriding console.debug I think the behavior will be less surprising.
http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:88: function downloadAuthBasicFail() { On 2012/03/01 22:16:26, cbentzel wrote: > On 2012/03/01 18:43:18, benjhayden_chromium wrote: > > On 2012/02/28 20:23:22, cbentzel wrote: > > > On 2012/02/27 14:09:55, benjhayden_chromium wrote: > > > > On 2012/02/24 22:36:07, cbentzel wrote: > > > > > What happens in this case? Do you see an auth prompt? > > > > > > > > Nope, no prompt. The test succeeds. It detects the 427-byte long error > html > > > > page. > > > > > > I'm still not entirely sure why you wouldn't get a prompt with the current > > code. > > > I'll patch this in and try it in a debugger. Probably just a hole in my > brain. > > > > Done. > > Done meaning the hole in my brain has been resolved? Done meaning I don't see anything that I need to do in order to resolve this comment. http://codereview.chromium.org/9460010/diff/8001/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/9460010/diff/8001/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/downloads/test.js:9: // console.debug = function() {}; On 2012/03/01 22:16:26, cbentzel wrote: > I'd prefer it if you introduced a new function which turns into a no-op > initially, than overriding console.debug I tried putting console.log inside another function and the line numbers were all the same. I use those line numbers, so that solution is not viable. I tried aliasing console.log: var debug = true ? console.log : function() {}; but this threw "TypeError: Illegal invocation". It looks like console.log is a bound method, not a function in a namespace, so that dot matters. Javascript != Python, so I can't alias bound methods; trying to do so just aliases the *un*bound method. I could alias log/debug to another method on console: console.MAYBE_LOG = true ? console.log : function() {}; but then I've simply duplicated console.debug. There is no difference between this console.MAYBE_LOG and console.debug. You could argue that other scripts are more likely to use console.debug than to copy my use of console.MAYBE_LOG, but it's still possible for another script to conflict. I suppose I could use something like lisp's gensym: var MAYBE_LOG = 'downloads_apitest_debug_' + Math.random(); console[MAYBE_LOG] = true ? console.log : function() {}; console[MAYBE_LOG](message); but somehow this seems just a tad, I don't know, ridiculous. I could manually parse the line number out of a stack trace and manually format the log line in about 40 LoC, but I am unaware of a way to print something from javascript to browser_tests's stdout without any implicit formatting, so these log lines would have two line numbers, one of which is a lie. If there's a specific method that you have in mind that would work, show me and I'll use it, but this thread seems like an awful lot of fuss and bother and 40 LoC seems like an awful lot of code just to avoid using console.debug. Especially since this test is still disabled, so this CL won't affect anybody but me until I figure out why it's stalling on the waterfall, which might be a while since I can't repro on my windows. > > I think the behavior will be less surprising. What do you think is surprising about this behavior? This is the only file in the stack that uses console.debug, so these console.debug()s are the only calls that would be suppressed. Everything else uses console.log, which is unchanged.
On 2012/03/02 14:43:47, benjhayden_chromium wrote: > http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/9460010/diff/2001/chrome/test/data/extensions/... > chrome/test/data/extensions/api_test/downloads/test.js:88: function > downloadAuthBasicFail() { > On 2012/03/01 22:16:26, cbentzel wrote: > > On 2012/03/01 18:43:18, benjhayden_chromium wrote: > > > On 2012/02/28 20:23:22, cbentzel wrote: > > > > On 2012/02/27 14:09:55, benjhayden_chromium wrote: > > > > > On 2012/02/24 22:36:07, cbentzel wrote: > > > > > > What happens in this case? Do you see an auth prompt? > > > > > > > > > > Nope, no prompt. The test succeeds. It detects the 427-byte long error > > html > > > > > page. > > > > > > > > I'm still not entirely sure why you wouldn't get a prompt with the current > > > code. > > > > I'll patch this in and try it in a debugger. Probably just a hole in my > > brain. > > > > > > Done. > > > > Done meaning the hole in my brain has been resolved? > > Done meaning I don't see anything that I need to do in order to resolve this > comment. > OK. I actually ended up patching it in and the reason that the auth prompt didn't appear was more complicated than I initially suspected. See my comment on the bug for an explanation, and a suggestion for a runtime change. > http://codereview.chromium.org/9460010/diff/8001/chrome/test/data/extensions/... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/9460010/diff/8001/chrome/test/data/extensions/... > chrome/test/data/extensions/api_test/downloads/test.js:9: // console.debug = > function() {}; > On 2012/03/01 22:16:26, cbentzel wrote: > > I'd prefer it if you introduced a new function which turns into a no-op > > initially, than overriding console.debug > > I tried putting console.log inside another function and the line numbers were > all the same. I use those line numbers, so that solution is not viable. > > I tried aliasing console.log: > var debug = true ? console.log : function() {}; > but this threw "TypeError: Illegal invocation". It looks like console.log is a > bound method, not a function in a namespace, so that dot matters. Javascript != > Python, so I can't alias bound methods; trying to do so just aliases the > *un*bound method. > > I could alias log/debug to another method on console: > console.MAYBE_LOG = true ? console.log : function() {}; > but then I've simply duplicated console.debug. There is no difference between > this console.MAYBE_LOG and console.debug. You could argue that other scripts are > more likely to use console.debug than to copy my use of console.MAYBE_LOG, but > it's still possible for another script to conflict. I suppose I could use > something like lisp's gensym: > var MAYBE_LOG = 'downloads_apitest_debug_' + Math.random(); > console[MAYBE_LOG] = true ? console.log : function() {}; > console[MAYBE_LOG](message); > but somehow this seems just a tad, I don't know, ridiculous. > > I could manually parse the line number out of a stack trace and manually format > the log line in about 40 LoC, but I am unaware of a way to print something from > javascript to browser_tests's stdout without any implicit formatting, so these > log lines would have two line numbers, one of which is a lie. > > If there's a specific method that you have in mind that would work, show me and > I'll use it, but this thread seems like an awful lot of fuss and bother and 40 > LoC seems like an awful lot of code just to avoid using console.debug. > Especially since this test is still disabled, so this CL won't affect anybody > but me until I figure out why it's stalling on the waterfall, which might be a > while since I can't repro on my windows. > > > > > I think the behavior will be less surprising. > > What do you think is surprising about this behavior? > This is the only file in the stack that uses console.debug, so these > console.debug()s are the only calls that would be suppressed. Everything else > uses console.log, which is unchanged. OK, you did a lot of investigation for alternatives and this seems like the best approach. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/9460010/14001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/9460010/15005
Try job failure for 9460010-15005 (retry) on linux_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
I realize my lgtm was only for the author basic tests. I did not review the binary value code. Mea culpa. On Mar 2, 2012 4:28 PM, <commit-bot@chromium.org> wrote: > Try job failure for 9460010-15005 (retry) on linux_rel for step "compile" > (clobber build). > It's a second try, previously, step "compile" failed. > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=linux_rel&**number=5784<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=5784> > > > http://codereview.chromium.**org/9460010/<http://codereview.chromium.org/9460... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/9460010/14007
Change committed as 124814 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
