|
|
DescriptionAdd PreloadCheck class for extension pre-install checks
PreloadCheck is an abstract preload check for an extension that may
be asynchronous. The idea is to de-duplicate ad-hoc checks into
PreloadCheck subclasses, then use these classes from places like
ExtensionInstallChecker, CrxInstaller, and ManagementAPI.
This will enable some of these classes to be moved into //extensions,
because we would be able to pick and choose which checks to run. E.g.,
AppShell would only use checks from //extensions, while Chrome would
also add checks from //chrome like BlacklistCheck.
See go/loading-extensions-in-appshell for more details on how this
relates to AppShell and launching apps/extensions.
BUG=679971
Review-Url: https://codereview.chromium.org/2693373003
Cr-Commit-Position: refs/heads/master@{#457943}
Committed: https://chromium.googlesource.com/chromium/src/+/c0145e6a749a559b1c50ecf2176f9ef0fce68c09
Patch Set 1 #Patch Set 2 : getting back to this #Patch Set 3 : review ready #
Total comments: 28
Patch Set 4 : devlin feedback #Patch Set 5 : missed one #Patch Set 6 : ...and the rest #Patch Set 7 : rebase #
Total comments: 26
Patch Set 8 : rebase #Patch Set 9 : use ExtensionsTest #Patch Set 10 : devlin #
Total comments: 14
Patch Set 11 : devlin 2 #Patch Set 12 : rebase #Patch Set 13 : fwd declare #Patch Set 14 : rebase #Patch Set 15 : don't break enable_extensions=false #Dependent Patchsets: Messages
Total messages: 31 (13 generated)
Description was changed from ========== Add AsyncChecker class for extension install checks This is a sample of the AsyncChecker design from go/loading-extensions-in-appshell. More for design criticque than actual code review for now. AsyncChecker: Encapsulated, possibly async validation check. AsyncCheckerManager: Given a bunch of AsyncCheckers, runs them in parallel, calling |callback| with any errors received once all have completed (or, if |fail_fast|, upon the first error, stopping other checks). COMMIT=false ========== to ========== Add PreloadCheck class for extension pre-install checks This is a sample of the PreloadCheck design from go/loading-extensions-in-appshell. More for design critique than actual code review for now. PreloadCheck: Encapsulated, possibly async validation check. PreloadCheckManager: Given a bunch of PreloadChecks, runs them in parallel, calling |callback| with any errors received once all have completed (or, if |fail_fast|, upon the first error, stopping other checks). COMMIT=false ==========
Message was sent while issue was closed.
Description was changed from ========== Add PreloadCheck class for extension pre-install checks This is a sample of the PreloadCheck design from go/loading-extensions-in-appshell. More for design critique than actual code review for now. PreloadCheck: Encapsulated, possibly async validation check. PreloadCheckManager: Given a bunch of PreloadChecks, runs them in parallel, calling |callback| with any errors received once all have completed (or, if |fail_fast|, upon the first error, stopping other checks). COMMIT=false ========== to ========== Add PreloadCheck class for extension pre-install checks PreloadCheck is an abstract preload check for an extension that may be asynchronous. The idea is to de-duplicate ad-hoc checks into PreloadCheck subclasses, then use these classes from places like ExtensionInstallChecker, CrxInstaller, and ManagementAPI. This will enable some of these classes to be moved into //extensions, because we would be able to pick and choose which checks to run. E.g., AppShell would only use checks from //extensions, while Chrome would also add checks from //chrome like BlacklistCheck. See go/loading-extensions-in-appshell for more details on how this relates to AppShell and launching apps/extensions. ==========
Message was sent while issue was closed.
michaelpg@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, PTAL. I'm kind of sad this ended up being so big, but it'll be offset a bit by other code we'll be able to remove (some of which will happen in the next patch)
Description was changed from ========== Add PreloadCheck class for extension pre-install checks PreloadCheck is an abstract preload check for an extension that may be asynchronous. The idea is to de-duplicate ad-hoc checks into PreloadCheck subclasses, then use these classes from places like ExtensionInstallChecker, CrxInstaller, and ManagementAPI. This will enable some of these classes to be moved into //extensions, because we would be able to pick and choose which checks to run. E.g., AppShell would only use checks from //extensions, while Chrome would also add checks from //chrome like BlacklistCheck. See go/loading-extensions-in-appshell for more details on how this relates to AppShell and launching apps/extensions. ========== to ========== Add PreloadCheck class for extension pre-install checks PreloadCheck is an abstract preload check for an extension that may be asynchronous. The idea is to de-duplicate ad-hoc checks into PreloadCheck subclasses, then use these classes from places like ExtensionInstallChecker, CrxInstaller, and ManagementAPI. This will enable some of these classes to be moved into //extensions, because we would be able to pick and choose which checks to run. E.g., AppShell would only use checks from //extensions, while Chrome would also add checks from //chrome like BlacklistCheck. See go/loading-extensions-in-appshell for more details on how this relates to AppShell and launching apps/extensions. BUG=679971 ==========
So, overall, I think this is pretty nice. It's clean and concise and we've probably needed a common loading-check type thing for a long time. That said, - Without any uses of this, it's hard to comment on the API of PreloadCheck. e.g., GetErrorMessage() seems a bit funky, but I can't tell without knowing where it will be used. - I'm worried about the duplication we have with the existing checks. Could we instead hook this up for at least one of these checks so we actually use it, and see where it fits in? We don't have to convert everything in one fell swoop, of course, but I think it'll be easier to see how it all fits together that way and maybe catch something we otherwise wouldn't. https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/blacklist_check.cc (right): https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/blacklist_check.cc:9: #include "extensions/common/constants.h" Needed? https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/blacklist_check.cc:26: void BlacklistCheck::OnGetIsBlacklisted(BlacklistState blacklist_state) { OnGetIsBlacklisted sounds strange - maybe OnRetrievedBlacklistState or something? https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/blacklist_check.cc:32: std::move(callback_).Run(errors); clever - but std::move() is designed to leave things in an undefined state (to allow for whatever optimization is fastest). As a OnceCallback, is the internal state not cleaned up after calling Run()? https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check.cc:30: if (!error || !error_.size()) Why do we want to handle null string16 pointers? Seems like something we shouldn't allow. I also wouldn't be opposed to just returning a base::string16 and using empty to signify no error, but it's hard to say without a bit more context. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check.h (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check.h:33: mutable base::string16 error_; why not just un-constify GetErrorMessage? https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:71: ExtensionsBrowserClient::Set(&extensions_browser_client_); This not being torn down causes badness. Additionally, unittests that run in the same process will share this same test client - is that what you want? https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:95: extensions::ExtensionSystem::Get(&context_) no need for extensions:: namespace https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... File extensions/browser/preload_check.h (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check.h:15: #include "extensions/common/constants.h" needed? https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check.h:36: typedef std::unordered_set<Error, std::hash<int>> Errors; prefer using x = foo syntax in new code. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check.h:54: const Extension* extension_; We should refcount this. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... File extensions/browser/preload_check_test_util.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check_test_util.cc:12: void PreloadCheckObserver::OnCheckComplete(PreloadCheck::Errors errors) { ASSERT_TRUE(errors_.empty())? https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... File extensions/browser/preload_check_test_util.h (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check_test_util.h:25: base::RunLoop().RunUntilIdle(); We already have the object file, so may as well move this definition. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check_test_util.h:33: int call_count_; DISALLOW_COPY_AND_ASSIGN
On 2017/03/08 03:01:50, Devlin wrote: > So, overall, I think this is pretty nice. It's clean and concise and we've > probably needed a common loading-check type thing for a long time. That said, > - Without any uses of this, it's hard to comment on the API of PreloadCheck. The dependent CL has some usage: https://codereview.chromium.org/2737053002 I agree GetErrorMessage() is a bit weird right now especially with how I'm intending to use it in composed PreloadChecks. > e.g., GetErrorMessage() seems a bit funky, but I can't tell without knowing > where it will be used. > - I'm worried about the duplication we have with the existing checks. > > Could we instead hook this up for at least one of these checks so we actually > use it, and see where it fits in? We don't have to convert everything in one > fell swoop, of course, but I think it'll be easier to see how it all fits > together that way and maybe catch something we otherwise wouldn't. > > https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/blacklist_check.cc (right): > > https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/blacklist_check.cc:9: #include > "extensions/common/constants.h" > Needed? > > https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/blacklist_check.cc:26: void > BlacklistCheck::OnGetIsBlacklisted(BlacklistState blacklist_state) { > OnGetIsBlacklisted sounds strange - maybe OnRetrievedBlacklistState or > something? > > https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/blacklist_check.cc:32: > std::move(callback_).Run(errors); > clever - but std::move() is designed to leave things in an undefined state (to > allow for whatever optimization is fastest). As a OnceCallback, is the internal > state not cleaned up after calling Run()? > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > File extensions/browser/policy_check.cc (right): > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > extensions/browser/policy_check.cc:30: if (!error || !error_.size()) > Why do we want to handle null string16 pointers? Seems like something we > shouldn't allow. > > I also wouldn't be opposed to just returning a base::string16 and using empty to > signify no error, but it's hard to say without a bit more context. > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > File extensions/browser/policy_check.h (right): > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > extensions/browser/policy_check.h:33: mutable base::string16 error_; > why not just un-constify GetErrorMessage? > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > File extensions/browser/policy_check_unittest.cc (right): > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > extensions/browser/policy_check_unittest.cc:71: > ExtensionsBrowserClient::Set(&extensions_browser_client_); > This not being torn down causes badness. > > Additionally, unittests that run in the same process will share this same test > client - is that what you want? > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... > extensions/browser/policy_check_unittest.cc:95: > extensions::ExtensionSystem::Get(&context_) > no need for extensions:: namespace > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > File extensions/browser/preload_check.h (right): > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > extensions/browser/preload_check.h:15: #include "extensions/common/constants.h" > needed? > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > extensions/browser/preload_check.h:36: typedef std::unordered_set<Error, > std::hash<int>> Errors; > prefer using x = foo syntax in new code.https://codereview.chromium.org/2737053002#ps1 > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > extensions/browser/preload_check.h:54: const Extension* extension_; > We should refcount this. > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > File extensions/browser/preload_check_test_util.cc (right): > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > extensions/browser/preload_check_test_util.cc:12: void > PreloadCheckObserver::OnCheckComplete(PreloadCheck::Errors errors) { > ASSERT_TRUE(errors_.empty())? > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > File extensions/browser/preload_check_test_util.h (right): > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > extensions/browser/preload_check_test_util.h:25: base::RunLoop().RunUntilIdle(); > We already have the object file, so may as well move this definition. > > https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... > extensions/browser/preload_check_test_util.h:33: int call_count_; > DISALLOW_COPY_AND_ASSIGN
Thanks for the feedback. Mind helping me out with the TestExtensionBrowserClient stuff? https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/blacklist_check.cc (right): https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/blacklist_check.cc:9: #include "extensions/common/constants.h" On 2017/03/08 03:01:44, Devlin wrote: > Needed? Removed. (Originally had the error codes in there, but they seem fine in preload_check.h.) https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/blacklist_check.cc:26: void BlacklistCheck::OnGetIsBlacklisted(BlacklistState blacklist_state) { On 2017/03/08 03:01:44, Devlin wrote: > OnGetIsBlacklisted sounds strange - maybe OnRetrievedBlacklistState or > something? Done. https://codereview.chromium.org/2693373003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/blacklist_check.cc:32: std::move(callback_).Run(errors); On 2017/03/08 03:01:44, Devlin wrote: > clever - but std::move() is designed to leave things in an undefined state (to > allow for whatever optimization is fastest). As a OnceCallback, is the internal > state not cleaned up after calling Run()? Those considerations don't apply to OnceCallback. Making this work and understanding it was a big pain point. Here's my explanation: It isn't optional; OnceCallback::Run *without* the move doesn't compile: https://cs.chromium.org/chromium/src/base/callback.h?type=cs&q=callback.h&sq=... callback_ being an lvalue, callback_.Run() would select OnceCallback:Run()&. std::move(callback_) makes it usable as rvalue for Run()&&. docs/callback.md has examples (also attempting to fix some of the documentation: https://codereview.chromium.org/2735903002/) Since Run()&& is selected and that calls operator=(OnceCallback&&), callback_'s members are fully moved out of it, so it's not left in an unspecified state. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check.cc:30: if (!error || !error_.size()) On 2017/03/08 03:01:45, Devlin wrote: > Why do we want to handle null string16 pointers? Seems like something we > shouldn't allow. you're right, that seems pointless, and it's unclear whether that case would return true or false. > > I also wouldn't be opposed to just returning a base::string16 and using empty to > signify no error, but it's hard to say without a bit more context. That's definitely simpler. Not sure what my motivation was for this. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check.h (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check.h:33: mutable base::string16 error_; On 2017/03/08 03:01:45, Devlin wrote: > why not just un-constify GetErrorMessage? GetErrorMessage shouldn't modify logical state. But error_ is only being copied anyway. Removed mutable. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:71: ExtensionsBrowserClient::Set(&extensions_browser_client_); On 2017/03/08 03:01:46, Devlin wrote: > This not being torn down causes badness. > > Additionally, unittests that run in the same process will share this same test > client - is that what you want? I'm not quite sure what you mean/how the unit test process model works. Do unrelated tests really run in parallel threads in the same process? Chrome uses so much global state that I wouldn't expect that to work. I figure I'm doing something similar to this: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_reen... https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:95: extensions::ExtensionSystem::Get(&context_) On 2017/03/08 03:01:46, Devlin wrote: > no need for extensions:: namespace Done. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... File extensions/browser/preload_check.h (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check.h:15: #include "extensions/common/constants.h" On 2017/03/08 03:01:47, Devlin wrote: > needed? Removed. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check.h:36: typedef std::unordered_set<Error, std::hash<int>> Errors; On 2017/03/08 03:01:46, Devlin wrote: > prefer using x = foo syntax in new code. TIL, done https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check.h:54: const Extension* extension_; On 2017/03/08 03:01:48, Devlin wrote: > We should refcount this. Done. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... File extensions/browser/preload_check_test_util.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check_test_util.cc:12: void PreloadCheckObserver::OnCheckComplete(PreloadCheck::Errors errors) { On 2017/03/08 03:01:48, Devlin wrote: > ASSERT_TRUE(errors_.empty())? CHECK? (I hesitate to #include gtest.h here.) https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... File extensions/browser/preload_check_test_util.h (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check_test_util.h:25: base::RunLoop().RunUntilIdle(); On 2017/03/08 03:01:48, Devlin wrote: > We already have the object file, so may as well move this definition. Doh. https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/prel... extensions/browser/preload_check_test_util.h:33: int call_count_; On 2017/03/08 03:01:49, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:71: ExtensionsBrowserClient::Set(&extensions_browser_client_); On 2017/03/09 01:53:30, michaelpg wrote: > On 2017/03/08 03:01:46, Devlin wrote: > > This not being torn down causes badness. > > > > Additionally, unittests that run in the same process will share this same test > > client - is that what you want? > > I'm not quite sure what you mean/how the unit test process model works. Do > unrelated tests really run in parallel threads in the same process? Chrome uses > so much global state that I wouldn't expect that to work. > > I figure I'm doing something similar to this: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_reen... Here's an example from //extensions/browser that is better than what I'm doing, but still sets the global ExtensionBrowserClient's ExtensionSystemFactory to a fake: https://cs.chromium.org/chromium/src/extensions/browser/updater/update_servic... Can I follow an example like that test, or wouldn't that cause problems for other tests run simultaneously in the same process? Appreciate some help here :-\
https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:71: ExtensionsBrowserClient::Set(&extensions_browser_client_); On 2017/03/13 19:18:30, michaelpg wrote: > On 2017/03/09 01:53:30, michaelpg wrote: > > On 2017/03/08 03:01:46, Devlin wrote: > > > This not being torn down causes badness. > > > > > > Additionally, unittests that run in the same process will share this same > test > > > client - is that what you want? > > > > I'm not quite sure what you mean/how the unit test process model works. Do > > unrelated tests really run in parallel threads in the same process? Chrome > uses > > so much global state that I wouldn't expect that to work. > > > > I figure I'm doing something similar to this: > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_reen... > > Here's an example from //extensions/browser that is better than what I'm doing, > but still sets the global ExtensionBrowserClient's ExtensionSystemFactory to a > fake: > > https://cs.chromium.org/chromium/src/extensions/browser/updater/update_servic... > > Can I follow an example like that test, or wouldn't that cause problems for > other tests run simultaneously in the same process? Appreciate some help here > :-\ Looks like ExtensionsTest itself calls ExtensionsBrowserClient::Set with a new TestExtensionsBrowserClient in SetUp(), so two of those running in parallel in the same process would be a problem. But these tests *seem* to run in sequence within a particular process (i'm only seeing parallelization at the process level). (Which makes sense as otherwise the tests would be a lot crashier.) I'll derive from ExtensionsTest (so I don't have to set up and tear down manually) and see how that goes.
https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/40001/extensions/browser/poli... extensions/browser/policy_check_unittest.cc:71: ExtensionsBrowserClient::Set(&extensions_browser_client_); On 2017/03/13 19:18:30, michaelpg wrote: > On 2017/03/09 01:53:30, michaelpg wrote: > > On 2017/03/08 03:01:46, Devlin wrote: > > > This not being torn down causes badness. > > > > > > Additionally, unittests that run in the same process will share this same > test > > > client - is that what you want? > > > > I'm not quite sure what you mean/how the unit test process model works. Do > > unrelated tests really run in parallel threads in the same process? Chrome > uses > > so much global state that I wouldn't expect that to work. > > > > I figure I'm doing something similar to this: > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_reen... > > Here's an example from //extensions/browser that is better than what I'm doing, > but still sets the global ExtensionBrowserClient's ExtensionSystemFactory to a > fake: > > https://cs.chromium.org/chromium/src/extensions/browser/updater/update_servic... > > Can I follow an example like that test, or wouldn't that cause problems for > other tests run simultaneously in the same process? Appreciate some help here > :-\ Looks like ExtensionsTest itself calls ExtensionsBrowserClient::Set with a new TestExtensionsBrowserClient in SetUp(), so two of those running in parallel in the same process would be a problem. But these tests *seem* to run in sequence within a particular process (i'm only seeing parallelization at the process level). (Which makes sense as otherwise the tests would be a lot crashier.) I'll derive from ExtensionsTest (so I don't have to set up and tear down manually) and see how that goes.
PTAnotherL -- see how PS9 differs from PS8 for the test fix.
I'm still not 100% sure we'll stick with this interface, but I think it's close enough that we can go with it and iterate. :) Sorry, I haven't had a chance to dive into the testing process model in the last few days. My recollection was that setting global state in unittests is bad because we, somehow, run multiple in the same process space - but not unconditionally. Maybe something like running different test cases in different processes? I'll see if I can't look it up again tomorrow. https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/blacklist_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:5: #include "chrome/browser/extensions/blacklist_check.h" grr... git cl format's include reordering isn't quite right, I think. \n here. https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:33: test_blacklist_.SetBlacklistState(extension_->id(), state, true); doc anonymous bool https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:43: void StopBlacklistCheck() { blacklist_check_.reset(); } nit: maybe s/Stop/Reset||Destroy? "Stop" seems so peaceful. https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:67: EXPECT_EQ(1u, observer.errors().size()); You could substitute these with: EXPECT_THAT(observer.errors(), testing::UnorderedElementsAre(PreloadCheck::BLACKLISTED_ID)); so that you don't have to check size and contents (and it prints out slightly more useful error messages). https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/BUI... File extensions/browser/BUILD.gn (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/BUI... extensions/browser/BUILD.gn:369: source_set("test_support") { do we need this in test_support (instead of unittests)? https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:52: class TestExtensionSystem : public MockExtensionSystem { I'd slightly prefer that we expose a SetManagementPolicy() method on TestExtensionSystem, which would also ensure we use the same management policy internally (in e.g. CreateExtensionService). https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check.h (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check.h:36: using Errors = std::unordered_set<Error, std::hash<int>>; Any reason to use unordered_set here? Seems like for the number of errors we'd get, either std::set<> (for its canonicalism) or base::flat_set (for its speed/optimization) would be more appropriate. https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check_test_util.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:16: CHECK(!called_); Can we ASSERT_FALSE here? https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:25: base::RunLoop().RunUntilIdle(); This can be pretty finicky, given post task complexities. Often what works better is a model where we have a run_loop_ member, and then do: void PreloadCheckObserver::OnCheckComplete(...) { run_loop_.Quit(); ... } void PreloadCheckObserver::Wait() { run_loop_.Run(); } (RunLoop::Run() also has the nice property of returning if Quit() has already been called.) https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check_test_util.h (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.h:18: virtual void OnCheckComplete(PreloadCheck::Errors errors); Given the common utility of this class, it seems like we might be able to have this be more streamlined. An easy case would be a GetCallback() method that returns base::Bind(&OnCheckComplete, base::Unretained(this)) so that you could just run with Start(observer.GetCheck()). A more elaborate version would be allowing to inline the start and wait functionality in a RunUntilComplete() type method, e.g. void RunUntilComplete(PreloadCheck* check) { check->Start(base::Bind(...)); base::RunLoop().Run(); } and then you could just have Observer observer; observer.RunUntilComplete(check); (Having a combination of these can also make sense, since you might not want to always wait in a blocking fashion.)
Thanks for the thoughtful comments. The tests look better but I did have some specific questions. The next CLs will definitely be smoother after these changes too. https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/blacklist_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:5: #include "chrome/browser/extensions/blacklist_check.h" On 2017/03/14 01:44:34, Devlin wrote: > grr... git cl format's include reordering isn't quite right, I think. \n here. Done. https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:33: test_blacklist_.SetBlacklistState(extension_->id(), state, true); On 2017/03/14 01:44:34, Devlin wrote: > doc anonymous bool Done. https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:43: void StopBlacklistCheck() { blacklist_check_.reset(); } On 2017/03/14 01:44:34, Devlin wrote: > nit: maybe s/Stop/Reset||Destroy? "Stop" seems so peaceful. removed, but renamed test s/Stop/Reset https://codereview.chromium.org/2693373003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:67: EXPECT_EQ(1u, observer.errors().size()); On 2017/03/14 01:44:34, Devlin wrote: > You could substitute these with: > EXPECT_THAT(observer.errors(), > testing::UnorderedElementsAre(PreloadCheck::BLACKLISTED_ID)); > > so that you don't have to check size and contents (and it prints out slightly > more useful error messages). sweet! https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/BUI... File extensions/browser/BUILD.gn (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/BUI... extensions/browser/BUILD.gn:369: source_set("test_support") { On 2017/03/14 01:44:34, Devlin wrote: > do we need this in test_support (instead of unittests)? this is how I'm getting these sources in chrome/test/BUILD.gn (ie blacklist_check_unittest.cc) https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:52: class TestExtensionSystem : public MockExtensionSystem { On 2017/03/14 01:44:34, Devlin wrote: > I'd slightly prefer that we expose a SetManagementPolicy() method on > TestExtensionSystem, which would also ensure we use the same management policy > internally (in e.g. CreateExtensionService). Not sure what you're referring to here. I see a CreateExtensionService in //chrome/browser/extensions/extension_service_test_base.h but nothing in //extensions. https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check.h (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check.h:36: using Errors = std::unordered_set<Error, std::hash<int>>; On 2017/03/14 01:44:34, Devlin wrote: > Any reason to use unordered_set here? Seems like for the number of errors we'd > get, either std::set<> (for its canonicalism) or base::flat_set (for its > speed/optimization) would be more appropriate. /shrug/ std::set sgtm https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check_test_util.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:16: CHECK(!called_); On 2017/03/14 01:44:34, Devlin wrote: > Can we ASSERT_FALSE here? I'd prefer not to #include gtest.h here. I'd rather leave the assertions to the test. https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:25: base::RunLoop().RunUntilIdle(); On 2017/03/14 01:44:34, Devlin wrote: > This can be pretty finicky, given post task complexities. Often what works > better is a model where we have a run_loop_ member, and then do: Can you expand on "finicky", please? I copied this from some existing code, so I'll admit I don't know the ins and outs of MessageLoops. But run_loop.h itself suggests: "Create a RunLoop on the stack and call Run/Quit to run a nested MessageLoop." I believe that's what this is doing. It seems clear to me that Wait() blocks until posted tasks have finished, at which point I can verify whether OnCheckComplete() was called. Having a RunLoop member is less clear to me: the loop will block until OnCheckComplete is recursively called to quit the loop? What happens if OnCheckComplete is never called? Take a look at BlacklistCheckTest::StopCheck -- renamed to ResetCheck in new patch. This never terminates if Wait() calls run_loop_.Run(), presumably because OnCheckComplete is never called so the loop is neither explicitly Quit(), nor configured to quit when idle. > void PreloadCheckObserver::OnCheckComplete(...) { > run_loop_.Quit(); > ... > } > > void PreloadCheckObserver::Wait() { > run_loop_.Run(); > } > > (RunLoop::Run() also has the nice property of returning if Quit() has already > been called.) https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check_test_util.h (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.h:18: virtual void OnCheckComplete(PreloadCheck::Errors errors); On 2017/03/14 01:44:34, Devlin wrote: > Given the common utility of this class, it seems like we might be able to have > this be more streamlined. An easy case would be a GetCallback() method that > returns base::Bind(&OnCheckComplete, base::Unretained(this)) so that you could > just run with Start(observer.GetCheck()). A more elaborate version would be > allowing to inline the start and wait functionality in a RunUntilComplete() type > method, e.g. > void RunUntilComplete(PreloadCheck* check) { > check->Start(base::Bind(...)); > base::RunLoop().Run(); > } > > and then you could just have > Observer observer; > observer.RunUntilComplete(check); > > (Having a combination of these can also make sense, since you might not want to > always wait in a blocking fashion.) Good idea. Renamed to Runner and added some methods.
A few more nits, but I think they're all pretty straightforward, so lgtm. Lemme know if you have any questions. :) https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:52: class TestExtensionSystem : public MockExtensionSystem { On 2017/03/14 21:58:30, michaelpg wrote: > On 2017/03/14 01:44:34, Devlin wrote: > > I'd slightly prefer that we expose a SetManagementPolicy() method on > > TestExtensionSystem, which would also ensure we use the same management policy > > internally (in e.g. CreateExtensionService). > > Not sure what you're referring to here. I see a CreateExtensionService in > //chrome/browser/extensions/extension_service_test_base.h but nothing in > //extensions. Nevermind. I forgot we had MockExtensionSystem *and* TestExtensionSystem. Gah. This is fine. [Edit] Though this may be moot with the new comment in policy_check. https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check_test_util.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:16: CHECK(!called_); On 2017/03/14 21:58:30, michaelpg wrote: > On 2017/03/14 01:44:34, Devlin wrote: > > Can we ASSERT_FALSE here? > > I'd prefer not to #include gtest.h here. I'd rather leave the assertions to the > test. Any particular reason? It's already a test-only file. (The reason I prefer ASSERT here is because CHECK kills the process quite rudely, slowing down subsequent tests, whereas ASSERT gracefully ends the test.) https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:25: base::RunLoop().RunUntilIdle(); On 2017/03/14 21:58:30, michaelpg wrote: > On 2017/03/14 01:44:34, Devlin wrote: > > This can be pretty finicky, given post task complexities. Often what works > > better is a model where we have a run_loop_ member, and then do: > > Can you expand on "finicky", please? I copied this from some existing code, so > I'll admit I don't know the ins and outs of MessageLoops. But run_loop.h itself > suggests: "Create a RunLoop on the stack and call Run/Quit to run a nested > MessageLoop." > > I believe that's what this is doing. It seems clear to me that Wait() blocks > until posted tasks have finished, at which point I can verify whether > OnCheckComplete() was called. > > Having a RunLoop member is less clear to me: the loop will block until > OnCheckComplete is recursively called to quit the loop? What happens if > OnCheckComplete is never called? > > Take a look at BlacklistCheckTest::StopCheck -- renamed to ResetCheck in new > patch. This never terminates if Wait() calls run_loop_.Run(), presumably because > OnCheckComplete is never called so the loop is neither explicitly Quit(), nor > configured to quit when idle. > > > void PreloadCheckObserver::OnCheckComplete(...) { > > run_loop_.Quit(); > > ... > > } > > > > void PreloadCheckObserver::Wait() { > > run_loop_.Run(); > > } > > > > (RunLoop::Run() also has the nice property of returning if Quit() has already > > been called.) > The finicky bits are when RunUntilIdle() doesn't actually run long enough (i.e., until whatever you want to happen, has happened). This won't commonly happen on the main thread, but if you have multiple threads in play (e.g. reading something from the file system), the main thread could be idle before the file system read is complete. There's also utilities for that (content::RunAllBlockingPoolTasksUntilIdle or something like that), but even that has subtleties. And, as we get farther out, none of those would account for if what you're waiting for is dependent on another process (such as waiting for an extension to act in the renderer and notify the browser). In most cases like this, what you really want to do is say "Run until [x] happens", rather than "run the tasks you immediately know about in the message loop" (though there are some exceptions). So, that's the finicky bit. But I forgot about the StopCheck() case, which is indeed a pain. There are ways around it... but none pretty. I'm okay with this for now, for lack of better alternatives for StopCheck(), but it's worth noting (perhaps adding a comment?) that if these checks ever get complex, a single RunUntilIdle() call may be insufficient - possibly leading to flakiness. https://codereview.chromium.org/2693373003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/blacklist_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:41: PreloadCheckRunner runner_; nit: it'd be my preference to expose this through an accessor and keep this private. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... File extensions/browser/policy_check.cc (right): https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... extensions/browser/policy_check.cc:13: PolicyCheck::PolicyCheck(content::BrowserContext* context, nit: Rather than pass in the context here, maybe pass in the management policy directly? Saves a bit of mocking in the test, and keeps the class a bit more narrow. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:29: const base::string16 kDummyPolicyError = can't have static non-POD. Just go ahead and inline this in UserMayLoad. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:110: // Test an invalid extension. s/invalid/disallowed (and analogous for valid on line 96) https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... File extensions/browser/preload_check.h (right): https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... extensions/browser/preload_check.h:8: #include <memory> Needed? https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... extensions/browser/preload_check.h:10: #include <utility> needed? https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... extensions/browser/preload_check.h:16: #include "extensions/common/extension.h" nit: forward declaration instead of this include
phew https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:52: class TestExtensionSystem : public MockExtensionSystem { On 2017/03/16 01:42:44, Devlin wrote: > On 2017/03/14 21:58:30, michaelpg wrote: > > On 2017/03/14 01:44:34, Devlin wrote: > > > I'd slightly prefer that we expose a SetManagementPolicy() method on > > > TestExtensionSystem, which would also ensure we use the same management > policy > > > internally (in e.g. CreateExtensionService). > > > > Not sure what you're referring to here. I see a CreateExtensionService in > > //chrome/browser/extensions/extension_service_test_base.h but nothing in > > //extensions. > > Nevermind. I forgot we had MockExtensionSystem *and* TestExtensionSystem. Gah. > > This is fine. > > [Edit] Though this may be moot with the new comment in policy_check. Acknowledged. https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... File extensions/browser/preload_check_test_util.cc (right): https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:16: CHECK(!called_); On 2017/03/16 01:42:44, Devlin wrote: > On 2017/03/14 21:58:30, michaelpg wrote: > > On 2017/03/14 01:44:34, Devlin wrote: > > > Can we ASSERT_FALSE here? > > > > I'd prefer not to #include gtest.h here. I'd rather leave the assertions to > the > > test. > > Any particular reason? It's already a test-only file. (The reason I prefer > ASSERT here is because CHECK kills the process quite rudely, slowing down > subsequent tests, whereas ASSERT gracefully ends the test.) Meh. The crash does look uglier, so Done. https://codereview.chromium.org/2693373003/diff/120001/extensions/browser/pre... extensions/browser/preload_check_test_util.cc:25: base::RunLoop().RunUntilIdle(); On 2017/03/16 01:42:44, Devlin wrote: > On 2017/03/14 21:58:30, michaelpg wrote: > > On 2017/03/14 01:44:34, Devlin wrote: > > > This can be pretty finicky, given post task complexities. Often what works > > > better is a model where we have a run_loop_ member, and then do: > > > > Can you expand on "finicky", please? I copied this from some existing code, so > > I'll admit I don't know the ins and outs of MessageLoops. But run_loop.h > itself > > suggests: "Create a RunLoop on the stack and call Run/Quit to run a nested > > MessageLoop." > > > > I believe that's what this is doing. It seems clear to me that Wait() blocks > > until posted tasks have finished, at which point I can verify whether > > OnCheckComplete() was called. > > > > Having a RunLoop member is less clear to me: the loop will block until > > OnCheckComplete is recursively called to quit the loop? What happens if > > OnCheckComplete is never called? > > > > Take a look at BlacklistCheckTest::StopCheck -- renamed to ResetCheck in new > > patch. This never terminates if Wait() calls run_loop_.Run(), presumably > because > > OnCheckComplete is never called so the loop is neither explicitly Quit(), nor > > configured to quit when idle. > > > > > void PreloadCheckObserver::OnCheckComplete(...) { > > > run_loop_.Quit(); > > > ... > > > } > > > > > > void PreloadCheckObserver::Wait() { > > > run_loop_.Run(); > > > } > > > > > > (RunLoop::Run() also has the nice property of returning if Quit() has > already > > > been called.) > > > > The finicky bits are when RunUntilIdle() doesn't actually run long enough (i.e., > until whatever you want to happen, has happened). This won't commonly happen on > the main thread, but if you have multiple threads in play (e.g. reading > something from the file system), the main thread could be idle before the file > system read is complete. There's also utilities for that > (content::RunAllBlockingPoolTasksUntilIdle or something like that), but even > that has subtleties. And, as we get farther out, none of those would account > for if what you're waiting for is dependent on another process (such as waiting > for an extension to act in the renderer and notify the browser). > > In most cases like this, what you really want to do is say "Run until [x] > happens", rather than "run the tasks you immediately know about in the message > loop" (though there are some exceptions). > > So, that's the finicky bit. But I forgot about the StopCheck() case, which is > indeed a pain. There are ways around it... but none pretty. > > I'm okay with this for now, for lack of better alternatives for StopCheck(), but > it's worth noting (perhaps adding a comment?) that if these checks ever get > complex, a single RunUntilIdle() call may be insufficient - possibly leading to > flakiness. Meh again. How about using two functions, WaitForCompletion and WaitForIdle? The downside is that the normal tests will hang for a long time if the callback doesn't get called, but I'm OK with that since the test would be failing anyway at that point. https://codereview.chromium.org/2693373003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/blacklist_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/blacklist_check_unittest.cc:41: PreloadCheckRunner runner_; On 2017/03/16 01:42:44, Devlin wrote: > nit: it'd be my preference to expose this through an accessor and keep this > private. I don't see value in that. Unlike, say, browser_thread_bundle_ (which is only used by the base class) or blacklist_ (which is set up and "owned" by the base class), the runner_ object exists solely for tests (derived classes) to play with. In fact, to be consistent, I'll move extension_ into the protected block as well :-P (The Google C++ guide allows for protected data members in Gtest tests.) https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... File extensions/browser/policy_check.cc (right): https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... extensions/browser/policy_check.cc:13: PolicyCheck::PolicyCheck(content::BrowserContext* context, On 2017/03/16 01:42:44, Devlin wrote: > nit: Rather than pass in the context here, maybe pass in the management policy > directly? Saves a bit of mocking in the test, and keeps the class a bit more > narrow. I'll keep this in mind in case it becomes an issue later. But at a certain point we might as well just inline the check (which is what I'm trying to avoid, because it gets untenable as the number of one-off checks grows). https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... File extensions/browser/policy_check_unittest.cc (right): https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:29: const base::string16 kDummyPolicyError = On 2017/03/16 01:42:44, Devlin wrote: > can't have static non-POD. Just go ahead and inline this in UserMayLoad. Inlined the string conversions. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pol... extensions/browser/policy_check_unittest.cc:110: // Test an invalid extension. On 2017/03/16 01:42:44, Devlin wrote: > s/invalid/disallowed (and analogous for valid on line 96) Done. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... File extensions/browser/preload_check.h (right): https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... extensions/browser/preload_check.h:8: #include <memory> On 2017/03/16 01:42:44, Devlin wrote: > Needed? Done. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... extensions/browser/preload_check.h:10: #include <utility> On 2017/03/16 01:42:44, Devlin wrote: > needed? Done. https://codereview.chromium.org/2693373003/diff/180001/extensions/browser/pre... extensions/browser/preload_check.h:16: #include "extensions/common/extension.h" On 2017/03/16 01:42:44, Devlin wrote: > nit: forward declaration instead of this include Done.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2693373003/#ps260001 (title: "rebase")
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2693373003/#ps280001 (title: "don't break enable_extensions=false")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by michaelpg@chromium.org
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": 280001, "attempt_start_ts": 1489801407481860, "parent_rev": "60defc8c87eeebe0cdc7e9a219abd6aea1651c41", "commit_rev": "c0145e6a749a559b1c50ecf2176f9ef0fce68c09"}
Message was sent while issue was closed.
Description was changed from ========== Add PreloadCheck class for extension pre-install checks PreloadCheck is an abstract preload check for an extension that may be asynchronous. The idea is to de-duplicate ad-hoc checks into PreloadCheck subclasses, then use these classes from places like ExtensionInstallChecker, CrxInstaller, and ManagementAPI. This will enable some of these classes to be moved into //extensions, because we would be able to pick and choose which checks to run. E.g., AppShell would only use checks from //extensions, while Chrome would also add checks from //chrome like BlacklistCheck. See go/loading-extensions-in-appshell for more details on how this relates to AppShell and launching apps/extensions. BUG=679971 ========== to ========== Add PreloadCheck class for extension pre-install checks PreloadCheck is an abstract preload check for an extension that may be asynchronous. The idea is to de-duplicate ad-hoc checks into PreloadCheck subclasses, then use these classes from places like ExtensionInstallChecker, CrxInstaller, and ManagementAPI. This will enable some of these classes to be moved into //extensions, because we would be able to pick and choose which checks to run. E.g., AppShell would only use checks from //extensions, while Chrome would also add checks from //chrome like BlacklistCheck. See go/loading-extensions-in-appshell for more details on how this relates to AppShell and launching apps/extensions. BUG=679971 Review-Url: https://codereview.chromium.org/2693373003 Cr-Commit-Position: refs/heads/master@{#457943} Committed: https://chromium.googlesource.com/chromium/src/+/c0145e6a749a559b1c50ecf2176f... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/c0145e6a749a559b1c50ecf2176f... |