Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23441032:
[SystemInfo API] Rewrite DisplayInfoProvider without SystemInfoProvider. (Closed)
|
Created:
7 years, 3 months ago by Haojian Wu Modified:
7 years, 2 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[SystemInfo API] Rewrite DisplayInfoProvider without SystemInfoProvider.
Since many UI operations are done on UI thread, there is no need to
create a SequenceWorkerPool and tasks for display API.
BUG=122863
TEST=browser_test --gtest_filter=SystemDisplayApiTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226102
Patch Set 1 #Patch Set 2 : Add missing head file #
Total comments: 10
Patch Set 3 : Address comments #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Rebase #
Total comments: 2
Patch Set 6 : Update. #Patch Set 7 : Address comments #
Total comments: 2
Patch Set 8 : Update #
Total comments: 2
Patch Set 9 : Update comments. #Patch Set 10 : Rebase #Patch Set 11 : Fix build error #Patch Set 12 : Update chromeos unittest code. #
Total comments: 3
Patch Set 13 : Update. #Patch Set 14 : Fix build error. #Patch Set 15 : Use valid display id. #
Total comments: 1
Patch Set 16 : Fix chromeos run error #Patch Set 17 : nits fix #Patch Set 18 : Set back screenAsh after test run #Patch Set 19 : set the original screen back #Patch Set 20 : Rebase #Messages
Total messages: 46 (0 generated)
Consider this CL please. Should we need to rewrite the DisplayInfoProvider? Thanks.
https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/system_display_api.h (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/system_display_api.h:14: class SystemDisplayGetInfoFunction : public AsyncExtensionFunction { Shouldn't this be UIThreadExtensionFunction? https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/system_display_apitest.cc (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/system_display_apitest.cc:128: unit->work_area.height = 720; Some of these must be already done in DisplayInfoProvider code?
https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/display_info_provider.cc:53: scoped_ptr<DisplayInfoProvider> g_display_info_provider; It is a common practice to use a base::LazyInstance to hold a global object and initialize it on demand. For this case, the global object might be designed to be leaky. https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/display_info_provider_chromeos.cc:361: return SetInfoImpl(display_id, info, &error); Right now SetInfoImpl has a signature "SetInfoImpl(const std::string&, const DisplayProperties&, std::string**), right? I suspect the usage of 'std::string**' usage in parameter. Could you double-check it?
https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/display_info_provider_chromeos.cc:361: return SetInfoImpl(display_id, info, &error); On 2013/09/04 06:58:02, Hongbo Min wrote: > Right now SetInfoImpl has a signature "SetInfoImpl(const std::string&, const > DisplayProperties&, std::string**), right? > > I suspect the usage of 'std::string**' usage in parameter. Could you > double-check it? Done. Passing |error| directly instead. https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/system_display_api.h (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/system_display_api.h:14: class SystemDisplayGetInfoFunction : public AsyncExtensionFunction { On 2013/09/03 23:14:46, oshima wrote: > Shouldn't this be UIThreadExtensionFunction? Refer to http://www.chromium.org/developers/design-documents/extensions/proposed-chang... An ExtensionFunction is implemented as subclasses of SyncExtensionFunction or AsyncExtensionFunction. UIThreadExtensionFunction is a upper level base class of SyncExtensionFunction/AsyncExtensionFunction. If you look into c/b/e/extension_function.h file, you will find ASyncExtensionFunction is just a subclass of UIThreadExtensionFunction without doing external work. And I don't find any ExtensionApiFunction inherit UIThreadExtensionFunction directly. So here I think it should be ASyncExtensionFunction. https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/system_display_apitest.cc (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/system_display_apitest.cc:128: unit->work_area.height = 720; On 2013/09/03 23:14:46, oshima wrote: > Some of these must be already done in DisplayInfoProvider code? Yes. Remove it now.
https://chromiumcodereview.appspot.com/23441032/diff/3001/chrome/browser/exte... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://chromiumcodereview.appspot.com/23441032/diff/3001/chrome/browser/exte... chrome/browser/extensions/api/system_display/display_info_provider.cc:53: scoped_ptr<DisplayInfoProvider> g_display_info_provider; On 2013/09/04 06:58:02, Hongbo Min wrote: > It is a common practice to use a base::LazyInstance to hold a global object and > initialize it on demand. For this case, the global object might be designed to > be leaky. Done.
https://codereview.chromium.org/23441032/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/23441032/diff/13001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.cc:56: static base::LazyInstance<scoped_ptr<DisplayInfoProvider> > Why not directly use DisplayInfoProvider instead of scoped_ptr?
https://chromiumcodereview.appspot.com/23441032/diff/13001/chrome/browser/ext... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://chromiumcodereview.appspot.com/23441032/diff/13001/chrome/browser/ext... chrome/browser/extensions/api/system_display/display_info_provider.cc:56: static base::LazyInstance<scoped_ptr<DisplayInfoProvider> > On 2013/09/05 08:43:36, Hongbo Min wrote: > Why not directly use DisplayInfoProvider instead of scoped_ptr? If uing DisplayInfoProvider directly, how to set a custom |display_info_provider| instance for Test in InitializeForTest()?
On 2013/09/05 08:52:22, Haojian Wu wrote: > https://chromiumcodereview.appspot.com/23441032/diff/13001/chrome/browser/ext... > File chrome/browser/extensions/api/system_display/display_info_provider.cc > (right): > > https://chromiumcodereview.appspot.com/23441032/diff/13001/chrome/browser/ext... > chrome/browser/extensions/api/system_display/display_info_provider.cc:56: static > base::LazyInstance<scoped_ptr<DisplayInfoProvider> > > On 2013/09/05 08:43:36, Hongbo Min wrote: > > Why not directly use DisplayInfoProvider instead of scoped_ptr? > > If uing DisplayInfoProvider directly, how to set a custom > |display_info_provider| instance for Test in InitializeForTest()? OK. LGTM.
https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/system_display_api.h (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/system_display_api.h:14: class SystemDisplayGetInfoFunction : public AsyncExtensionFunction { On 2013/09/04 12:11:33, Haojian Wu wrote: > On 2013/09/03 23:14:46, oshima wrote: > > Shouldn't this be UIThreadExtensionFunction? > > Refer to > http://www.chromium.org/developers/design-documents/extensions/proposed-chang... > > An ExtensionFunction is implemented as subclasses of SyncExtensionFunction or > AsyncExtensionFunction. > > UIThreadExtensionFunction is a upper level base class of > SyncExtensionFunction/AsyncExtensionFunction. If you look into > c/b/e/extension_function.h file, you will find ASyncExtensionFunction is just a > subclass of UIThreadExtensionFunction without doing external work. > > And I don't find any ExtensionApiFunction inherit UIThreadExtensionFunction > directly. > > So here I think it should be ASyncExtensionFunction. Shouldn't this be SyncExtensionFunction then? https://codereview.chromium.org/23441032/diff/22001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/23441032/diff/22001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.cc:57: g_display_info_provider = LAZY_INSTANCE_INITIALIZER; Given that you instantiate new object yourself, there is no need to use LazyInstance. Just use DisplayInfoProvider* display_info_provider = NULL; (g_ prefix is optional and up to you) in anonymous namespace in this file.
https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_display/system_display_api.h (right): https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/system_display/system_display_api.h:14: class SystemDisplayGetInfoFunction : public AsyncExtensionFunction { On 2013/09/06 18:30:48, oshima wrote: > On 2013/09/04 12:11:33, Haojian Wu wrote: > > On 2013/09/03 23:14:46, oshima wrote: > > > Shouldn't this be UIThreadExtensionFunction? > > > > Refer to > > > http://www.chromium.org/developers/design-documents/extensions/proposed-chang... > > > > An ExtensionFunction is implemented as subclasses of SyncExtensionFunction or > > AsyncExtensionFunction. > > > > UIThreadExtensionFunction is a upper level base class of > > SyncExtensionFunction/AsyncExtensionFunction. If you look into > > c/b/e/extension_function.h file, you will find ASyncExtensionFunction is just > a > > subclass of UIThreadExtensionFunction without doing external work. > > > > And I don't find any ExtensionApiFunction inherit UIThreadExtensionFunction > > directly. > > > > So here I think it should be ASyncExtensionFunction. > > Shouldn't this be SyncExtensionFunction then? Oh, Yeah. Should be SyncExtensionFunction. Done. https://codereview.chromium.org/23441032/diff/22001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/23441032/diff/22001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.cc:57: g_display_info_provider = LAZY_INSTANCE_INITIALIZER; On 2013/09/06 18:30:48, oshima wrote: > Given that you instantiate new object yourself, there is no need > to use LazyInstance. > > Just use > > DisplayInfoProvider* display_info_provider = NULL; (g_ prefix is optional and up > to you) > > in anonymous namespace in this file. One big concern: if we use the raw global pointer instead of C++ RAII technology, I think it's hard to safely maintain the pointer's lifttime(such as. when to delete it?) Any suggestion about it? Maybe we can put this pointer into DisplayInfoProvider class.
On 2013/09/07 03:16:53, Haojian Wu wrote: > https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... > File chrome/browser/extensions/api/system_display/system_display_api.h (right): > > https://codereview.chromium.org/23441032/diff/3001/chrome/browser/extensions/... > chrome/browser/extensions/api/system_display/system_display_api.h:14: class > SystemDisplayGetInfoFunction : public AsyncExtensionFunction { > On 2013/09/06 18:30:48, oshima wrote: > > On 2013/09/04 12:11:33, Haojian Wu wrote: > > > On 2013/09/03 23:14:46, oshima wrote: > > > > Shouldn't this be UIThreadExtensionFunction? > > > > > > Refer to > > > > > > http://www.chromium.org/developers/design-documents/extensions/proposed-chang... > > > > > > An ExtensionFunction is implemented as subclasses of SyncExtensionFunction > or > > > AsyncExtensionFunction. > > > > > > UIThreadExtensionFunction is a upper level base class of > > > SyncExtensionFunction/AsyncExtensionFunction. If you look into > > > c/b/e/extension_function.h file, you will find ASyncExtensionFunction is > just > > a > > > subclass of UIThreadExtensionFunction without doing external work. > > > > > > And I don't find any ExtensionApiFunction inherit UIThreadExtensionFunction > > > directly. > > > > > > So here I think it should be ASyncExtensionFunction. > > > > Shouldn't this be SyncExtensionFunction then? > > Oh, Yeah. Should be SyncExtensionFunction. Done. > > https://codereview.chromium.org/23441032/diff/22001/chrome/browser/extensions... > File chrome/browser/extensions/api/system_display/display_info_provider.cc > (right): > > https://codereview.chromium.org/23441032/diff/22001/chrome/browser/extensions... > chrome/browser/extensions/api/system_display/display_info_provider.cc:57: > g_display_info_provider = LAZY_INSTANCE_INITIALIZER; > On 2013/09/06 18:30:48, oshima wrote: > > Given that you instantiate new object yourself, there is no need > > to use LazyInstance. > > > > Just use > > > > DisplayInfoProvider* display_info_provider = NULL; (g_ prefix is optional and > up > > to you) > > > > in anonymous namespace in this file. > > One big concern: if we use the raw global pointer instead of C++ RAII > technology, I think it's hard to safely maintain the pointer's lifttime(such as. > when to delete it?) It doesn't hold any important states, so we can leave it without deleting it. This is not uncommon. > Any suggestion about it? Maybe we can put this pointer into DisplayInfoProvider > class.
On 2013/09/07 03:31:42, oshima wrote: > > > > One big concern: if we use the raw global pointer instead of C++ RAII > > technology, I think it's hard to safely maintain the pointer's lifttime(such > as. > > when to delete it?) > > It doesn't hold any important states, so we can leave it without deleting it. > This is not uncommon. Done. But I still wonder it will memory leak without deleting the pointer manually.
https://codereview.chromium.org/23441032/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/23441032/diff/34001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.cc:70: DCHECK(display_info_provider); Note. While this works for browser tests, which runs its own process, this doesn't work for unit tests where multiple tests are run within one process. You may want to comment in the header, or you can simply do "delete display_info_provider;". (I'm fine with either way)
On 2013/09/08 01:22:42, Haojian Wu wrote: > On 2013/09/07 03:31:42, oshima wrote: > > > > > > One big concern: if we use the raw global pointer instead of C++ RAII > > > technology, I think it's hard to safely maintain the pointer's lifttime(such > > as. > > > when to delete it?) > > > > It doesn't hold any important states, so we can leave it without deleting it. > > This is not uncommon. > > Done. But I still wonder it will memory leak without deleting the pointer > manually. global objects aren't considered as a leak.
https://codereview.chromium.org/23441032/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/23441032/diff/34001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.cc:70: DCHECK(display_info_provider); On 2013/09/10 21:49:52, oshima wrote: > Note. While this works for browser tests, which runs its own process, > this doesn't work for unit tests where multiple tests are run within > one process. You may want to comment in the header, or > you can simply do "delete display_info_provider;". (I'm fine with either way) Done. Added comments.
On 2013/09/10 21:50:57, oshima wrote: > > Done. But I still wonder it will memory leak without deleting the pointer > > manually. > > global objects aren't considered as a leak. It would be much appreciated of you if you could explain more details about this.
lgtm https://codereview.chromium.org/23441032/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.h (right): https://codereview.chromium.org/23441032/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.h:29: // Works for browser tests, which runs its own process, this doesn't work for This is for tests that run in its own process (e.g. browser_tests). Using this in other tests (e.g. unit_tests) will result in DCHECK failure.
https://codereview.chromium.org/23441032/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider.h (right): https://codereview.chromium.org/23441032/diff/39001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider.h:29: // Works for browser tests, which runs its own process, this doesn't work for On 2013/09/11 02:55:40, oshima wrote: > This is for tests that run in its own process (e.g. browser_tests). Using this > in other tests (e.g. unit_tests) will result in DCHECK failure. Done. Updated.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/48001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/69001
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=android_cl...
https://codereview.chromium.org/23441032/diff/47001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/23441032/diff/47001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc:5: #include "chrome/browser/extensions/api/system_display/display_info_provider.h" oshima@, please review this file. I forget to update unittest code on chromeos. Thanks.
lgtm with nit https://codereview.chromium.org/23441032/diff/47001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/23441032/diff/47001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc:34: *error = ""; error->clear();
https://codereview.chromium.org/23441032/diff/47001/chrome/browser/extensions... File chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/23441032/diff/47001/chrome/browser/extensions... chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc:34: *error = ""; On 2013/09/17 19:45:30, oshima wrote: > error->clear(); Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/90001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/109001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
https://chromiumcodereview.appspot.com/23441032/diff/47002/chrome/browser/ext... File chrome/browser/extensions/api/system_display/system_display_apitest.cc (right): https://chromiumcodereview.appspot.com/23441032/diff/47002/chrome/browser/ext... chrome/browser/extensions/api/system_display/system_display_apitest.cc:142: gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, screen_.get()); oshima@, Is this reset-screen usage available? I test on my local machine(linux build with flag: aura and chromeos). The browsertest will crash, the following are output log(run SystemDisplayApiTest.GetDisplay test): 0x7f2e8e40ceb0] base::debug::StackTrace::StackTrace() [0x7f2e8e40c7b7] base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x7f2e8125fcb0] <unknown> [0x00000244f8d4] std::vector<>::begin() [0x7f2e867d2497] aura::Window::GetChildById() [0x7f2e867d2468] aura::Window::GetChildById() [0x7f2e8cd68d48] ash::Shell::GetContainer() [0x7f2e8ce46525] ash::(anonymous namespace)::GetContainerById() [0x7f2e8ce467ff] ash::StackingController::GetDefaultParent() [0x7f2e867d1a9e] aura::Window::SetDefaultParentByRootWindow() [0x7f2e86ad02de] views::internal::NativeWidgetPrivate::ReparentNativeView() [0x7f2e86ad2f2d] views::Widget::ReparentNativeView() [0x7f2e86a3940e] views::NativeViewHostAura::NativeViewDetaching() [0x7f2e86a389b3] views::NativeViewHost::Detach() [0x7f2e86a3832f] views::NativeViewHost::Detach() [0x7f2e90f2137b] views::WebView::DetachWebContents() [0x7f2e90f20a79] views::WebView::SetWebContents() [0x0000029f7b6f] BrowserView::TabDetachedAt() [0x000002988c27] TabStripModel::DetachWebContentsAt() [0x00000298c52a] TabStripModel::Observe() [0x7f2e83e159f8] content::NotificationServiceImpl::Notify() [0x7f2e83ffd43e] content::WebContentsImpl::~WebContentsImpl() [0x7f2e83ffd768] content::WebContentsImpl::~WebContentsImpl() [0x00000298cec6] TabStripModel::InternalCloseTab() [0x00000298cd9a] TabStripModel::InternalCloseTabs() [0x00000298967e] TabStripModel::CloseAllTabs() [0x0000029121e2] Browser::OnWindowClosing() [0x0000029f8b8d] BrowserView::CanClose() [0x7f2e86ae3de5] views::NonClientView::CanClose() [0x7f2e86ad3d05] views::Widget::Close() [0x0000029f5c19] BrowserView::Close() [0x0000015d3424] BrowserCloseManager::CloseBrowsers() [0x0000015d2eb5] BrowserCloseManager::StartClosingBrowsers() [0x0000015d2124] chrome::CloseAllBrowsers() [0x000001e08ca7] BrowserProcessPlatformPartBase::AttemptExit() [0x0000015d2040] chrome::AttemptExitInternal() [0x0000015d26b3] chrome::AttemptExit() [0x000000922f87] base::internal::RunnableAdapter<>::Run() [0x00000092253d] base::internal::InvokeHelper<>::MakeItSo() [0x00000092183b] base::internal::Invoker<>::Run() [0x7f2e8e3fff49] base::Callback<>::Run() [0x7f2e8e4624dc] base::MessageLoop::RunTask() [0x7f2e8e462600] base::MessageLoop::DeferOrRunPendingTask() [0x7f2e8e462b25] base::MessageLoop::DoWork() [0x7f2e8e3e1f75] base::MessagePumpGlib::HandleDispatch() [0x7f2e8e3e1757] base::(anonymous namespace)::WorkSourceDispatch() [0x7f2e7f0e4d53] g_main_context_dispatch [0x7f2e7f0e50a0] <unknown> [0x7f2e7f0e5164] g_main_context_iteration [0x7f2e8e3e1b82] base::MessagePumpGlib::RunWithDispatcher() [0x7f2e8e3e1ffe] base::MessagePumpGlib::Run() [0x7f2e8e462069] base::MessageLoop::RunInternal() [0x7f2e8e461f20] base::MessageLoop::RunHandler() [0x7f2e8e4a338c] base::RunLoop::Run() [0x000001e7ecad] content::RunThisRunLoop() [0x000001e7ec3a] content::RunMessageLoop() [0x0000024396a7] InProcessBrowserTest::QuitBrowsers() [0x000002439540] InProcessBrowserTest::RunTestOnMainThreadLoop() [0x000003665037] content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() [0x00000366590a] base::internal::RunnableAdapter<>::Run() [0x000003665855] base::internal::InvokeHelper<>::MakeItSo() [0x00000366577b] base::internal::Invoker<>::Run() r8: 0000000000000030 r9: 0101010101010101 r10: 0000000000000000 r11: 00007f2e7fa69d36 r12: 0000000000000001 r13: 0000000000000001 r14: 0000000000000000 r15: 0000388d5ccf1890 di: 00000000000000e8 si: 0000000000000003 bp: 00007ffffa3bb020 bx: 0000000000000000 dx: 0000000000000003 ax: 00000000000000e8 cx: 0000000000000001 sp: 00007ffffa3bb000 ip: 000000000244f8d4 efl: 0000000000010206 cgf: 0000000000000033 erf: 0000000000000004 trp: 000000000000000e msk: 0000000000000000 cr2: 00000000000000e8 [0918/053445:ERROR:kill_posix.cc(190)] Unable to terminate process group 12188: No such process [1/1] SystemDisplayApiTest.GetDisplay (12131 ms) I can confirm the crash made by this statement(If we remove this statement, the crash will disappear.). Since I'm not familar with chromeos ui code. Could you please help to take a look this? Thanks.
On 2013/09/18 13:03:54, Haojian Wu wrote: > https://chromiumcodereview.appspot.com/23441032/diff/47002/chrome/browser/ext... > File chrome/browser/extensions/api/system_display/system_display_apitest.cc > (right): > > https://chromiumcodereview.appspot.com/23441032/diff/47002/chrome/browser/ext... > chrome/browser/extensions/api/system_display/system_display_apitest.cc:142: > gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, screen_.get()); > oshima@, Is this reset-screen usage available? > > I test on my local machine(linux build with flag: aura and chromeos). The > browsertest will crash, the following are output log(run > SystemDisplayApiTest.GetDisplay test): > > 0x7f2e8e40ceb0] base::debug::StackTrace::StackTrace() > [0x7f2e8e40c7b7] base::debug::(anonymous namespace)::StackDumpSignalHandler() > [0x7f2e8125fcb0] <unknown> > [0x00000244f8d4] std::vector<>::begin() > [0x7f2e867d2497] aura::Window::GetChildById() > [0x7f2e867d2468] aura::Window::GetChildById() > [0x7f2e8cd68d48] ash::Shell::GetContainer() > [0x7f2e8ce46525] ash::(anonymous namespace)::GetContainerById() > [0x7f2e8ce467ff] ash::StackingController::GetDefaultParent() > [0x7f2e867d1a9e] aura::Window::SetDefaultParentByRootWindow() > [0x7f2e86ad02de] views::internal::NativeWidgetPrivate::ReparentNativeView() > [0x7f2e86ad2f2d] views::Widget::ReparentNativeView() > [0x7f2e86a3940e] views::NativeViewHostAura::NativeViewDetaching() > [0x7f2e86a389b3] views::NativeViewHost::Detach() > [0x7f2e86a3832f] views::NativeViewHost::Detach() > [0x7f2e90f2137b] views::WebView::DetachWebContents() > [0x7f2e90f20a79] views::WebView::SetWebContents() > [0x0000029f7b6f] BrowserView::TabDetachedAt() > [0x000002988c27] TabStripModel::DetachWebContentsAt() > [0x00000298c52a] TabStripModel::Observe() > [0x7f2e83e159f8] content::NotificationServiceImpl::Notify() > [0x7f2e83ffd43e] content::WebContentsImpl::~WebContentsImpl() > [0x7f2e83ffd768] content::WebContentsImpl::~WebContentsImpl() > [0x00000298cec6] TabStripModel::InternalCloseTab() > [0x00000298cd9a] TabStripModel::InternalCloseTabs() > [0x00000298967e] TabStripModel::CloseAllTabs() > [0x0000029121e2] Browser::OnWindowClosing() > [0x0000029f8b8d] BrowserView::CanClose() > [0x7f2e86ae3de5] views::NonClientView::CanClose() > [0x7f2e86ad3d05] views::Widget::Close() > [0x0000029f5c19] BrowserView::Close() > [0x0000015d3424] BrowserCloseManager::CloseBrowsers() > [0x0000015d2eb5] BrowserCloseManager::StartClosingBrowsers() > [0x0000015d2124] chrome::CloseAllBrowsers() > [0x000001e08ca7] BrowserProcessPlatformPartBase::AttemptExit() > [0x0000015d2040] chrome::AttemptExitInternal() > [0x0000015d26b3] chrome::AttemptExit() > [0x000000922f87] base::internal::RunnableAdapter<>::Run() > [0x00000092253d] base::internal::InvokeHelper<>::MakeItSo() > [0x00000092183b] base::internal::Invoker<>::Run() > [0x7f2e8e3fff49] base::Callback<>::Run() > [0x7f2e8e4624dc] base::MessageLoop::RunTask() > [0x7f2e8e462600] base::MessageLoop::DeferOrRunPendingTask() > [0x7f2e8e462b25] base::MessageLoop::DoWork() > [0x7f2e8e3e1f75] base::MessagePumpGlib::HandleDispatch() > [0x7f2e8e3e1757] base::(anonymous namespace)::WorkSourceDispatch() > [0x7f2e7f0e4d53] g_main_context_dispatch > [0x7f2e7f0e50a0] <unknown> > [0x7f2e7f0e5164] g_main_context_iteration > [0x7f2e8e3e1b82] base::MessagePumpGlib::RunWithDispatcher() > [0x7f2e8e3e1ffe] base::MessagePumpGlib::Run() > [0x7f2e8e462069] base::MessageLoop::RunInternal() > [0x7f2e8e461f20] base::MessageLoop::RunHandler() > [0x7f2e8e4a338c] base::RunLoop::Run() > [0x000001e7ecad] content::RunThisRunLoop() > [0x000001e7ec3a] content::RunMessageLoop() > [0x0000024396a7] InProcessBrowserTest::QuitBrowsers() > [0x000002439540] InProcessBrowserTest::RunTestOnMainThreadLoop() > [0x000003665037] content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() > [0x00000366590a] base::internal::RunnableAdapter<>::Run() > [0x000003665855] base::internal::InvokeHelper<>::MakeItSo() > [0x00000366577b] base::internal::Invoker<>::Run() > r8: 0000000000000030 r9: 0101010101010101 r10: 0000000000000000 r11: > 00007f2e7fa69d36 > r12: 0000000000000001 r13: 0000000000000001 r14: 0000000000000000 r15: > 0000388d5ccf1890 > di: 00000000000000e8 si: 0000000000000003 bp: 00007ffffa3bb020 bx: > 0000000000000000 > dx: 0000000000000003 ax: 00000000000000e8 cx: 0000000000000001 sp: > 00007ffffa3bb000 > ip: 000000000244f8d4 efl: 0000000000010206 cgf: 0000000000000033 erf: > 0000000000000004 > trp: 000000000000000e msk: 0000000000000000 cr2: 00000000000000e8 > [0918/053445:ERROR:kill_posix.cc(190)] Unable to terminate process group 12188: > No such process > [1/1] SystemDisplayApiTest.GetDisplay (12131 ms) > > I can confirm the crash made by this statement(If we remove this statement, the > crash will disappear.). Since I'm not familar with chromeos ui code. Could you > please help to take a look this? > > Thanks. Screen object must outlive other UI. You can just create an leave it. Use ANNOTATE_LEAKING_OBJECT so that memory bot won't complain. See Shell::Shell() in ash/shell.cc
On 2013/09/18 16:40:11, oshima wrote: > On 2013/09/18 13:03:54, Haojian Wu wrote: > > > https://chromiumcodereview.appspot.com/23441032/diff/47002/chrome/browser/ext... > > File chrome/browser/extensions/api/system_display/system_display_apitest.cc > > (right): > > > > > https://chromiumcodereview.appspot.com/23441032/diff/47002/chrome/browser/ext... > > chrome/browser/extensions/api/system_display/system_display_apitest.cc:142: > > gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, screen_.get()); > > oshima@, Is this reset-screen usage available? > > > > I test on my local machine(linux build with flag: aura and chromeos). The > > browsertest will crash, the following are output log(run > > SystemDisplayApiTest.GetDisplay test): > > > > 0x7f2e8e40ceb0] base::debug::StackTrace::StackTrace() > > [0x7f2e8e40c7b7] base::debug::(anonymous namespace)::StackDumpSignalHandler() > > [0x7f2e8125fcb0] <unknown> > > [0x00000244f8d4] std::vector<>::begin() > > [0x7f2e867d2497] aura::Window::GetChildById() > > [0x7f2e867d2468] aura::Window::GetChildById() > > [0x7f2e8cd68d48] ash::Shell::GetContainer() > > [0x7f2e8ce46525] ash::(anonymous namespace)::GetContainerById() > > [0x7f2e8ce467ff] ash::StackingController::GetDefaultParent() > > [0x7f2e867d1a9e] aura::Window::SetDefaultParentByRootWindow() > > [0x7f2e86ad02de] views::internal::NativeWidgetPrivate::ReparentNativeView() > > [0x7f2e86ad2f2d] views::Widget::ReparentNativeView() > > [0x7f2e86a3940e] views::NativeViewHostAura::NativeViewDetaching() > > [0x7f2e86a389b3] views::NativeViewHost::Detach() > > [0x7f2e86a3832f] views::NativeViewHost::Detach() > > [0x7f2e90f2137b] views::WebView::DetachWebContents() > > [0x7f2e90f20a79] views::WebView::SetWebContents() > > [0x0000029f7b6f] BrowserView::TabDetachedAt() > > [0x000002988c27] TabStripModel::DetachWebContentsAt() > > [0x00000298c52a] TabStripModel::Observe() > > [0x7f2e83e159f8] content::NotificationServiceImpl::Notify() > > [0x7f2e83ffd43e] content::WebContentsImpl::~WebContentsImpl() > > [0x7f2e83ffd768] content::WebContentsImpl::~WebContentsImpl() > > [0x00000298cec6] TabStripModel::InternalCloseTab() > > [0x00000298cd9a] TabStripModel::InternalCloseTabs() > > [0x00000298967e] TabStripModel::CloseAllTabs() > > [0x0000029121e2] Browser::OnWindowClosing() > > [0x0000029f8b8d] BrowserView::CanClose() > > [0x7f2e86ae3de5] views::NonClientView::CanClose() > > [0x7f2e86ad3d05] views::Widget::Close() > > [0x0000029f5c19] BrowserView::Close() > > [0x0000015d3424] BrowserCloseManager::CloseBrowsers() > > [0x0000015d2eb5] BrowserCloseManager::StartClosingBrowsers() > > [0x0000015d2124] chrome::CloseAllBrowsers() > > [0x000001e08ca7] BrowserProcessPlatformPartBase::AttemptExit() > > [0x0000015d2040] chrome::AttemptExitInternal() > > [0x0000015d26b3] chrome::AttemptExit() > > [0x000000922f87] base::internal::RunnableAdapter<>::Run() > > [0x00000092253d] base::internal::InvokeHelper<>::MakeItSo() > > [0x00000092183b] base::internal::Invoker<>::Run() > > [0x7f2e8e3fff49] base::Callback<>::Run() > > [0x7f2e8e4624dc] base::MessageLoop::RunTask() > > [0x7f2e8e462600] base::MessageLoop::DeferOrRunPendingTask() > > [0x7f2e8e462b25] base::MessageLoop::DoWork() > > [0x7f2e8e3e1f75] base::MessagePumpGlib::HandleDispatch() > > [0x7f2e8e3e1757] base::(anonymous namespace)::WorkSourceDispatch() > > [0x7f2e7f0e4d53] g_main_context_dispatch > > [0x7f2e7f0e50a0] <unknown> > > [0x7f2e7f0e5164] g_main_context_iteration > > [0x7f2e8e3e1b82] base::MessagePumpGlib::RunWithDispatcher() > > [0x7f2e8e3e1ffe] base::MessagePumpGlib::Run() > > [0x7f2e8e462069] base::MessageLoop::RunInternal() > > [0x7f2e8e461f20] base::MessageLoop::RunHandler() > > [0x7f2e8e4a338c] base::RunLoop::Run() > > [0x000001e7ecad] content::RunThisRunLoop() > > [0x000001e7ec3a] content::RunMessageLoop() > > [0x0000024396a7] InProcessBrowserTest::QuitBrowsers() > > [0x000002439540] InProcessBrowserTest::RunTestOnMainThreadLoop() > > [0x000003665037] content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() > > [0x00000366590a] base::internal::RunnableAdapter<>::Run() > > [0x000003665855] base::internal::InvokeHelper<>::MakeItSo() > > [0x00000366577b] base::internal::Invoker<>::Run() > > r8: 0000000000000030 r9: 0101010101010101 r10: 0000000000000000 r11: > > 00007f2e7fa69d36 > > r12: 0000000000000001 r13: 0000000000000001 r14: 0000000000000000 r15: > > 0000388d5ccf1890 > > di: 00000000000000e8 si: 0000000000000003 bp: 00007ffffa3bb020 bx: > > 0000000000000000 > > dx: 0000000000000003 ax: 00000000000000e8 cx: 0000000000000001 sp: > > 00007ffffa3bb000 > > ip: 000000000244f8d4 efl: 0000000000010206 cgf: 0000000000000033 erf: > > 0000000000000004 > > trp: 000000000000000e msk: 0000000000000000 cr2: 00000000000000e8 > > [0918/053445:ERROR:kill_posix.cc(190)] Unable to terminate process group > 12188: > > No such process > > [1/1] SystemDisplayApiTest.GetDisplay (12131 ms) > > > > I can confirm the crash made by this statement(If we remove this statement, > the > > crash will disappear.). Since I'm not familar with chromeos ui code. Could you > > please help to take a look this? > > > > Thanks. > > Screen object must outlive other UI code. You can just create an leave it. > Use ANNOTATE_LEAKING_OBJECT so that memory bot won't complain. > > See Shell::Shell() in ash/shell.cc
On 2013/09/18 16:40:11, oshima wrote: > Screen object must outlive other UI. You can just create an leave it. > Use ANNOTATE_LEAKING_OBJECT so that memory bot won't complain. > > See Shell::Shell() in ash/shell.cc oshima@, I looked the code more throughly. From what I understand, this may not cause by Screen object lifetime. On chromeos, it will use ScreenAsh object in ash/screen_ash.h while on other platforms using ui/gfx/screen_XX.h ScreenAsh class has defined some methods which ScreenWin/ScreenGtk/ScreenIos objects don't. Maybe browser_test crash casues by this. Now I have updated this patch with a potential solution(inherit ScreenAsh class on chrome os), and pass test on my local machine.
On 2013/09/24 00:52:54, Haojian Wu wrote: > On 2013/09/18 16:40:11, oshima wrote: > > Screen object must outlive other UI. You can just create an leave it. > > Use ANNOTATE_LEAKING_OBJECT so that memory bot won't complain. > > > > See Shell::Shell() in ash/shell.cc > > oshima@, I looked the code more throughly. From what I understand, this may not > cause by Screen object lifetime. > > On chromeos, it will use ScreenAsh object in ash/screen_ash.h while on other > platforms using ui/gfx/screen_XX.h > ScreenAsh class has defined some methods which ScreenWin/ScreenGtk/ScreenIos > objects don't. Maybe browser_test crash casues by this. I see, in that case, you should set the screen back to ScreenAsh after the test run. > > Now I have updated this patch with a potential solution(inherit ScreenAsh class > on chrome os), and pass test on my local machine.
On 2013/09/24 16:06:43, oshima wrote: > I see, in that case, you should set the screen back to ScreenAsh after the test > run. Done.
On 2013/09/25 03:06:25, Haojian Wu wrote: > On 2013/09/24 16:06:43, oshima wrote: > > I see, in that case, you should set the screen back to ScreenAsh after the > test > > run. > > Done. I uploaded what I meant. It wasn't probably clear enough, sorry. lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/138001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/138001
Failed to apply patch for
chrome/browser/extensions/api/system_display/system_display_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
chrome/browser/extensions/api/system_display/system_display_api.cc
Hunk #1 FAILED at 4.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/system_display/system_display_api.cc.rej
Patch: chrome/browser/extensions/api/system_display/system_display_api.cc
Index: chrome/browser/extensions/api/system_display/system_display_api.cc
diff --git a/chrome/browser/extensions/api/system_display/system_display_api.cc
b/chrome/browser/extensions/api/system_display/system_display_api.cc
index
36dd2f26e73fc2a75e368ff3af643be94b34c923..6dfc774649da952d3308ee85172ea0e40411f4d8
100644
--- a/chrome/browser/extensions/api/system_display/system_display_api.cc
+++ b/chrome/browser/extensions/api/system_display/system_display_api.cc
@@ -4,8 +4,15 @@
#include "chrome/browser/extensions/api/system_display/system_display_api.h"
+#include <string>
+
#include "base/memory/scoped_ptr.h"
+#include "base/strings/string_number_conversions.h"
+#include "chrome/browser/extensions/api/system_display/display_info_provider.h"
+#include "chrome/common/extensions/api/system_display.h"
#include "chrome/common/extensions/manifest_handlers/kiosk_enabled_info.h"
+#include "ui/gfx/display.h"
+#include "ui/gfx/screen.h"
namespace extensions {
@@ -13,25 +20,16 @@ using api::system_display::DisplayUnitInfo;
namespace SetDisplayProperties = api::system_display::SetDisplayProperties;
+typedef std::vector<linked_ptr<
+ api::system_display::DisplayUnitInfo> > DisplayInfo;
+
bool SystemDisplayGetInfoFunction::RunImpl() {
- DisplayInfoProvider::Get()->RequestInfo(
- base::Bind(
- &SystemDisplayGetInfoFunction::OnGetDisplayInfoCompleted,
- this));
+ DisplayInfo all_displays_info =
+ DisplayInfoProvider::Get()->GetAllDisplaysInfo();
+ results_ = api::system_display::GetInfo::Results::Create(all_displays_info);
return true;
}
-void SystemDisplayGetInfoFunction::OnGetDisplayInfoCompleted(
- bool success) {
- if (success) {
- results_ = api::system_display::GetInfo::Results::Create(
- DisplayInfoProvider::Get()->display_info());
- } else {
- SetError("Error occurred when querying display information.");
- }
- SendResponse(success);
-}
-
bool SystemDisplaySetDisplayPropertiesFunction::RunImpl() {
#if !defined(OS_CHROMEOS)
SetError("Function available only on ChromeOS.");
@@ -41,22 +39,16 @@ bool SystemDisplaySetDisplayPropertiesFunction::RunImpl() {
SetError("The extension needs to be kiosk enabled to use the function.");
return false;
}
-
+ std::string error;
scoped_ptr<SetDisplayProperties::Params> params(
SetDisplayProperties::Params::Create(*args_));
- DisplayInfoProvider::Get()->SetInfo(params->id, params->info,
- base::Bind(
- &SystemDisplaySetDisplayPropertiesFunction::OnPropertiesSet,
- this));
- return true;
-#endif
-}
-
-void SystemDisplaySetDisplayPropertiesFunction::OnPropertiesSet(
- bool success, const std::string& error) {
+ bool success = DisplayInfoProvider::Get()->SetInfo(params->id,
+ params->info,
+ &error);
if (!success)
SetError(error);
- SendResponse(success);
+ return true;
+#endif
}
} // namespace extensions
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/169001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
all of a suddend google chrome has been shutting down my computer. i do a virus scan <http://www.city-data.com/knowledge/Antivirus_software.html> and nothing comes up. I remove google chrome and re install. same thing. can anyone help me out? ** Warm regards * * * Farooq B* * 099 4 77 6 0000* * * On Sat, Sep 28, 2013 at 3:32 PM, <commit-bot@chromium.org> wrote: > Retried try job too often on linux_aura for step(s) unit_tests > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=linux_**aura&number=82978<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=82978> > > > https://codereview.chromium.**org/23441032/<https://codereview.chromium.org/2... > > -- > You received this message because you are subscribed to the Google Groups > "Extensions reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to extensions-reviews+**unsubscribe@chromium.org<extensions-reviews%2Bunsubscrib... > . > To view this discussion on the web visit https://groups.google.com/a/** > chromium.org/d/msgid/**extensions-reviews/**089e013d05ea0e40af04e76eb479%* > *40google.com<https://groups.google.com/a/chromium.org/d/msgid/extensions-reviews/089e013d05ea0e40af04e76eb479%40google.com> > . > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23441032/169001
Message was sent while issue was closed.
Change committed as 226102 |
