[DevTools] Extended RemoteDebuggingTest.RemoteDebugger with additional json protocol tests.

[DevTools] Extended RemoteDebuggingTest.RemoteDebugger with additional json protocol tests.

BUG=313168
TEST=RemoteDebuggingTest.RemoteDebugger

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

[Dmitry Zvorygin, 2013-10-31 16:12:08]

Please take a look.

[Vladislav Kaznacheev, 2013-11-01 11:48:18]

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
File chrome/test/data/devtools/target_list/background.js (right):

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:5: function requestUrl(url,
callback) {
This method is always called with an url starting with "http://" +
REMOTE_DEBUGGER_HOST. Lets move it inside, it will make the call site cleaner.

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:47: function
waitForTab(pred, callback) {
'pred' name is cryptic. I needed to read the entire function to figure out that
this is probably a contraction of 'predicate'.

Consider changing it to something clearer. Perhaps 'match' or 'filter'.

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:81: var extension_target;
JS naming convension is extensionTarget

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:115:
chrome.test.assertEq(versionInfo["Browser"].indexOf("Chrome/"), 0);
assert(versionInfo["Browser"].match(/^Chrome\//)) is cleaner.
You can even match a four-part numeric version if you want.

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:122: function openNewPage()
{
Insert blank lines between the functions

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:122: function openNewPage()
{
We also need a test for json/new with no url which was the only option before
M32 and therefore widely used.

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:143: function(waitedTab) {
waitedTab is poor English

https://chromiumcodereview.appspot.com/54333004/diff/1/chrome/test/data/devto...
chrome/test/data/devtools/target_list/background.js:170: extension_tab = tab;
Lets save the id only.

[Dmitry Zvorygin, 2013-11-01 13:49:36]

Please take a look.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
File chrome/test/data/devtools/target_list/background.js (right):

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:5: function requestUrl(url,
callback) {
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> This method is always called with an url starting with "http://" +
> REMOTE_DEBUGGER_HOST. Lets move it inside, it will make the call site cleaner.

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:47: function
waitForTab(pred, callback) {
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> 'pred' name is cryptic. I needed to read the entire function to figure out
that
> this is probably a contraction of 'predicate'.
> 
> Consider changing it to something clearer. Perhaps 'match' or 'filter'.

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:81: var extension_target;
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> JS naming convension is extensionTarget

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:115:
chrome.test.assertEq(versionInfo["Browser"].indexOf("Chrome/"), 0);
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> assert(versionInfo["Browser"].match(/^Chrome\//)) is cleaner.
> You can even match a four-part numeric version if you want.

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:122: function openNewPage()
{
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> We also need a test for json/new with no url which was the only option before
> M32 and therefore widely used.

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:122: function openNewPage()
{
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> Insert blank lines between the functions

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:143: function(waitedTab) {
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> waitedTab is poor English

Done.

https://codereview.chromium.org/54333004/diff/1/chrome/test/data/devtools/tar...
chrome/test/data/devtools/target_list/background.js:170: extension_tab = tab;
On 2013/11/01 11:48:19, Vladislav Kaznacheev wrote:
> Lets save the id only.

Done.

[Vladislav Kaznacheev, 2013-11-01 14:00:52]

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
File chrome/test/data/devtools/target_list/background.js (right):

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:9: req.open('GET', 'http://'
+ REMOTE_DEBUGGER_HOST + path, true);
Lets move '/json/' in here as well

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:71: })
; after the statement

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:118: var extensionTarget;
you forgot to change this to extensionTargetId

[Dmitry Zvorygin, 2013-11-01 14:48:37]

Fixed minor comments.

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
File chrome/test/data/devtools/target_list/background.js (right):

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:9: req.open('GET', 'http://'
+ REMOTE_DEBUGGER_HOST + path, true);
On 2013/11/01 14:00:52, Vladislav Kaznacheev wrote:
> Lets move '/json/' in here as well

Decided to leave it as is.

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:71: })
On 2013/11/01 14:00:52, Vladislav Kaznacheev wrote:
> ; after the statement

Done.

https://codereview.chromium.org/54333004/diff/60001/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:118: var extensionTarget;
On 2013/11/01 14:00:52, Vladislav Kaznacheev wrote:
> you forgot to change this to extensionTargetId

Done.

[Vladislav Kaznacheev, 2013-11-01 14:56:06]

LGTM with one suggestion

https://codereview.chromium.org/54333004/diff/60002/chrome/test/data/devtools...
File chrome/test/data/devtools/target_list/background.js (right):

https://codereview.chromium.org/54333004/diff/60002/chrome/test/data/devtools...
chrome/test/data/devtools/target_list/background.js:161: function
newSpecificPage() {
Please move newSpecificPage and newDefaultPage above discoverTargets or below
closePage so that the logical link between discoverTarget/activatePage/closePage
is move obvious.

[commit-bot: I haz the power, 2013-11-01 15:58:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/54333004/150001

[commit-bot: I haz the power, 2013-11-01 16:07:08]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-11-02 07:28:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/54333004/150001

[commit-bot: I haz the power, 2013-11-02 09:17:57]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 05:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/54333004/450001

[commit-bot: I haz the power, 2013-11-05 07:42:57]

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

[commit-bot: I haz the power, 2013-11-05 08:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/54333004/450001

[commit-bot: I haz the power, 2013-11-05 10:42:12]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 11:02:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/54333004/450001

[commit-bot: I haz the power, 2013-11-05 14:10:13]

Change committed as 232973