Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431593008:
Remove deprecated extension notification from device_local_account_browsertest.cc (Closed)
|
Created:
5 years, 1 month ago by limasdf Modified:
5 years, 1 month ago Reviewers:
cschuet (SLOW) CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove deprecated extension notification from device_local_account_browsertest.cc
TEST=browser_tests --gtest_filter=DeviceLocalAccountTest.*
BUG=411568
Committed: https://crrev.com/9edb38eae6c5feec1243d3d6e620b03c8e4d9340
Cr-Commit-Position: refs/heads/master@{#360106}
Patch Set 1 : #Patch Set 2 : no busy waiting #
Total comments: 2
Patch Set 3 : check if already installed #Messages
Total messages: 23 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== [DO NOT SUBMIT] TO RUN TRYBOT BUG= ========== to ========== Remove deprecated extension notification from device_local_account_browsertest.cc BUG=411568 ==========
limasdf@gmail.com changed reviewers: + cschuet@chromium.org
Please take a look.
On 2015/11/09 09:49:57, limasdf wrote: > Please take a look. Could you provide some context on this CL? I understand that NOTIFICATION_EXTENSION_WILL_BE_INSTALLED_DEPRECATED will be deprecated, but is really the (not quite busy) wait the only way to write this test now?
On 2015/11/09 22:05:53, cschuet wrote: > On 2015/11/09 09:49:57, limasdf wrote: > > Please take a look. > > Could you provide some context on this CL? I understand that > NOTIFICATION_EXTENSION_WILL_BE_INSTALLED_DEPRECATED will be deprecated, but is > really the (not quite busy) wait the only way to write this test now? I normally use extensions::TestExtensionRegistryObserver to listen the extension is installed. (TestExtensionRegistryObserver receive ExtensionRegistry from contructor.) https://codereview.chromium.org/1422803010/ The problem is StartLogin() on line#1056 create new ExtensionRegistry. so that previously declared instance of extensions::TestExtensionRegistryObserver cannot listen the extension installed with newly created ExtensionRegistry. Sorry about my bad English, by the way.
Kindly ping.
On 2015/11/13 11:42:11, limasdf wrote: > Kindly ping. Sorry for the delay. I see. Unfortunately we can't do the busy wait. You are right in that the StartLogin() creates a new user and as such a new ExtensionRegistry. So you will have to attach your observer to this newly created ExtensionRegistry. The tricky thing is that the creation of this ExtensionRegistry happens asynchronously (LoginPerformer::LoginAsPublicSession). So you will have to find a way to wait for the login to have successfully happened before you can retrieve the ExtensionRegistry() to first check whether the install has already happened and if not install your Observer which waits for the installation finished notification. Does that makes sense?
all right. now there's no busy waiting. Listening profile creation first and then listen exntension installation with newly created profile. PTAL!
On 2015/11/16 19:28:52, limasdf wrote: > all right. now there's no busy waiting. > > Listening profile creation first and then listen exntension installation with > newly created profile. > > PTAL! Cool thanks a lot - this looks much better.
https://codereview.chromium.org/1431593008/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/1431593008/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_browsertest.cc:845: DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED, type); You should probably check whether the app has already been installed here. Maybe we have missed the OnExtensionWillBeInstalled aleady?
Description was changed from ========== Remove deprecated extension notification from device_local_account_browsertest.cc BUG=411568 ========== to ========== Remove deprecated extension notification from device_local_account_browsertest.cc TEST=browser_tests --gtest_filter=DeviceLocalAccountTest.* BUG=411568 ==========
adding a code that checking 'already installed'. Thanks you for the review again! :) PTAL. https://codereview.chromium.org/1431593008/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/1431593008/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_browsertest.cc:845: DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED, type); On 2015/11/17 15:02:48, cschuet wrote: > You should probably check whether the app has already been installed here. Maybe > we have missed the OnExtensionWillBeInstalled aleady? Done.
On 2015/11/17 17:00:57, limasdf wrote: > adding a code that checking 'already installed'. > > Thanks you for the review again! :) > > PTAL. > > https://codereview.chromium.org/1431593008/diff/140001/chrome/browser/chromeo... > File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right): > > https://codereview.chromium.org/1431593008/diff/140001/chrome/browser/chromeo... > chrome/browser/chromeos/policy/device_local_account_browsertest.cc:845: > DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED, type); > On 2015/11/17 15:02:48, cschuet wrote: > > You should probably check whether the app has already been installed here. > Maybe > > we have missed the OnExtensionWillBeInstalled aleady? > > Done. LGTM ! Thanks for fixing this.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431593008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431593008/160001
Message was sent while issue was closed.
Committed patchset #3 (id:160001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9edb38eae6c5feec1243d3d6e620b03c8e4d9340 Cr-Commit-Position: refs/heads/master@{#360106} |
