Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11779002:
Make specch input extension api a PKS. (Closed)
|
Created:
7 years, 11 months ago by SanjoyPal Modified:
7 years, 11 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionMake specch input extension api a PKS.
BUG=159265
TEST=SpeechInputExtensionApiTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177105
Patch Set 1 #Patch Set 2 : Make specch input extension api a PKS. #Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 19 (0 generated)
Please review.
On 2013/01/04 19:37:29, SanjoyPal wrote: > Please review. profiles/* LGTM.
+tommi for speech There is already a SpeechInputExtensionManager::Factory. Can you use that instead? At least, I think you should be able to change it to use ProfileKeyedAPIFactory, in any case. It's already using ServiceIsCreatedWithProfile = true, but because it's not declared in ProfileDependencyManager, I don't think that it actually behaves that way.
Make specch input extension api a PKS. BUG=159265 TEST=SpeechInputExtensionApiTest.*
Moved speech input function registration to SpeechInputExtensionManager. PTAL. Thanks
lgtm
Converted SpeechInputExtensionManager::Factory to ProfileKeyedAPIFactory. Please review again. Thanks. On 2013/01/04 20:13:24, Yoyo Zhou wrote: > +tommi for speech > > There is already a SpeechInputExtensionManager::Factory. Can you use that > instead? At least, I think you should be able to change it to use > ProfileKeyedAPIFactory, in any case. > > It's already using ServiceIsCreatedWithProfile = true, but because it's not > declared in ProfileDependencyManager, I don't think that it actually behaves > that way.
lgtm with a couple of requests. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:695: manager_ = new SpeechInputExtensionManager(profile); initializer list? https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:716: SpeechInputAPI::GetFactoryInstance() { I don't think you need to wrap here
Updated the patch. Please review. On 2013/01/15 16:59:41, tommi wrote: > lgtm with a couple of requests. > > https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... > File chrome/browser/speech/speech_input_extension_manager.cc (right): > > https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... > chrome/browser/speech/speech_input_extension_manager.cc:695: manager_ = new > SpeechInputExtensionManager(profile); > initializer list? > > https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... > chrome/browser/speech/speech_input_extension_manager.cc:716: > SpeechInputAPI::GetFactoryInstance() { > I don't think you need to wrap here
Updated the patch. Please review. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:695: manager_ = new SpeechInputExtensionManager(profile); On 2013/01/15 16:59:42, tommi wrote: > initializer list? Done. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:716: SpeechInputAPI::GetFactoryInstance() { Do you mean, I need to implement ProfileKeyedAPI stuff inside SpeechInputExtensionManager, instead of wrapping it with SpeechInputAPI? But, it leads to same error as before "ref_counted.cc(67)] Check failed: in_dtor_. RefCountedThreadSafe object deleted without calling Release()" On 2013/01/15 16:59:42, tommi wrote: > I don't think you need to wrap here
https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:716: SpeechInputAPI::GetFactoryInstance() { On 2013/01/16 01:15:41, SanjoyPal wrote: > Do you mean, I need to implement ProfileKeyedAPI stuff inside > SpeechInputExtensionManager, instead of wrapping it with SpeechInputAPI? But, it > leads to same error as before "ref_counted.cc(67)] Check failed: in_dtor_. > RefCountedThreadSafe object deleted without calling Release()" > > On 2013/01/15 16:59:42, tommi wrote: > > I don't think you need to wrap here > I think he's referring to line wrapping. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.h (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.h:235: static const bool kServiceRedirectedInIncognito = false; false is the default. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.h:238: virtual void Shutdown() OVERRIDE { I would prefer if this was not inlined in the header file. https://codereview.chromium.org/11779002/diff/19001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/19001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:89: extensions::SpeechInputAPI* speech_input = Call this speech_input_api
Done. Please review. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:716: SpeechInputAPI::GetFactoryInstance() { On 2013/01/16 01:37:19, Yoyo Zhou wrote: > On 2013/01/16 01:15:41, SanjoyPal wrote: > > Do you mean, I need to implement ProfileKeyedAPI stuff inside > > SpeechInputExtensionManager, instead of wrapping it with SpeechInputAPI? But, > it > > leads to same error as before "ref_counted.cc(67)] Check failed: in_dtor_. > > RefCountedThreadSafe object deleted without calling Release()" > > > > On 2013/01/15 16:59:42, tommi wrote: > > > I don't think you need to wrap here > > > > I think he's referring to line wrapping. Done. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.h (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.h:235: static const bool kServiceRedirectedInIncognito = false; On 2013/01/16 01:37:19, Yoyo Zhou wrote: > false is the default. Done. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.h:238: virtual void Shutdown() OVERRIDE { On 2013/01/16 01:37:19, Yoyo Zhou wrote: > I would prefer if this was not inlined in the header file. Done. https://codereview.chromium.org/11779002/diff/19001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/19001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:89: extensions::SpeechInputAPI* speech_input = On 2013/01/16 01:37:19, Yoyo Zhou wrote: > Call this speech_input_api Done.
https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:716: SpeechInputAPI::GetFactoryInstance() { On 2013/01/16 01:52:23, SanjoyPal wrote: > On 2013/01/16 01:37:19, Yoyo Zhou wrote: > > On 2013/01/16 01:15:41, SanjoyPal wrote: > > > Do you mean, I need to implement ProfileKeyedAPI stuff inside > > > SpeechInputExtensionManager, instead of wrapping it with SpeechInputAPI? > But, > > it > > > leads to same error as before "ref_counted.cc(67)] Check failed: in_dtor_. > > > RefCountedThreadSafe object deleted without calling Release()" > > > > > > On 2013/01/15 16:59:42, tommi wrote: > > > > I don't think you need to wrap here > > > > > > > I think he's referring to line wrapping. > > Done. It's still wrapped. https://codereview.chromium.org/11779002/diff/24001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/24001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:91: extensions::SpeechInputAPI>::GetForProfile(profile); Line breaking is awkward: break after the last ::
Updated. https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/13001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:716: SpeechInputAPI::GetFactoryInstance() { On 2013/01/16 02:07:46, Yoyo Zhou wrote: > On 2013/01/16 01:52:23, SanjoyPal wrote: > > On 2013/01/16 01:37:19, Yoyo Zhou wrote: > > > On 2013/01/16 01:15:41, SanjoyPal wrote: > > > > Do you mean, I need to implement ProfileKeyedAPI stuff inside > > > > SpeechInputExtensionManager, instead of wrapping it with SpeechInputAPI? > > But, > > > it > > > > leads to same error as before "ref_counted.cc(67)] Check failed: in_dtor_. > > > > RefCountedThreadSafe object deleted without calling Release()" > > > > > > > > On 2013/01/15 16:59:42, tommi wrote: > > > > > I don't think you need to wrap here > > > > > > > > > > I think he's referring to line wrapping. > > > > Done. > > It's still wrapped. Done. https://codereview.chromium.org/11779002/diff/24001/chrome/browser/speech/spe... File chrome/browser/speech/speech_input_extension_manager.cc (right): https://codereview.chromium.org/11779002/diff/24001/chrome/browser/speech/spe... chrome/browser/speech/speech_input_extension_manager.cc:91: extensions::SpeechInputAPI>::GetForProfile(profile); On 2013/01/16 02:07:46, Yoyo Zhou wrote: > Line breaking is awkward: break after the last :: Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/11779002/24002
Retried try job too often on win for step(s) compile
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/11779002/24002
Message was sent while issue was closed.
Change committed as 177105 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
