Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11412281:
Moved dev tools initialization bit earlier for ChromeOS browser start. (Closed)
|
Created:
8 years ago by zel Modified:
8 years ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMoved dev tools initialization bit earlier for ChromeOS browser start.
We need to launch DevTools before WebUI for login screen is up and running so we can attach remotely do run automation tasks there.
BUG=162730
TEST=make sure you can connect to devtools when ChromeOS is camping on the login/OOBE screen
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171052
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/11412281/diff/3001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:179: if (base::StringToInt64(port_str, &port) && port > 0 && port < 65535) { Use StringToInt (avoids cast on 188). https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:332: remove newline. https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1042: Profile* profile, const CommandLine& command_line) { wrap command_line to new line. https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1043: if (command_line.HasSwitch(switches::kRemoteDebuggingPort)) { Refactor this so you don't duplicate it. https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.h:151: const FilePath cur_dir_; remove white space.
https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1043: if (command_line.HasSwitch(switches::kRemoteDebuggingPort)) { On 2012/12/04 00:10:53, sky wrote: > Refactor this so you don't duplicate it. Yeah, that's what I wanted to ping you about - where would be a good place to move this to?
https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1043: if (command_line.HasSwitch(switches::kRemoteDebuggingPort)) { On 2012/12/04 00:38:47, zel wrote: > On 2012/12/04 00:10:53, sky wrote: > > Refactor this so you don't duplicate it. > > Yeah, that's what I wanted to ping you about - where would be a good place to > move this to? If there is a directory for devtools stuff I would stick it there. Otherwise maybe a function in the chrome namespace in the file chrome/browser/launch_devtools.
https://codereview.chromium.org/11412281/diff/3001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:179: if (base::StringToInt64(port_str, &port) && port > 0 && port < 65535) { On 2012/12/04 00:10:53, sky wrote: > Use StringToInt (avoids cast on 188). Done. https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:332: On 2012/12/04 00:10:53, sky wrote: > remove newline. Done. https://codereview.chromium.org/11412281/diff/3001/chrome/browser/ui/startup/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1043: if (command_line.HasSwitch(switches::kRemoteDebuggingPort)) { On 2012/12/04 01:22:45, sky wrote: > On 2012/12/04 00:38:47, zel wrote: > > On 2012/12/04 00:10:53, sky wrote: > > > Refactor this so you don't duplicate it. > > > > Yeah, that's what I wanted to ping you about - where would be a good place to > > move this to? > > If there is a directory for devtools stuff I would stick it there. Otherwise > maybe a function in the chrome namespace in the file > chrome/browser/launch_devtools. Moved to chrome_browser_main.cc... invoked from ChromeBrowserMainParts::PostProfileInit()
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zelidrag@chromium.org/11412281/11001
Message was sent while issue was closed.
Change committed as 171052
Message was sent while issue was closed.
+Steven https://codereview.chromium.org/11412281/diff/11001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412281/diff/11001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:888: LaunchDevToolsHandlerIfNeeded(profile(), parsed_command_line()); This part isn't reached on ChromeOS build. #0 chromeos::ChromeBrowserMainPartsChromeos::PostProfileInit (this=0x7fffe17abb80) at ../../chrome/browser/chromeos/chrome_browser_main_chromeos.cc:484 #1 0x00007ffff6b26ced in ChromeBrowserMainParts::PreMainMessageLoopRunImpl (this=0x7fffe17abb80) at ../../chrome/browser/chrome_browser_main.cc:1158 then #0 ChromeBrowserMainPartsLinux::PostProfileInit (this=0x7fffe17abb80) at ../../chrome/browser/chrome_browser_main_linux.cc:126 #1 0x00007ffff6095141 in chromeos::ChromeBrowserMainPartsChromeos::PostProfileInit (this=0x7fffe17abb80) at ../../chrome/browser/chromeos/chrome_browser_main_chromeos.cc:551 #2 0x00007ffff6b26ced in ChromeBrowserMainParts::PreMainMessageLoopRunImpl (this=0x7fffe17abb80) at ../../chrome/browser/chrome_browser_main.cc:1158 and ChromeBrowserMainPartsLinux::PostProfileInit() doesn't call base class (ChromeBrowserMainPartsPosix) implementation. void ChromeBrowserMainPartsLinux::PostProfileInit() { media_transfer_protocol_device_observer_.reset( new chrome::MediaTransferProtocolDeviceObserverLinux()); }
Message was sent while issue was closed.
+Lei Introduced here http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_browser... On 2012/12/05 14:06:04, Nikita Kostylev wrote: > +Steven > > https://codereview.chromium.org/11412281/diff/11001/chrome/browser/chrome_bro... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/11412281/diff/11001/chrome/browser/chrome_bro... > chrome/browser/chrome_browser_main.cc:888: > LaunchDevToolsHandlerIfNeeded(profile(), parsed_command_line()); > This part isn't reached on ChromeOS build. > > #0 chromeos::ChromeBrowserMainPartsChromeos::PostProfileInit > (this=0x7fffe17abb80) > at ../../chrome/browser/chromeos/chrome_browser_main_chromeos.cc:484 > #1 0x00007ffff6b26ced in ChromeBrowserMainParts::PreMainMessageLoopRunImpl > (this=0x7fffe17abb80) > at ../../chrome/browser/chrome_browser_main.cc:1158 > > then > > #0 ChromeBrowserMainPartsLinux::PostProfileInit (this=0x7fffe17abb80) > at ../../chrome/browser/chrome_browser_main_linux.cc:126 > #1 0x00007ffff6095141 in > chromeos::ChromeBrowserMainPartsChromeos::PostProfileInit (this=0x7fffe17abb80) > at ../../chrome/browser/chromeos/chrome_browser_main_chromeos.cc:551 > #2 0x00007ffff6b26ced in ChromeBrowserMainParts::PreMainMessageLoopRunImpl > (this=0x7fffe17abb80) > at ../../chrome/browser/chrome_browser_main.cc:1158 > > and ChromeBrowserMainPartsLinux::PostProfileInit() doesn't call base class > (ChromeBrowserMainPartsPosix) implementation. > > void ChromeBrowserMainPartsLinux::PostProfileInit() { > media_transfer_protocol_device_observer_.reset( > new chrome::MediaTransferProtocolDeviceObserverLinux()); > }
Message was sent while issue was closed.
Fixed in https://codereview.chromium.org/11443009/
Message was sent while issue was closed.
https://codereview.chromium.org/11412281/diff/11001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412281/diff/11001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:888: LaunchDevToolsHandlerIfNeeded(profile(), parsed_command_line()); On 2012/12/05 14:06:04, Nikita Kostylev wrote: > This part isn't reached on ChromeOS build. > > #0 chromeos::ChromeBrowserMainPartsChromeos::PostProfileInit > (this=0x7fffe17abb80) > at ../../chrome/browser/chromeos/chrome_browser_main_chromeos.cc:484 > #1 0x00007ffff6b26ced in ChromeBrowserMainParts::PreMainMessageLoopRunImpl > (this=0x7fffe17abb80) > at ../../chrome/browser/chrome_browser_main.cc:1158 > > then > > #0 ChromeBrowserMainPartsLinux::PostProfileInit (this=0x7fffe17abb80) > at ../../chrome/browser/chrome_browser_main_linux.cc:126 > #1 0x00007ffff6095141 in > chromeos::ChromeBrowserMainPartsChromeos::PostProfileInit (this=0x7fffe17abb80) > at ../../chrome/browser/chromeos/chrome_browser_main_chromeos.cc:551 > #2 0x00007ffff6b26ced in ChromeBrowserMainParts::PreMainMessageLoopRunImpl > (this=0x7fffe17abb80) > at ../../chrome/browser/chrome_browser_main.cc:1158 > > and ChromeBrowserMainPartsLinux::PostProfileInit() doesn't call base class > (ChromeBrowserMainPartsPosix) implementation. > > void ChromeBrowserMainPartsLinux::PostProfileInit() { > media_transfer_protocol_device_observer_.reset( > new chrome::MediaTransferProtocolDeviceObserverLinux()); > } Yikes! Good thing that (until now) PostProfileInit() only had a ChromeBrowserMainPartsChromeos implementation. That's my bad, sorry about that! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
