WebUI: Move test_browser_proxy.js one level up.

WebUI: Move test_browser_proxy.js one level up.

TestBrowserProxy class is already being used outside of MD Settings
(md_user_manager, bluetooth_internals, print_preview).

BUG=None

Review-Url: https://codereview.chromium.org/2927653002
Cr-Commit-Position: refs/heads/master@{#477692}
Committed: https://chromium.googlesource.com/chromium/src/+/d7fb46ad1354f84752e490f21898202fb16df377

[dpapad, 2017-06-06 22:10:48]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 22:11:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-06 22:11:41]

Description was changed from

==========
WebUI: Move test_browser_proxy.js one level up.

TestBrowserProxy class is already being used outside of MD Settings.

BUG=None
==========

to

==========
WebUI: Move test_browser_proxy.js one level up.

TestBrowserProxy class is already being used outside of MD Settings
(md_user_manager, bluetooth_internals, print_preview).

BUG=None
==========

[dpapad, 2017-06-06 22:34:09]

dpapad@chromium.org changed reviewers:
+ rbpotter@chromium.org

[commit-bot: I haz the power, 2017-06-06 23:45:01]

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

[commit-bot: I haz the power, 2017-06-06 23:45:02]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-06-07 00:32:01]

lgtm

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/blue...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/blue...
chrome/test/data/webui/bluetooth_internals_browsertest.js:60: * @extends
{TestBrowserProxyBase}
Should probably be test.TestBrowserProxy. TestBrowserProxyBase is not
defined anywhere as far as I can tell (same thing elsewhere in this
file).

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/fake_language_settings_private.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/fake_language_settings_private.js:25: * @extends
{TestBrowserProxy}
Minor/optional: This is different from most other instances in
settings/, which have settings.TestBrowserProxy in the old version
and test.TestBrowserProxy in the new version. If this isn't
intentional maybe this should be changed to be consistent.

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/privacy_page_test.js:8: * @extends
{TestBrowserProxy}
Same comment as above.

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/system_page_tests.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/system_page_tests.js:10: * @extends
{TestBrowserProxy}
Same here.

[dpapad, 2017-06-07 00:42:54]

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/blue...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/blue...
chrome/test/data/webui/bluetooth_internals_browsertest.js:60: * @extends
{TestBrowserProxyBase}
On 2017/06/07 at 00:32:01, rbpotter wrote:
> Should probably be test.TestBrowserProxy. TestBrowserProxyBase is not
> defined anywhere as far as I can tell (same thing elsewhere in this
> file).

Done.

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/fake_language_settings_private.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/fake_language_settings_private.js:25: * @extends
{TestBrowserProxy}
On 2017/06/07 at 00:32:01, rbpotter wrote:
> Minor/optional: This is different from most other instances in
> settings/, which have settings.TestBrowserProxy in the old version
> and test.TestBrowserProxy in the new version. If this isn't
> intentional maybe this should be changed to be consistent.

Done. I don't think this was intentional. I run a regex over all the files to
replace settings.TestBrowserProxy to test.TestBrowserProxy, so this file (and a
few others) were missed.

Since we don't compile our tests, those annotations unfortunately can be
incorrect, without the compiler complaining.

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/privacy_page_test.js:8: * @extends
{TestBrowserProxy}
On 2017/06/07 at 00:32:01, rbpotter wrote:
> Same comment as above.

Done.

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/system_page_tests.js (right):

https://codereview.chromium.org/2927653002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/system_page_tests.js:10: * @extends
{TestBrowserProxy}
On 2017/06/07 at 00:32:01, rbpotter wrote:
> Same here.

Done.

[dpapad, 2017-06-07 00:44:55]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-06-07 00:44:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rbpotter@chromium.org
Link to the patchset: https://codereview.chromium.org/2927653002/#ps20001
(title: "Nits.")

[commit-bot: I haz the power, 2017-06-07 00:45:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-06-07 01:16:56]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-06-07 01:16:57]

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/test_browser_proxy.js (right):

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/test_browser_proxy.js:5: cr.define('test', function() {
why test.Test?

[dpapad, 2017-06-07 01:19:11]

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/test_browser_proxy.js (right):

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/test_browser_proxy.js:5: cr.define('test', function() {
On 2017/06/07 at 01:16:57, Dan Beam wrote:
> why test.Test?

As opposed to what? A different namespace? No namespace? I think having a
namespace is better than adding yet another global. 'test' seemed appropriate.

[Dan Beam, 2017-06-07 01:33:51]

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/test_browser_proxy.js (right):

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/test_browser_proxy.js:5: cr.define('test', function() {
On 2017/06/07 01:19:10, dpapad wrote:
> On 2017/06/07 at 01:16:57, Dan Beam wrote:
> > why test.Test?
> 
> As opposed to what? A different namespace? No namespace? I think having a
> namespace is better than adding yet another global. 'test' seemed appropriate.

arguably, this is "testing" code.  as in: it's not a test; you're not testing
the browser proxy.  you're using it in a supporting role, which I often see
"testing" used for.

note the subtle distinction in gtest:
https://cs.chromium.org/chromium/src/third_party/googletest/src/googletest/in...

it's in a "testing" namespace, but the base class is a "test".

i get that it's a "TestBrowserProxy" instead of a "BrowserProxyTest", but just
general nit.

additionally, I just also have an issue with redundant prefixes.  I don't think
having 2 x "test" in the identifier adds any information, it just makes things
longer.

I (personally) would just remove the cr.define() wrapper.

this means:
a) no redundant "test" or "testing" namespace (and no need to decide which one)
b) no dependency on cr.js (which otherwise this file doesn't use, it does rely
on promise_resolver.js, though)

[dpapad, 2017-06-07 01:48:58]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2017-06-07 03:23:08]

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/test_browser_proxy.js (right):

https://codereview.chromium.org/2927653002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/test_browser_proxy.js:5: cr.define('test', function() {
> I (personally) would just remove the cr.define() wrapper.
> 
> this means:
> a) no redundant "test" or "testing" namespace (and no need to decide which
one)
> b) no dependency on cr.js (which otherwise this file doesn't use, it does rely
on promise_resolver.js, though)

Done.

[dpapad, 2017-06-07 03:24:09]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 03:24:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 04:56:09]

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

[commit-bot: I haz the power, 2017-06-07 04:56:10]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-06-07 06:06:24]

rs lgtm

[dpapad, 2017-06-07 17:15:02]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-06-07 17:15:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
rbpotter@chromium.org
Link to the patchset: https://codereview.chromium.org/2927653002/#ps40001
(title: "Remove namespace")

[commit-bot: I haz the power, 2017-06-07 17:15:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 17:22:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496855702023650,
"parent_rev": "6ee44a7247c639c0703f291d320bdf05c1531b57", "commit_rev":
"d7fb46ad1354f84752e490f21898202fb16df377"}

[commit-bot: I haz the power, 2017-06-07 17:22:54]

Description was changed from

==========
WebUI: Move test_browser_proxy.js one level up.

TestBrowserProxy class is already being used outside of MD Settings
(md_user_manager, bluetooth_internals, print_preview).

BUG=None
==========

to

==========
WebUI: Move test_browser_proxy.js one level up.

TestBrowserProxy class is already being used outside of MD Settings
(md_user_manager, bluetooth_internals, print_preview).

BUG=None

Review-Url: https://codereview.chromium.org/2927653002
Cr-Commit-Position: refs/heads/master@{#477692}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7fb46ad1354f84752e490f21898...
==========

[commit-bot: I haz the power, 2017-06-07 17:22:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d7fb46ad1354f84752e490f21898...