Upstream ChromeHttpAuthHandler

Upstream ChromeHttpAuthHandler

BUG=138142
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150245

[Yusuf, 2012-07-31 21:37:40]

Please let me know if I need to add anyone else to the review. Thanks!

[Yusuf, 2012-08-02 20:15:28]

sky@ friendly ping to have a look at this one for chrome/browser. Thanks!

[Yaron, 2012-08-02 20:50:24]

What's the test story for this? Is there coverage? Can we add it upstream?

http://codereview.chromium.org/10821116/diff/3002/chrome/DEPS
File chrome/DEPS (right):

http://codereview.chromium.org/10821116/diff/3002/chrome/DEPS#newcode24
chrome/DEPS:24: "+content/public/android/java",
It's probably better to instead add:
content/public/android/java/src/org/chromium/content/browser/ in
chrome/browser/DEPS. My thinking then is that you'll avoid accidentally having
browser import something from renderer.

http://codereview.chromium.org/10821116/diff/3002/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeHttpAuthHandler.java
(right):

http://codereview.chromium.org/10821116/diff/3002/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeHttpAuthHandler.java:39:
// Note on legacy useHttpAuthUsernamePassword() method:
Nit: odd comment style. Please fix.

http://codereview.chromium.org/10821116/diff/3002/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeHttpAuthHandler.java:91:
public static interface AutofillObserver {
You should document this interface better. It seems like this class is just a
hook to get to some chrome resources and then provide another external hook.
Actually lit looks like login_prompt_android.cc is part of this and shoudl be
included.

http://codereview.chromium.org/10821116/diff/3002/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeHttpAuthHandler.java:120:
private native void nativeSetAuth(int nativeChromeHttpAuthHandler,
Nit: sort

http://codereview.chromium.org/10821116/diff/3002/chrome/browser/ui/android/c...
File chrome/browser/ui/android/chrome_http_auth_handler.h (right):

http://codereview.chromium.org/10821116/diff/3002/chrome/browser/ui/android/c...
chrome/browser/ui/android/chrome_http_auth_handler.h:5: #ifndef
CHROME_BROWSER_UI_ANDROID_CHROME_CHROME_HTTP_AUTH_HANDLER_H_
There's an extra "chrome" in there.

http://codereview.chromium.org/10821116/diff/3002/chrome/browser/ui/android/c...
chrome/browser/ui/android/chrome_http_auth_handler.h:75: bool
RegisterChromeHttpAuthHandler(JNIEnv* env);
Make this a static member of ChromeHttpAuthHandler.

[sky, 2012-08-03 16:40:20]

On 2012/08/02 20:15:28, yusufo1 wrote:
> sky@ friendly ping to have a look at this one for chrome/browser. Thanks!

Get Yaron happy first, since the only files you need me to review are
chrome_browser.gypi and DEPS.

[Yusuf, 2012-08-03 21:14:44]

I am adding tests downstream, but it seems adding the build target for chrome
javatests will need to wait a bit, so the tests will have to be upstreamed
later.

Had to rebase to include the patch for guarding C++--> Java calls.

Added the related callbacks to content_view_core_impl also.

[Yaron, 2012-08-03 21:25:15]

Looks like you need to rebase. This has also changed downstream.

[Yaron, 2012-08-03 23:29:16]

lgtm mod nits

http://codereview.chromium.org/10821116/diff/5006/chrome/browser/ui/android/c...
File chrome/browser/ui/android/chrome_http_auth_handler.h (right):

http://codereview.chromium.org/10821116/diff/5006/chrome/browser/ui/android/c...
chrome/browser/ui/android/chrome_http_auth_handler.h:74: // Registers the
ChromeHttpAuthHandler native methods.
Nit (as requesetd before): please make this a static function of
ChromeHttpAuthHandler

http://codereview.chromium.org/10821116/diff/5006/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10821116/diff/5006/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2413: 'browser/ui/android/login_prompt_android.cc',
nit: sort

[sky, 2012-08-03 23:56:47]

LGTM

[commit-bot: I haz the power, 2012-08-06 16:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/10821116/17002

[commit-bot: I haz the power, 2012-08-06 18:48:55]

Try job failure for 10821116-17002 (retry) (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-06 21:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/10821116/17002

[commit-bot: I haz the power, 2012-08-06 22:36:03]

Try job failure for 10821116-17002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-06 22:44:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/10821116/17002

[commit-bot: I haz the power, 2012-08-06 23:13:29]

Try job failure for 10821116-17002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-07 00:54:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/10821116/17002

[commit-bot: I haz the power, 2012-08-07 02:28:06]

Change committed as 150245