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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 7322004: DevTools: devtools manager should know nothing about DevToolsWindow (Closed)

Created:
9 years, 5 months ago by yurys
Modified:
9 years, 5 months ago
Reviewers:
Jói, pfeldman
CC:
chromium-reviews, Avi (use Gerrit), jam, Erik does not do reviews, Paweł Hajdan Jr., kkania, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

DevTools: devtools manager should know nothing about DevToolsWindow All code specific to DevToolsWindow moved into that class. There is no DevToolsClientHost::AsDevToolsWindow method anymore. To open devtools window clients should invoke corresponding static methods on DevToolsWindow which is the only way to create default devtools window. BUG=None TEST=Existing DevTools tests. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91695

Patch Set 1 #

Total comments: 5

Patch Set 2 : fixed win compilation #

Patch Set 3 : Rebase #

Total comments: 14

Patch Set 4 : Removed strange #include #

Patch Set 5 : Comments addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+173 lines, -248 lines) Patch
M chrome/browser/automation/testing_automation_provider.cc View 1 2 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/extensions/extensions_ui.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/external_tab_container_win.cc View 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/printing/print_dialog_cloud.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/tab_contents/render_view_context_menu.cc View 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/gtk/extensions/extension_popup_gtk.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_popup.cc View 1 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/options/extension_settings_handler.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M content/browser/debugger/devtools_client_host.h View 2 chunks +0 lines, -3 lines 0 comments Download
M content/browser/debugger/devtools_client_host.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M content/browser/debugger/devtools_handler.cc View 1 2 3 chunks +22 lines, -6 lines 0 comments Download
M content/browser/debugger/devtools_manager.h View 1 2 3 chunks +2 lines, -27 lines 0 comments Download
M content/browser/debugger/devtools_manager.cc View 1 2 3 10 chunks +4 lines, -156 lines 0 comments Download
M content/browser/debugger/devtools_sanity_unittest.cc View 1 2 3 4 1 chunk +1 line, -6 lines 0 comments Download
M content/browser/debugger/devtools_window.h View 1 2 3 4 5 chunks +16 lines, -2 lines 0 comments Download
M content/browser/debugger/devtools_window.cc View 1 2 3 4 6 chunks +105 lines, -15 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
yurys
9 years, 5 months ago (2011-07-07 08:50:53 UTC) #1
pfeldman
http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_handler.cc File content/browser/debugger/devtools_handler.cc (right): http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_handler.cc#newcode41 content/browser/debugger/devtools_handler.cc:41: DevToolsWindow* window = DevToolsWindow::FindDevToolsWindow( This does not seem right. ...
9 years, 5 months ago (2011-07-07 09:05:06 UTC) #2
yurys
http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_handler.cc File content/browser/debugger/devtools_handler.cc (right): http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_handler.cc#newcode41 content/browser/debugger/devtools_handler.cc:41: DevToolsWindow* window = DevToolsWindow::FindDevToolsWindow( On 2011/07/07 09:05:06, pfeldman wrote: ...
9 years, 5 months ago (2011-07-07 09:41:00 UTC) #3
pfeldman
http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_manager.h File content/browser/debugger/devtools_manager.h (right): http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_manager.h#newcode61 content/browser/debugger/devtools_manager.h:61: void CloseClient(DevToolsClientHost* client_host); How does this correlate with NotifyCloseListener? ...
9 years, 5 months ago (2011-07-07 12:17:51 UTC) #4
yurys
http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_manager.h File content/browser/debugger/devtools_manager.h (right): http://codereview.chromium.org/7322004/diff/1/content/browser/debugger/devtools_manager.h#newcode61 content/browser/debugger/devtools_manager.h:61: void CloseClient(DevToolsClientHost* client_host); On 2011/07/07 12:17:52, pfeldman wrote: > ...
9 years, 5 months ago (2011-07-07 12:30:39 UTC) #5
pfeldman
LGTM
9 years, 5 months ago (2011-07-07 12:33:08 UTC) #6
Jói
9 years, 5 months ago (2011-07-07 17:10:03 UTC) #7 
LGTM

Thanks for doing this Yury!

Powered by Google App Engine
This is Rietveld 408576698