Anti Phising UI for web auth flow window:

Anti Phising UI for web auth flow window:
- change the implementation of web auth flow window to use browwer popups
- Use notifications instead of being a WebContentsDelegate for navigation events and window close events.
- Change tests accordingly.

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

[Mihai Parparita -not on Chrome, 2012-07-31 22:05:19]

LGTM

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:80: // redirect URLs.
A longer comment here indicating the expected order of notifications (and what
happens when) might be helpful. For example, it's not immediately obvious that
the window is only shown after NOTIFICATION_LOAD_STOP (if it's not the redirect
URL).

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:119: // Observe method
will be called before URls are loaded. That is where
Typo ("URLls")

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:127: } else {
No need for this to be in an else block (due to the return statement above).

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:150: if (type ==
content::NOTIFICATION_LOAD_START) {
::Observe implementations generally use switch statements.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:158: if (load_details !=
NULL) {
No need for braces around the body here.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:202: bool
WebAuthFlow::ReportResultIfUrlValid(const GURL& url) {
This doesn't seem to be used.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:206: } else {
No need for else here.

[Munjal (Google), 2012-07-31 23:22:49]

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/identity/web_auth_flow.cc (right):

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:119: // Observe method
will be called before URls are loaded. That is where
On 2012/07/31 22:05:20, Mihai Parparita wrote:
> Typo ("URLls")

Done.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:127: } else {
On 2012/07/31 22:05:20, Mihai Parparita wrote:
> No need for this to be in an else block (due to the return statement above).

Done.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:150: if (type ==
content::NOTIFICATION_LOAD_START) {
On 2012/07/31 22:05:20, Mihai Parparita wrote:
> ::Observe implementations generally use switch statements.

Done.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:158: if (load_details !=
NULL) {
On 2012/07/31 22:05:20, Mihai Parparita wrote:
> No need for braces around the body here.

Done.

https://chromiumcodereview.appspot.com/10826078/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/identity/web_auth_flow.cc:202: bool
WebAuthFlow::ReportResultIfUrlValid(const GURL& url) {
On 2012/07/31 22:05:20, Mihai Parparita wrote:
> This doesn't seem to be used.

Done. Good catch.

[commit-bot: I haz the power, 2012-08-01 03:52:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/munjal@chromium.org/10826078/10002

[commit-bot: I haz the power, 2012-08-01 05:14:16]

Change committed as 149374