Eliminate the dependency of Profile from TtsMessageFilter.

Eliminate the dependency of Profile from TtsMessageFilter.

Eliminating the dependency of Profile from TtsMessageFilter by
delegating GetProfile function to TtsEngineDelegate.
This is part of an effort to move TTS to content.

BUG=347045
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289245

[dmazzoni, 2014-08-11 07:23:09]

Thanks for your patience, I was out for a week.

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/extens...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/extens...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:243: return
Profile::FromBrowserContext(rph->GetBrowserContext());
Perhaps converting from BrowserContext to Profile should be done elsewhere? This
should just return a BrowserContext.

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/tts_co...
File chrome/browser/speech/tts_controller.h (right):

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller.h:105: // Return the profile associated
with the renderer
Nit: end sentence with period.

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller.h:106: virtual Profile* GetProfile(int
render_process_id) = 0;
If you want to move TtsController to content, then perhaps this should be
BrowserContext instead of Profile - may as well make that change now?

[mrunal, 2014-08-11 23:25:07]

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/extens...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/extens...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:243: return
Profile::FromBrowserContext(rph->GetBrowserContext());
On 2014/08/11 07:23:09, dmazzoni wrote:
> Perhaps converting from BrowserContext to Profile should be done elsewhere?
This
> should just return a BrowserContext.
What's wrong with keeping it here? I agree TtsExtensionEngine is not the best
place for it considering it's not related to Extensions. May be I can create a
ChromeTtsControllerDelegate(derived from TtsControllerDelegate) which can
implement this GetProfile method but then it will be lone-method class as I
don't see any more methods going there. Any suggestions, Where do you think this
conversion should happen?

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/tts_co...
File chrome/browser/speech/tts_controller_impl.cc (right):

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:119: : current_utterance_(0),
Looking at this, I might have mistakenly pushed this unnecessary change from
other branch. I will revert this.

[mrunal, 2014-08-12 02:58:38]

After some rethinking I have replaced the use of Profile altogether by
BrowserContext.
I don't know if that was what you were trying to suggest?

[dmazzoni, 2014-08-12 06:16:01]

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/extens...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/448033002/diff/1/chrome/browser/speech/extens...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:243: return
Profile::FromBrowserContext(rph->GetBrowserContext());
On 2014/08/11 23:25:07, mrunal wrote:
> On 2014/08/11 07:23:09, dmazzoni wrote:
> > Perhaps converting from BrowserContext to Profile should be done elsewhere?
> This
> > should just return a BrowserContext.
> What's wrong with keeping it here?

What's wrong is that this is an implementation of a virtual function defined in
tts_controller.h, which we want to move to content.

> I agree TtsExtensionEngine is not the best
> place for it considering it's not related to Extensions. May be I can create a
> ChromeTtsControllerDelegate(derived from TtsControllerDelegate) which can
> implement this GetProfile method but then it will be lone-method class as I
> don't see any more methods going there. Any suggestions, Where do you think
this
> conversion should happen? 

As an example, TtsExtensionEventHandler::OnTtsEvent currently takes the profile
from an utterance and uses that to fire an extension event. Instead, OnTtsEvent
could take the utterance's browser context and convert it to a profile just
before using it.

[dmazzoni, 2014-08-12 06:21:08]

lgtm

Great! This is what I had in mind. It looks to me like now tts_controller.[cc,h]
doesn't depend on chrome anymore, which is excellent.

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:803: host->AddFilter(new
TtsMessageFilter(id, host->GetBrowserContext()));
A profile is a browser context, so I don't think you need to change this, right?
Just pass |profile|.

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/ex...
File chrome/browser/speech/extension_api/tts_engine_extension_api.h (right):

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/ex...
chrome/browser/speech/extension_api/tts_engine_extension_api.h:48:
content::BrowserContext* browser_context) OVERRIDE;
nit: indentation should be 4 spaces from beginning of previous line

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/ex...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/ex...
chrome/browser/speech/extension_api/tts_extension_api.cc:131:
event->restrict_to_browser_context = profile;
You don't need to convert this one either - event->restrict_to_browser_context
can take the browser context directly.

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/tt...
File chrome/browser/speech/tts_controller.h (right):

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/tt...
chrome/browser/speech/tts_controller.h:107: content::BrowserContext*
browser_context) = 0;
nit: indentation

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/tt...
chrome/browser/speech/tts_controller.h:223: // The profile that initiated this
utterance.
Update the comment.

[mrunal, 2014-08-12 17:40:58]

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:803: host->AddFilter(new
TtsMessageFilter(id, host->GetBrowserContext()));
On 2014/08/12 06:21:08, dmazzoni wrote:
> A profile is a browser context, so I don't think you need to change this,
right?
> Just pass |profile|.

I agree but once I move TtsMessageFilter to content I plan to initialize it from
RenderProcessHostImpl::CreateMessageFilters() where you won't have Profile.
That's why I think we should keep BrowserContext here.

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/ex...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/448033002/diff/20001/chrome/browser/speech/ex...
chrome/browser/speech/extension_api/tts_extension_api.cc:131:
event->restrict_to_browser_context = profile;
On 2014/08/12 06:21:08, dmazzoni wrote:
> You don't need to convert this one either - event->restrict_to_browser_context
> can take the browser context directly.

I agreee.

[dmazzoni, 2014-08-12 18:26:43]

On Tue, Aug 12, 2014 at 10:40 AM, <mrunal.kapade@intel.com> wrote:

>
> https://codereview.chromium.org/448033002/diff/20001/
> chrome/browser/chrome_content_browser_client.cc
> File chrome/browser/chrome_content_browser_client.cc (right):
>
> https://codereview.chromium.org/448033002/diff/20001/
> chrome/browser/chrome_content_browser_client.cc#newcode803
> chrome/browser/chrome_content_browser_client.cc:803: host->AddFilter(new
> TtsMessageFilter(id, host->GetBrowserContext()));
> On 2014/08/12 06:21:08, dmazzoni wrote:
>
>> A profile is a browser context, so I don't think you need to change
>>
> this, right?
>
>> Just pass |profile|.
>>
>
> I agree but once I move TtsMessageFilter to content I plan to initialize
> it from RenderProcessHostImpl::CreateMessageFilters() where you won't
> have Profile. That's why I think we should keep BrowserContext here.


Makes sense.


>
>
> https://codereview.chromium.org/448033002/diff/20001/
> chrome/browser/speech/extension_api/tts_extension_api.cc
> File chrome/browser/speech/extension_api/tts_extension_api.cc (right):
>
> https://codereview.chromium.org/448033002/diff/20001/
> chrome/browser/speech/extension_api/tts_extension_api.cc#newcode131
> chrome/browser/speech/extension_api/tts_extension_api.cc:131:
> event->restrict_to_browser_context = profile;
> On 2014/08/12 06:21:08, dmazzoni wrote:
>
>> You don't need to convert this one either -
>>
> event->restrict_to_browser_context
>
>> can take the browser context directly.
>>
>
> I agreee.
>
> https://codereview.chromium.org/448033002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mrunal, 2014-08-12 18:55:17]

https://codereview.chromium.org/448033002/diff/40001/chrome/browser/speech/ex...
File chrome/browser/speech/extension_api/tts_engine_extension_api.h (right):

https://codereview.chromium.org/448033002/diff/40001/chrome/browser/speech/ex...
chrome/browser/speech/extension_api/tts_engine_extension_api.h:42:
std::vector<VoiceData>* out_voices) OVERRIDE;
Also fixed the indentation here.

https://codereview.chromium.org/448033002/diff/40001/chrome/browser/speech/tt...
File chrome/browser/speech/tts_controller.h (right):

https://codereview.chromium.org/448033002/diff/40001/chrome/browser/speech/tt...
chrome/browser/speech/tts_controller.h:310: const std::string& error_message) =
0;
Also fixed the indentation here.

[mrunal, 2014-08-12 21:40:21]

The CQ bit was checked by mrunal.kapade@intel.com

[commit-bot: I haz the power, 2014-08-12 21:45:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/448033002/40001

[commit-bot: I haz the power, 2014-08-13 01:13:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 01:30:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:30:08]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mrunal, 2014-08-13 01:46:43]

On 2014/08/13 01:30:08, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Uploaded the patch tp fix the compilation error on ChromeOS now. Still didn't
quite understand the reason behind the failure of win_chromium_x64_rel.

[mrunal, 2014-08-13 06:03:32]

The CQ bit was checked by mrunal.kapade@intel.com

[commit-bot: I haz the power, 2014-08-13 06:04:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/448033002/60001

[commit-bot: I haz the power, 2014-08-13 10:04:32]

Change committed as 289245