Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(102)

Issue 12319114: Extract debugger target enumeration into a separate class (Closed)

Created:
7 years, 10 months ago by Vladislav Kaznacheev
Modified:
7 years, 9 months ago
Reviewers:
jam, Matt Perry, pfeldman
CC:
chromium-reviews, vsevik, yurys, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@debugger
Visibility:
Public.

Description

This is required to simplify the implementation of chrome.debugger.getTargets. BUG=179342 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187540

Patch Set 1 #

Patch Set 2 : Cleaner version ready for review #

Total comments: 22

Patch Set 3 : Addressed comments, split the patch in two #

Total comments: 18

Patch Set 4 : Addressed comments #

Total comments: 8

Patch Set 5 : Fixed thumbnail url generation, added a test #

Total comments: 4

Patch Set 6 : Moved the test out of extensions/api_test #

Patch Set 7 : Renamed getters, simplified thumbnail generation. #

Patch Set 8 : Fixed compile #

Total comments: 10

Patch Set 9 : Addressed comments #

Patch Set 10 : Addressed comments #

Total comments: 7

Patch Set 11 : Addressed comments #

Total comments: 1

Patch Set 12 : Addressed comments, fixed favicon test #

Patch Set 13 : Fixed compile #

Patch Set 14 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+273 lines, -154 lines) Patch
M chrome/browser/devtools/devtools_sanity_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +74 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_service.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/extension_service.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
A chrome/test/data/devtools/target_list/background.js View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +62 lines, -0 lines 0 comments Download
A + chrome/test/data/devtools/target_list/manifest.json View 1 2 3 4 5 1 chunk +2 lines, -3 lines 0 comments Download
A chrome/test/data/devtools/target_list/test_page.html View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/devtools/devtools_agent_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M content/browser/devtools/devtools_agent_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +6 lines, -1 line 0 comments Download
M content/browser/devtools/devtools_http_handler_impl.h View 1 2 3 4 5 6 7 8 9 3 chunks +2 lines, -12 lines 0 comments Download
M content/browser/devtools/devtools_http_handler_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 11 chunks +64 lines, -120 lines 0 comments Download
M content/browser/devtools/render_view_devtools_agent_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +39 lines, -7 lines 0 comments Download
M content/public/browser/devtools_agent_host.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +12 lines, -5 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
Vladislav Kaznacheev
Please take a look. This is still work in progress.
7 years, 10 months ago (2013-02-26 15:23:16 UTC) #1
Vladislav Kaznacheev
Please review. I am not sure about the name for the new class (DevToolsTargetList), advise ...
7 years, 9 months ago (2013-02-28 12:38:49 UTC) #2
pfeldman
https://codereview.chromium.org/12319114/diff/3001/chrome/browser/extensions/api/debugger/debugger_api.cc File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/12319114/diff/3001/chrome/browser/extensions/api/debugger/debugger_api.cc#newcode216 chrome/browser/extensions/api/debugger/debugger_api.cc:216: dictionary->SetString("type", "extension"); So remote debugging won't know that it ...
7 years, 9 months ago (2013-03-01 08:49:30 UTC) #3
Vladislav Kaznacheev
Split the patch in two, the actual change to chrome.debugger API is to follow. https://chromiumcodereview.appspot.com/12319114/diff/3001/chrome/browser/extensions/api/debugger/debugger_api.cc ...
7 years, 9 months ago (2013-03-01 12:58:35 UTC) #4
pfeldman
https://chromiumcodereview.appspot.com/12319114/diff/10001/content/browser/devtools/devtools_agent_host_impl.cc File content/browser/devtools/devtools_agent_host_impl.cc (right): https://chromiumcodereview.appspot.com/12319114/diff/10001/content/browser/devtools/devtools_agent_host_impl.cc#newcode63 content/browser/devtools/devtools_agent_host_impl.cc:63: return !!DevToolsManager::GetInstance()->GetDevToolsClientHostFor(this); So this is a convenience method that ...
7 years, 9 months ago (2013-03-01 14:28:17 UTC) #5
Vladislav Kaznacheev
PTAL https://chromiumcodereview.appspot.com/12319114/diff/10001/content/browser/devtools/devtools_agent_host_impl.cc File content/browser/devtools/devtools_agent_host_impl.cc (right): https://chromiumcodereview.appspot.com/12319114/diff/10001/content/browser/devtools/devtools_agent_host_impl.cc#newcode63 content/browser/devtools/devtools_agent_host_impl.cc:63: return !!DevToolsManager::GetInstance()->GetDevToolsClientHostFor(this); On 2013/03/01 14:28:17, pfeldman wrote: > ...
7 years, 9 months ago (2013-03-01 16:16:37 UTC) #6
pfeldman
https://codereview.chromium.org/12319114/diff/9003/content/browser/devtools/devtools_agent_host_impl.cc File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/12319114/diff/9003/content/browser/devtools/devtools_agent_host_impl.cc#newcode17 content/browser/devtools/devtools_agent_host_impl.cc:17: static std::string GenerateId() { Why? https://codereview.chromium.org/12319114/diff/9003/content/browser/devtools/devtools_http_handler_impl.cc File content/browser/devtools/devtools_http_handler_impl.cc (right): ...
7 years, 9 months ago (2013-03-04 11:43:01 UTC) #7
pfeldman
https://codereview.chromium.org/12319114/diff/16001/chrome/test/data/extensions/api_test/debugger_remote/background.js File chrome/test/data/extensions/api_test/debugger_remote/background.js (right): https://codereview.chromium.org/12319114/diff/16001/chrome/test/data/extensions/api_test/debugger_remote/background.js#newcode1 chrome/test/data/extensions/api_test/debugger_remote/background.js:1: // Copyright (c) 2013 The Chromium Authors. All rights ...
7 years, 9 months ago (2013-03-04 11:48:42 UTC) #8
Vladislav Kaznacheev
https://codereview.chromium.org/12319114/diff/9003/content/browser/devtools/devtools_agent_host_impl.cc File content/browser/devtools/devtools_agent_host_impl.cc (right): https://codereview.chromium.org/12319114/diff/9003/content/browser/devtools/devtools_agent_host_impl.cc#newcode17 content/browser/devtools/devtools_agent_host_impl.cc:17: static std::string GenerateId() { Inlined back. On 2013/03/04 11:43:01, ...
7 years, 9 months ago (2013-03-04 14:30:02 UTC) #9
Vladislav Kaznacheev
PTAL
7 years, 9 months ago (2013-03-04 16:22:33 UTC) #10
pfeldman
https://codereview.chromium.org/12319114/diff/18001/content/browser/devtools/devtools_http_handler_impl.cc File content/browser/devtools/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/12319114/diff/18001/content/browser/devtools/devtools_http_handler_impl.cc#newcode478 content/browser/devtools/devtools_http_handler_impl.cc:478: target_list->Refresh(); This should be its private detail. https://codereview.chromium.org/12319114/diff/18001/content/browser/devtools/render_view_devtools_agent_host.h File ...
7 years, 9 months ago (2013-03-05 12:06:40 UTC) #11
Vladislav Kaznacheev
PTAL https://chromiumcodereview.appspot.com/12319114/diff/18001/content/browser/devtools/devtools_http_handler_impl.cc File content/browser/devtools/devtools_http_handler_impl.cc (right): https://chromiumcodereview.appspot.com/12319114/diff/18001/content/browser/devtools/devtools_http_handler_impl.cc#newcode478 content/browser/devtools/devtools_http_handler_impl.cc:478: target_list->Refresh(); Moved into begin() On 2013/03/05 12:06:41, pfeldman ...
7 years, 9 months ago (2013-03-05 13:35:44 UTC) #12
pfeldman
lgtm
7 years, 9 months ago (2013-03-05 15:25:40 UTC) #13
Vladislav Kaznacheev
Hello OWNERS, jam@, please review: content/public/browser/devtools_agent_host.h content/public/browser/devtools_target_list.h content/public/browser/devtools_target_list.cc mpcomplete@, please review: chrome/browser/extensions/extension_service.h chrome/browser/extensions/extension_service.cc Thanks! Vlad
7 years, 9 months ago (2013-03-05 15:46:04 UTC) #14
jam
I don't see why any changes to content are needed? chrome.debugger.getTargets is a chrome feature ...
7 years, 9 months ago (2013-03-05 17:56:22 UTC) #15
Vladislav Kaznacheev
I agree that all the additions to DevToolsAgentHost can be done from chrome. However the ...
7 years, 9 months ago (2013-03-05 19:13:06 UTC) #16
jam
On 2013/03/05 19:13:06, Vladislav Kaznacheev wrote: > I agree that all the additions to DevToolsAgentHost ...
7 years, 9 months ago (2013-03-05 20:40:51 UTC) #17
pfeldman
> However it seems that this is a pretty small class (note also we don't ...
7 years, 9 months ago (2013-03-05 20:53:47 UTC) #18
Matt Perry
extensions change LGTM
7 years, 9 months ago (2013-03-05 22:00:57 UTC) #19
Vladislav Kaznacheev
I have hidden the DevToolsTargetList class into the implementation part. Now the only change to ...
7 years, 9 months ago (2013-03-06 16:08:38 UTC) #20
jam
On 2013/03/05 20:53:47, pfeldman wrote: > > However it seems that this is a pretty ...
7 years, 9 months ago (2013-03-06 17:41:51 UTC) #21
Vladislav Kaznacheev
https://codereview.chromium.org/12319114/diff/35001/content/public/browser/devtools_agent_host.h File content/public/browser/devtools_agent_host.h (right): https://codereview.chromium.org/12319114/diff/35001/content/public/browser/devtools_agent_host.h#newcode42 content/public/browser/devtools_agent_host.h:42: static std::string DisconnectRenderViewHost(RenderViewHost* rvh); Several reasons: 1. It does ...
7 years, 9 months ago (2013-03-06 18:26:14 UTC) #22
jam
https://codereview.chromium.org/12319114/diff/35001/content/public/browser/devtools_agent_host.h File content/public/browser/devtools_agent_host.h (right): https://codereview.chromium.org/12319114/diff/35001/content/public/browser/devtools_agent_host.h#newcode42 content/public/browser/devtools_agent_host.h:42: static std::string DisconnectRenderViewHost(RenderViewHost* rvh); On 2013/03/06 18:26:15, Vladislav Kaznacheev ...
7 years, 9 months ago (2013-03-06 22:45:26 UTC) #23
Vladislav Kaznacheev
On 2013/03/06 22:45:26, jam wrote: > https://codereview.chromium.org/12319114/diff/35001/content/public/browser/devtools_agent_host.h > File content/public/browser/devtools_agent_host.h (right): > > https://codereview.chromium.org/12319114/diff/35001/content/public/browser/devtools_agent_host.h#newcode42 > ...
7 years, 9 months ago (2013-03-07 08:59:34 UTC) #24
jam
lgtm On 2013/03/07 08:59:34, Vladislav Kaznacheev wrote: > On 2013/03/06 22:45:26, jam wrote: > > ...
7 years, 9 months ago (2013-03-07 17:50:24 UTC) #25
Vladislav Kaznacheev
> ok, can we just say empty string instead of NullId, and get rid of ...
7 years, 9 months ago (2013-03-11 08:49:05 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12319114/59013
7 years, 9 months ago (2013-03-12 07:14:11 UTC) #27
Vladislav Kaznacheev
Committed patchset #14 manually as r187540 (presubmit successful).
7 years, 9 months ago (2013-03-12 08:48:06 UTC) #28
falken
7 years, 9 months ago (2013-03-12 11:03:43 UTC) #29
Message was sent while issue was closed.
On 2013/03/12 08:48:06, Vladislav Kaznacheev wrote:
> Committed patchset #14 manually as r187540 (presubmit successful).

Sorry to revert. This seems to cause browser_tests to fail on Chrome OS:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

Output:
chrome/browser/devtools/devtools_sanity_browsertest.cc:684: Failure
Value of: RunExtensionTest("target_list")
Actual: false
Expected: true

Powered by Google App Engine
This is Rietveld 408576698