Refactoring: Create TranslateScript from TranslateManager

Refactoring: Create TranslateScript from TranslateManager

This CL separates the function to fetch the Translate script from TranslateManager.

TEST=browser_tests, unit_tests

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

[hajimehoshi, 2013-06-20 04:46:16]

Can you take a look?

[hajimehoshi, 2013-06-20 05:48:45]

As we discussed offline, I'm waiting for Toyoshima-san's CL
(https://codereview.chromium.org/15949022/) is comitted.

[Takashi Toyoshima, 2013-06-20 06:59:53]

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_script.cc (right):

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:25: const char
kTranslateScriptURL[] =
We can simplify these const names.
ScriptURL?

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:46: if (fetcher_.get() != NULL)
NOTREACHED, and it's better to return false as TranslateURLFetcher did.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:81: fetcher_.reset(new
TranslateURLFetcher(0));
Define it as an anonymous const int and use it here and DCHECK below.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:90: return fetcher_ != NULL;
.get() != NULL

Also, implementing it in the header looks a standard way for chromium to me.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_script.h (right):

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:25: const std::string&
translate_script() { return translate_script_; }
just data(), script(), or something looks better.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:35: void ClearTranslateScript() {
translate_script_.clear(); }
[optional] just Clear() looks fine for now.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:41: // Returns true if this is now
fetching the script.
HasPendingRequest() looks better because this function is a pair with Request().

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:45:
DISALLOW_COPY_AND_ASSIGN(TranslateScript);
from style rule,
"the DISALLOW_COPY_AND_ASSIGN macro invocation should be at the end of the
private: section"
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:56: std::string translate_script_;
script_ or data_

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_url_fetcher.h (right):

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_url_fetcher.h:29: int retry_count() {
retry_count* used in function and variable names should be renamed to declare
it's for 5xx.
I introduced another retry, and need to avoid confusion.

[hajimehoshi, 2013-06-20 08:50:08]

Almost done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_script.cc (right):

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:25: const char
kTranslateScriptURL[] =
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> We can simplify these const names.
> ScriptURL?

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:46: if (fetcher_.get() != NULL)
I expect returning boolean will be implemented an commited at your another CL.

On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> NOTREACHED, and it's better to return false as TranslateURLFetcher did.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:81: fetcher_.reset(new
TranslateURLFetcher(0));
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> Define it as an anonymous const int and use it here and DCHECK below.

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.cc:90: return fetcher_ != NULL;
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> .get() != NULL
> 
> Also, implementing it in the header looks a standard way for chromium to me.

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_script.h (right):

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:25: const std::string&
translate_script() { return translate_script_; }
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> just data(), script(), or something looks better.

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:35: void ClearTranslateScript() {
translate_script_.clear(); }
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> [optional] just Clear() looks fine for now.

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:41: // Returns true if this is now
fetching the script.
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> HasPendingRequest() looks better because this function is a pair with
Request().

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:45:
DISALLOW_COPY_AND_ASSIGN(TranslateScript);
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> from style rule,
> "the DISALLOW_COPY_AND_ASSIGN macro invocation should be at the end of the
> private: section"
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_script.h:56: std::string translate_script_;
On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> script_ or data_

Done.

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_url_fetcher.h (right):

https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_url_fetcher.h:29: int retry_count() {
OK, I'll do it later.

On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> retry_count* used in function and variable names should be renamed to declare
> it's for 5xx.
> I introduced another retry, and need to avoid confusion.

[hajimehoshi, 2013-06-21 04:24:52]

ping toyoshim@, I rebased this CL to match your committed CL.  Can you take a
look?

On 2013/06/20 08:50:08, hajimehoshi wrote:
> Almost done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> File chrome/browser/translate/translate_script.cc (right):
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.cc:25: const char
> kTranslateScriptURL[] =
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > We can simplify these const names.
> > ScriptURL?
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.cc:46: if (fetcher_.get() != NULL)
> I expect returning boolean will be implemented an commited at your another CL.
> 
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > NOTREACHED, and it's better to return false as TranslateURLFetcher did.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.cc:81: fetcher_.reset(new
> TranslateURLFetcher(0));
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > Define it as an anonymous const int and use it here and DCHECK below.
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.cc:90: return fetcher_ != NULL;
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > .get() != NULL
> > 
> > Also, implementing it in the header looks a standard way for chromium to me.
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> File chrome/browser/translate/translate_script.h (right):
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.h:25: const std::string&
> translate_script() { return translate_script_; }
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > just data(), script(), or something looks better.
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.h:35: void ClearTranslateScript() {
> translate_script_.clear(); }
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > [optional] just Clear() looks fine for now.
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.h:41: // Returns true if this is now
> fetching the script.
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > HasPendingRequest() looks better because this function is a pair with
> Request().
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.h:45:
> DISALLOW_COPY_AND_ASSIGN(TranslateScript);
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > from style rule,
> > "the DISALLOW_COPY_AND_ASSIGN macro invocation should be at the end of the
> > private: section"
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_script.h:56: std::string translate_script_;
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > script_ or data_
> 
> Done.
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> File chrome/browser/translate/translate_url_fetcher.h (right):
> 
>
https://codereview.chromium.org/17390018/diff/10001/chrome/browser/translate/...
> chrome/browser/translate/translate_url_fetcher.h:29: int retry_count() {
> OK, I'll do it later.
> 
> On 2013/06/20 06:59:53, Takashi Toyoshima (chromium) wrote:
> > retry_count* used in function and variable names should be renamed to
declare
> > it's for 5xx.
> > I introduced another retry, and need to avoid confusion.

[Takashi Toyoshima, 2013-06-21 05:38:55]

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:544: 
Should relocate DCHECK at line 562 to here?

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:562: DCHECK(script_ != NULL);
script_.get()

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:618: if (!script_) {
script_.get() == NULL

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:698:
ProcessPendingRequests(!success);
How about moving ProcessPendingRequests() implementation here.
There is no reason to factor it out.

[hajimehoshi, 2013-06-21 07:01:44]

Thanks!

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:544: 
On 2013/06/21 05:38:55, Takashi Toyoshima (chromium) wrote:
> Should relocate DCHECK at line 562 to here?

Done.

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:562: DCHECK(script_ != NULL);
On 2013/06/21 05:38:55, Takashi Toyoshima (chromium) wrote:
> script_.get()

Done.

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:618: if (!script_) {
On 2013/06/21 05:38:55, Takashi Toyoshima (chromium) wrote:
> script_.get() == NULL

Done.

https://codereview.chromium.org/17390018/diff/19001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:698:
ProcessPendingRequests(!success);
On 2013/06/21 05:38:55, Takashi Toyoshima (chromium) wrote:
> How about moving ProcessPendingRequests() implementation here.
> There is no reason to factor it out.

Done.

[Takashi Toyoshima, 2013-06-21 07:06:12]

lgtm

[commit-bot: I haz the power, 2013-06-21 07:16:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/17390018/29001

[commit-bot: I haz the power, 2013-06-21 16:11:13]

Change committed as 207848