Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(626)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 243953002: Update PasswordManagerBrowserTest to use the new Observer class from InfoBarManager. (Closed)

Created:
6 years, 8 months ago by tfarina
Modified:
6 years, 8 months ago
CC:
chromium-reviews, droger
Visibility:
Public.

Description

Update PasswordManagerBrowserTest to use the new Observer class from InfoBarManager. This way we can move away from content notifications, which is deprecated and should be removed. The observer is the new way to listen for infobar notifications. BUG=354380 TEST=browser_tests --gtest_filter=PasswordManagerBrowserTest.* R=pkasting@chromium.org,isherman@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265203

Patch Set 1 #

Total comments: 9

Patch Set 2 : cleanup #

Total comments: 4

Patch Set 3 : inline #

Patch Set 4 : REBASED #

Patch Set 5 : include infobar_manager.h it is now in components/infobars/ #

Total comments: 4

Patch Set 6 : fix Peter nits #

Patch Set 7 : it is in infobars namespace now #

Patch Set 8 : fix compilation - add missing infobars:: #

Patch Set 9 : remove content notification includes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -38 lines) Patch
M chrome/browser/password_manager/password_manager_browsertest.cc View 1 2 3 4 5 6 7 8 6 chunks +29 lines, -34 lines 0 comments Download
M components/infobars/core/infobar_manager.h View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download
M components/infobars/core/infobar_manager.cc View 1 2 3 4 5 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
tfarina
6 years, 8 months ago (2014-04-18 23:36:46 UTC) #1
Peter Kasting
https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc#newcode95 chrome/browser/password_manager/password_manager_browsertest.cc:95: // InfoBarManager::Observer Nit: Add trailing colon https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc#newcode101 chrome/browser/password_manager/password_manager_browsertest.cc:101: ->Accept(); ...
6 years, 8 months ago (2014-04-18 23:52:39 UTC) #2
Ilya Sherman
https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc#newcode104 chrome/browser/password_manager/password_manager_browsertest.cc:104: } nit: Please leave a blank line between method ...
6 years, 8 months ago (2014-04-19 04:21:27 UTC) #3
tfarina
https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/243953002/diff/1/chrome/browser/password_manager/password_manager_browsertest.cc#newcode95 chrome/browser/password_manager/password_manager_browsertest.cc:95: // InfoBarManager::Observer On 2014/04/18 23:52:39, Peter Kasting wrote: > ...
6 years, 8 months ago (2014-04-19 18:43:10 UTC) #4
Ilya Sherman
https://codereview.chromium.org/243953002/diff/10001/chrome/browser/infobars/infobar_manager.h File chrome/browser/infobars/infobar_manager.h (right): https://codereview.chromium.org/243953002/diff/10001/chrome/browser/infobars/infobar_manager.h#newcode27 chrome/browser/infobars/infobar_manager.h:27: virtual ~Observer() {} nit: I don't think you need ...
6 years, 8 months ago (2014-04-19 19:46:03 UTC) #5
tfarina
https://codereview.chromium.org/243953002/diff/10001/chrome/browser/infobars/infobar_manager.h File chrome/browser/infobars/infobar_manager.h (right): https://codereview.chromium.org/243953002/diff/10001/chrome/browser/infobars/infobar_manager.h#newcode27 chrome/browser/infobars/infobar_manager.h:27: virtual ~Observer() {} On 2014/04/19 19:46:04, Ilya Sherman wrote: ...
6 years, 8 months ago (2014-04-19 22:03:23 UTC) #6
Peter Kasting
LGTM https://codereview.chromium.org/243953002/diff/90001/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/243953002/diff/90001/chrome/browser/password_manager/password_manager_browsertest.cc#newcode102 chrome/browser/password_manager/password_manager_browsertest.cc:102: ->Accept(); Nit: This still isn't wrapped like the ...
6 years, 8 months ago (2014-04-21 20:34:05 UTC) #7
tfarina
https://codereview.chromium.org/243953002/diff/90001/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/243953002/diff/90001/chrome/browser/password_manager/password_manager_browsertest.cc#newcode102 chrome/browser/password_manager/password_manager_browsertest.cc:102: ->Accept(); On 2014/04/21 20:34:06, Peter Kasting wrote: > Nit: ...
6 years, 8 months ago (2014-04-21 20:56:40 UTC) #8
Ilya Sherman
LGTM, thanks.
6 years, 8 months ago (2014-04-21 21:57:32 UTC) #9
Ilya Sherman
The CQ bit was checked by isherman@chromium.org
6 years, 8 months ago (2014-04-21 21:57:37 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/243953002/130001
6 years, 8 months ago (2014-04-21 21:58:04 UTC) #11
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-21 22:48:54 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
6 years, 8 months ago (2014-04-21 22:48:55 UTC) #13
tfarina
The CQ bit was checked by tfarina@chromium.org
6 years, 8 months ago (2014-04-22 01:23:05 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/243953002/90002
6 years, 8 months ago (2014-04-22 01:23:21 UTC) #15
tfarina
The CQ bit was unchecked by tfarina@chromium.org
6 years, 8 months ago (2014-04-22 01:41:00 UTC) #16
tfarina
The CQ bit was checked by tfarina@chromium.org
6 years, 8 months ago (2014-04-22 03:21:20 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/243953002/160001
6 years, 8 months ago (2014-04-22 03:22:00 UTC) #18
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-22 04:02:59 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
6 years, 8 months ago (2014-04-22 04:03:00 UTC) #20
tfarina
The CQ bit was checked by tfarina@chromium.org
6 years, 8 months ago (2014-04-22 04:32:07 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/243953002/160001
6 years, 8 months ago (2014-04-22 04:32:27 UTC) #22
commit-bot: I haz the power
6 years, 8 months ago (2014-04-22 07:48:46 UTC) #23
Message was sent while issue was closed. 
Change committed as 265203

Powered by Google App Engine
This is Rietveld 408576698