|
|
DescriptionRemove ...LOADED_DEPRECATED and its friends
Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED
from ExtensionTestNotificationObserver.
Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad()
since it has a possibility to catch unexpected extension's loaded
notification. Instead of that, use TestExtensionRegistryObserver.
BUG=411568
TEST=browser_tests
Review-Url: https://codereview.chromium.org/2753513006
Cr-Commit-Position: refs/heads/master@{#464703}
Committed: https://chromium.googlesource.com/chromium/src/+/184283c3b2e690cf6839278e7dfb034fe98413b3
Patch Set 1 #
Total comments: 5
Patch Set 2 : To run test bot #Patch Set 3 : Back to DISABLED_ as original did #
Total comments: 2
Patch Set 4 : include .h #Messages
Total messages: 55 (38 generated)
Patchset #6 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== Remove loaded BUG= ========== to ========== Remove deprecated notification and its friends from extension_test_notification_observer BUG= ==========
The CQ bit was checked by limasdf@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove deprecated notification and its friends from extension_test_notification_observer BUG= ========== to ========== Remove deprecated notification and its friends BUG=411568 TEST=browser_tests ==========
Description was changed from ========== Remove deprecated notification and its friends BUG=411568 TEST=browser_tests ========== to ========== Remove deprecated extension notification and its friends BUG=411568 TEST=browser_tests ==========
Description was changed from ========== Remove deprecated extension notification and its friends BUG=411568 TEST=browser_tests ========== to ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED From ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has possibility to catch unexpected extension's loaded notification. instead of that, Use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ==========
Description was changed from ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED From ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has possibility to catch unexpected extension's loaded notification. instead of that, Use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ========== to ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED From ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has possibility to catch unexpected extension's loaded notification. instead of that, Use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ==========
Description was changed from ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED From ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has possibility to catch unexpected extension's loaded notification. instead of that, Use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ========== to ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED from ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has possibility to catch unexpected extension's loaded notification. Instead of that, use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ==========
Description was changed from ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED from ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has possibility to catch unexpected extension's loaded notification. Instead of that, use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ========== to ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED from ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has a possibility to catch unexpected extension's loaded notification. Instead of that, use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
limasdf@gmail.com changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin, Please take a look when you have time. https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_test_notification_observer.cc (right): https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_test_notification_observer.cc:5: #include <chrome/browser/extensions/chrome_extension_test_notification_observer.h> is this intended? I think < should be "
https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_test_notification_observer.cc (right): https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_test_notification_observer.cc:5: #include <chrome/browser/extensions/chrome_extension_test_notification_observer.h> On 2017/03/29 15:24:55, limasdf wrote: > is this intended? I think < should be " Yeah, this should be "". https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right): https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_crash_recovery_browsertest.cc:151: base::RunLoop().RunUntilIdle(); I think this is a little less deterministic than the previous method of listening to WebContents loading and checking them against the process manager; any reason to migrate away from that tactic (using WaitForExtensionViewsToLoad())?
Thanks for the comment. I'll ping you again when ready. (No updates yet. but I just want to let you know that I'm working on it.) https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right): https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_crash_recovery_browsertest.cc:151: base::RunLoop().RunUntilIdle(); On 2017/03/30 22:21:57, Devlin wrote: > I think this is a little less deterministic than the previous method of > listening to WebContents loading and checking them against the process manager; > any reason to migrate away from that tactic (using > WaitForExtensionViewsToLoad())? At the moment when WaitForExtensionViewsToloaded() called, ProcessManger::all_extension_frames_ doesn't holds the waiting extensions's frames. because of that, CheckExtensionConsistency(line#104) failed. I think I can solve this problem with ProcessManagerObserver.
Patchset #3 (id:260001) has been deleted
Patchset #2 (id:240001) has been deleted
Patchset #3 (id:300001) has been deleted
Patchset #2 (id:280001) has been deleted
Patchset #2 (id:320001) has been deleted
PTAL when you have time. https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right): https://codereview.chromium.org/2753513006/diff/220001/chrome/browser/extensi... chrome/browser/extensions/extension_crash_recovery_browsertest.cc:151: base::RunLoop().RunUntilIdle(); On 2017/04/03 13:52:50, limasdf wrote: > On 2017/03/30 22:21:57, Devlin wrote: > > I think this is a little less deterministic than the previous method of > > listening to WebContents loading and checking them against the process > manager; > > any reason to migrate away from that tactic (using > > WaitForExtensionViewsToLoad())? > > At the moment when WaitForExtensionViewsToloaded() called, > ProcessManger::all_extension_frames_ doesn't holds the waiting extensions's > frames. because of that, CheckExtensionConsistency(line#104) failed. > I think I can solve this problem with ProcessManagerObserver. Done.
Nice! As long as the bots are happy, lgtm. Thanks for the cleanup! :)
limasdf@gmail.com changed reviewers: + alemate@chromium.org, emaxx@chromium.org
Hi Alexander and emaxx. Could you please take a look this cleanup? Alexander -> chrome/browser/chromeos/first_run/drive_first_run_browsertest.cc emaxx -> chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc
LGTM with one note for chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc https://codereview.chromium.org/2753513006/diff/350001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc (right): https://codereview.chromium.org/2753513006/diff/350001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:30: #include "chrome/browser/extensions/chrome_test_extension_loader.cc" .h should be included instead of .cc
https://codereview.chromium.org/2753513006/diff/350001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc (right): https://codereview.chromium.org/2753513006/diff/350001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:30: #include "chrome/browser/extensions/chrome_test_extension_loader.cc" On 2017/04/07 10:05:25, emaxx wrote: > .h should be included instead of .cc Oops. fixed. Thanks!
I don't think I am owner of any of these. It also looks like there are enough "looks good". Removing myself from reviewers.
alemate@chromium.org changed reviewers: - alemate@chromium.org
The CQ bit was checked by limasdf@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2753513006/#ps370001 (title: "include .h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
limasdf@gmail.com changed reviewers: + alemate@chromium.org
Alexander, you're the owner of chrome/browser/chromeos/first_run/ the change is small I believe it doesn't take long time to review. PTAL.
limasdf@gmail.com changed reviewers: + oshima@chromium.org
Oshima, Would you mind taking a look hrome/browser/chromeos/first_run/drive_first_run_browsertest.cc?
lgtm
Thank you very much, Oshima!
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 370001, "attempt_start_ts": 1492155620339010, "parent_rev": "b9b0560ef757a95e871f72263e8932559a0e23d0", "commit_rev": "184283c3b2e690cf6839278e7dfb034fe98413b3"}
Message was sent while issue was closed.
Description was changed from ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED from ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has a possibility to catch unexpected extension's loaded notification. Instead of that, use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests ========== to ========== Remove ...LOADED_DEPRECATED and its friends Use ExtensionRegistryObserver instead of NOTIFICATION_EXTENSION_LOADED_DEPRECATED from ExtensionTestNotificationObserver. Also, Remove ExtensionTestNotificationObserver::WaitForExtensionLoad() since it has a possibility to catch unexpected extension's loaded notification. Instead of that, use TestExtensionRegistryObserver. BUG=411568 TEST=browser_tests Review-Url: https://codereview.chromium.org/2753513006 Cr-Commit-Position: refs/heads/master@{#464703} Committed: https://chromium.googlesource.com/chromium/src/+/184283c3b2e690cf6839278e7dfb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:370001) as https://chromium.googlesource.com/chromium/src/+/184283c3b2e690cf6839278e7dfb... |