Cleanup: Remove legacy accessors from URLRequestContext.

Cleanup: Remove legacy accessors from URLRequestContext.

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

[pauljensen, 2013-12-02 15:24:41]

Sorry for the slow response, I was on vacation all last week.

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
File content/browser/speech/google_one_shot_remote_engine.cc (left):

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
content/browser/speech/google_one_shot_remote_engine.cc:176: // accept language
through the URLRequestContext.
This comment should not be removed as the TODO has not been accomplished and is
still pertinent.

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
File content/browser/speech/google_streaming_remote_engine.cc (left):

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
content/browser/speech/google_streaming_remote_engine.cc:565: // accept language
through the URLRequestContext.
This comment should not be removed as the TODO has not been accomplished and is
still pertinent.

https://codereview.chromium.org/83433008/diff/1/sync/internal_api/http_bridge.cc
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/83433008/diff/1/sync/internal_api/http_bridge...
sync/internal_api/http_bridge.cc:160: accepted_language_list, user_agent));
I think these two arguments should be on seperate lines.

[Lei Zhang, 2013-12-11 03:32:24]

No rush. Slow here too. Finally got around to addressing your comments.

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
File content/browser/speech/google_one_shot_remote_engine.cc (left):

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
content/browser/speech/google_one_shot_remote_engine.cc:176: // accept language
through the URLRequestContext.
On 2013/12/02 15:24:42, pauljensen wrote:
> This comment should not be removed as the TODO has not been accomplished and
is
> still pertinent.

Done.

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
File content/browser/speech/google_streaming_remote_engine.cc (left):

https://codereview.chromium.org/83433008/diff/1/content/browser/speech/google...
content/browser/speech/google_streaming_remote_engine.cc:565: // accept language
through the URLRequestContext.
On 2013/12/02 15:24:42, pauljensen wrote:
> This comment should not be removed as the TODO has not been accomplished and
is
> still pertinent.

Done.

https://codereview.chromium.org/83433008/diff/1/sync/internal_api/http_bridge.cc
File sync/internal_api/http_bridge.cc (right):

https://codereview.chromium.org/83433008/diff/1/sync/internal_api/http_bridge...
sync/internal_api/http_bridge.cc:160: accepted_language_list, user_agent));
On 2013/12/02 15:24:42, pauljensen wrote:
> I think these two arguments should be on seperate lines.

Done.

[pauljensen, 2013-12-11 16:07:47]

lgtm.  Removing these legacy accessors is a nice API reduction/cleanup and is
overall a slight code size reduction.  I'm not super happy about the fact that
we're just inlining the accessor code in several places but I think it's for the
greater good.  As the TODO's allude to, the next step is to remove the accesses
to URLRequestContext::http_user_agent_settings() by instead constructing classes
like GoogleOneShotRemoteEngine and GoogleStreamingRemoteEngine with pointers to
the HttpUserAgentSettings.

[Lei Zhang, 2013-12-11 19:51:18]

OWNERS:
xians: content/browser/speech
cbentzel: net
tim: sync

[cbentzel, 2013-12-12 21:42:00]

net LGTM rubberstamp based on pauljensen's review.

paul, I'll add you to net/OWNERS as well.

[Lei Zhang, 2013-12-16 22:15:40]

OWNERS ping.

xians: content/browser/speech
tim: sync

[no longer working on chromium, 2013-12-30 12:35:15]

Hey, just got back from my vacation today.

lgtm with some minor comments, please address them.

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_one_shot_remote_engine.cc:175: //
TODO(pauljensen): GoogleOneShotRemoteEngine should be constructed with
remove this TODO if your CL has addressed it.

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_one_shot_remote_engine.cc:184: lang_param =
accepted_language_list.substr(0, separator);
move these two lines into the if case, and also move the declaration of
accepted_language_list there

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_one_shot_remote_engine.cc:184: lang_param =
accepted_language_list.substr(0, separator);
how about checking if (separator != std::string::npos) before doing the
substring

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
File content/browser/speech/google_streaming_remote_engine.cc (right):

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_streaming_remote_engine.cc:574: langs =
accepted_language_list.substr(0, separator);
ditto.

[Lei Zhang, 2014-01-08 22:44:25]

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_one_shot_remote_engine.cc:175: //
TODO(pauljensen): GoogleOneShotRemoteEngine should be constructed with
On 2013/12/30 12:35:15, xians1 wrote:
> remove this TODO if your CL has addressed it.

nay

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_one_shot_remote_engine.cc:184: lang_param =
accepted_language_list.substr(0, separator);
On 2013/12/30 12:35:15, xians1 wrote:
> move these two lines into the if case, and also move the declaration of
> accepted_language_list there

Done.

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_one_shot_remote_engine.cc:184: lang_param =
accepted_language_list.substr(0, separator);
On 2013/12/30 12:35:15, xians1 wrote:
> how about checking if (separator != std::string::npos) before doing the
> substring

I see this deviates from google_streaming_remote_engine.cc, but since I'm just
refactoring, I'm inclined to just leave this alone rather than change the
behavior in some subtle way.

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
File content/browser/speech/google_streaming_remote_engine.cc (right):

https://codereview.chromium.org/83433008/diff/70001/content/browser/speech/go...
content/browser/speech/google_streaming_remote_engine.cc:574: langs =
accepted_language_list.substr(0, separator);
On 2013/12/30 12:35:15, xians1 wrote:
> ditto.

Done.

[ncarter (slow), 2014-01-08 22:49:29]

sync LGTM

[commit-bot: I haz the power, 2014-01-08 22:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/83433008/160001

[tim (not reviewing), 2014-01-08 23:22:06]

(sorry I missed this, thanks Nick!)

[commit-bot: I haz the power, 2014-01-09 00:12:17]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 02:10:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/83433008/160001

[commit-bot: I haz the power, 2014-01-09 04:02:43]

Change committed as 243750