Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12217120:
Add IsInternal property to gfx::Display (Closed)
|
Created:
7 years, 10 months ago by ynovikov Modified:
7 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, sadrul, ben+watch_chromium.org, Aaron Boodman, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd IsInternal property to gfx::Display
Move knowledge whether a display is internal or not
from ash::DisplayManager into gfx::Display.
BUG=171310
TEST=ash_unittests, ui_unittests, unit_tests pass
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182494
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183230
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review #
Total comments: 6
Patch Set 3 : Nits + Rebase #
Total comments: 2
Patch Set 4 : Get rid of static initializer #
Messages
Total messages: 24 (0 generated)
This is a pre-requisite for https://codereview.chromium.org/12087124/, as suggested by it's review. I've decided to take a bit different approach than suggested by oshima in https://codereview.chromium.org/12194023, as I don't feel comfortable with external entity being able to modify gfx::Display internal state in such a way.
https://codereview.chromium.org/12217120/diff/1/ash/accelerators/accelerator_... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/12217120/diff/1/ash/accelerators/accelerator_... ash/accelerators/accelerator_controller_unittest.cc:320: void DisableInternalDisplay() { It's better to reset in AshTestBase::TearDown, and you probably don't need a method. https://codereview.chromium.org/12217120/diff/1/ash/display/display_manager.h File ash/display/display_manager.h (right): https://codereview.chromium.org/12217120/diff/1/ash/display/display_manager.h... ash/display/display_manager.h:173: void ResetInternalDisplayForTest(); If you want to keep this, then please move this to test::DisplayManagerTestApi. (I'm moving other tests method to there) https://codereview.chromium.org/12217120/diff/1/ash/wm/system_gesture_event_f... File ash/wm/system_gesture_event_filter_unittest.cc (right): https://codereview.chromium.org/12217120/diff/1/ash/wm/system_gesture_event_f... ash/wm/system_gesture_event_filter_unittest.cc:223: Shell::GetInstance()->display_manager()->ResetInternalDisplayForTest(); ditto. should be in AshTestBase https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h#newcode105 ui/gfx/display.h:105: bool is_internal() const; IsInternal() https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h#newcode109 ui/gfx/display.h:109: static void set_internal_display_id(int64 internal_display_id); same for these https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h#newcode121 ui/gfx/display.h:121: static int64 internal_display_id_; move this to anonymous namespace in .cc file.
https://codereview.chromium.org/12217120/diff/1/ash/accelerators/accelerator_... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/12217120/diff/1/ash/accelerators/accelerator_... ash/accelerators/accelerator_controller_unittest.cc:320: void DisableInternalDisplay() { On 2013/02/12 17:38:51, oshima wrote: > It's better to reset in AshTestBase::TearDown, > and you probably don't need a method. Done. https://codereview.chromium.org/12217120/diff/1/ash/display/display_manager.h File ash/display/display_manager.h (right): https://codereview.chromium.org/12217120/diff/1/ash/display/display_manager.h... ash/display/display_manager.h:173: void ResetInternalDisplayForTest(); On 2013/02/12 17:38:51, oshima wrote: > If you want to keep this, then please move this to test::DisplayManagerTestApi. > > (I'm moving other tests method to there) Done. https://codereview.chromium.org/12217120/diff/1/ash/wm/system_gesture_event_f... File ash/wm/system_gesture_event_filter_unittest.cc (right): https://codereview.chromium.org/12217120/diff/1/ash/wm/system_gesture_event_f... ash/wm/system_gesture_event_filter_unittest.cc:223: Shell::GetInstance()->display_manager()->ResetInternalDisplayForTest(); On 2013/02/12 17:38:51, oshima wrote: > ditto. should be in AshTestBase Done. https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h#newcode105 ui/gfx/display.h:105: bool is_internal() const; On 2013/02/12 17:38:51, oshima wrote: > IsInternal() Done. https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h#newcode109 ui/gfx/display.h:109: static void set_internal_display_id(int64 internal_display_id); On 2013/02/12 17:38:51, oshima wrote: > same for these Done. https://codereview.chromium.org/12217120/diff/1/ui/gfx/display.h#newcode121 ui/gfx/display.h:121: static int64 internal_display_id_; On 2013/02/12 17:38:51, oshima wrote: > move this to anonymous namespace in .cc file. Done.
lgtm with nits https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc File ui/gfx/display.cc (right): https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode35 ui/gfx/display.cc:35: } // namespace this is nor your fault, but could please fix this? two spaced between } and // https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode39 ui/gfx/display.cc:39: int64 internal_display_id_ = Display::kInvalidDisplayID; indent https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode40 ui/gfx/display.cc:40: } // namespace move this into the anonymous namespace above.
oshima, thank you. ben, can you review ash/system/chromeos, ash/test, chrome/browser/extensions/api/system_info_display and chrome/browser/ui/webui/options/chromeos, or should I look for more specific owners? I'll still need you for ash/test, though. https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc File ui/gfx/display.cc (right): https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode35 ui/gfx/display.cc:35: } // namespace On 2013/02/12 21:39:53, oshima wrote: > this is nor your fault, but could please fix this? > two spaced between } and // Done. https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode39 ui/gfx/display.cc:39: int64 internal_display_id_ = Display::kInvalidDisplayID; On 2013/02/12 21:39:53, oshima wrote: > indent Done. https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode40 ui/gfx/display.cc:40: } // namespace On 2013/02/12 21:39:53, oshima wrote: > move this into the anonymous namespace above. Done.
yeah lgtm.
Actually nevermind, I'll take this. On Wed, Feb 13, 2013 at 2:37 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > I think you should find more specific owners. > > > On Wed, Feb 13, 2013 at 5:48 AM, <ynovikov@chromium.org> wrote: > >> oshima, thank you. >> ben, can you review ash/system/chromeos, ash/test, >> chrome/browser/extensions/api/**system_info_display and >> chrome/browser/ui/webui/**options/chromeos, or should I look for more >> specific >> owners? I'll still need you for ash/test, though. >> >> >> >> https://codereview.chromium.**org/12217120/diff/12004/ui/**gfx/display.cc<htt... >> File ui/gfx/display.cc (right): >> >> https://codereview.chromium.**org/12217120/diff/12004/ui/** >> gfx/display.cc#newcode35<https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode35> >> ui/gfx/display.cc:35: } // namespace >> On 2013/02/12 21:39:53, oshima wrote: >> >>> this is nor your fault, but could please fix this? >>> two spaced between } and // >>> >> >> Done. >> >> >> https://codereview.chromium.**org/12217120/diff/12004/ui/** >> gfx/display.cc#newcode39<https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode39> >> ui/gfx/display.cc:39: int64 internal_display_id_ = >> Display::kInvalidDisplayID; >> On 2013/02/12 21:39:53, oshima wrote: >> >>> indent >>> >> >> Done. >> >> >> https://codereview.chromium.**org/12217120/diff/12004/ui/** >> gfx/display.cc#newcode40<https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode40> >> ui/gfx/display.cc:40: } // namespace >> On 2013/02/12 21:39:53, oshima wrote: >> >>> move this into the anonymous namespace above. >>> >> >> Done. >> >> https://codereview.chromium.**org/12217120/<https://codereview.chromium.org/1... >> > >
I think you should find more specific owners. On Wed, Feb 13, 2013 at 5:48 AM, <ynovikov@chromium.org> wrote: > oshima, thank you. > ben, can you review ash/system/chromeos, ash/test, > chrome/browser/extensions/api/**system_info_display and > chrome/browser/ui/webui/**options/chromeos, or should I look for more > specific > owners? I'll still need you for ash/test, though. > > > > https://codereview.chromium.**org/12217120/diff/12004/ui/**gfx/display.cc<htt... > File ui/gfx/display.cc (right): > > https://codereview.chromium.**org/12217120/diff/12004/ui/** > gfx/display.cc#newcode35<https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode35> > ui/gfx/display.cc:35: } // namespace > On 2013/02/12 21:39:53, oshima wrote: > >> this is nor your fault, but could please fix this? >> two spaced between } and // >> > > Done. > > > https://codereview.chromium.**org/12217120/diff/12004/ui/** > gfx/display.cc#newcode39<https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode39> > ui/gfx/display.cc:39: int64 internal_display_id_ = > Display::kInvalidDisplayID; > On 2013/02/12 21:39:53, oshima wrote: > >> indent >> > > Done. > > > https://codereview.chromium.**org/12217120/diff/12004/ui/** > gfx/display.cc#newcode40<https://codereview.chromium.org/12217120/diff/12004/ui/gfx/display.cc#newcode40> > ui/gfx/display.cc:40: } // namespace > On 2013/02/12 21:39:53, oshima wrote: > >> move this into the anonymous namespace above. >> > > Done. > > https://codereview.chromium.**org/12217120/<https://codereview.chromium.org/1... >
Thank you. On 2013-02-13 6:03 PM, <ben@chromium.org> wrote: > yeah lgtm. > > https://codereview.chromium.**org/12217120/<https://codereview.chromium.org/1... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ynovikov@chromium.org/12217120/17001
Message was sent while issue was closed.
Change committed as 182494
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12217120/diff/17001/ui/gfx/display.cc File ui/gfx/display.cc (right): https://chromiumcodereview.appspot.com/12217120/diff/17001/ui/gfx/display.cc#... ui/gfx/display.cc:35: int64 internal_display_id_ = Display::kInvalidDisplayID; Sorry for not catching this. This doesn't work (it depends on the order that static variables are initialized). Can you change to something like const int64 kInternalInvalidDisplayID = -1; int64 internal_display_id = kInternalInvalidDisplayID; // static const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; ?
https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc File ui/gfx/display.cc (right): https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 ui/gfx/display.cc:35: int64 internal_display_id_ = Display::kInvalidDisplayID; On 2013/02/14 21:01:33, oshima wrote: > Sorry for not catching this. This doesn't work (it depends on the order > that static variables are initialized). Can you change to something like > > const int64 kInternalInvalidDisplayID = -1; > > int64 internal_display_id = kInternalInvalidDisplayID; > > // static > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > ? Done. (That's why I've split the anonymous namespace in the first place.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ynovikov@chromium.org/12217120/30001
On 2013/02/15 17:42:42, ynovikov wrote: > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > File ui/gfx/display.cc (right): > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > ui/gfx/display.cc:35: int64 internal_display_id_ = Display::kInvalidDisplayID; > On 2013/02/14 21:01:33, oshima wrote: > > Sorry for not catching this. This doesn't work (it depends on the order > > that static variables are initialized). Can you change to something like > > > > const int64 kInternalInvalidDisplayID = -1; > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > // static > > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > > > ? > > Done. > (That's why I've split the anonymous namespace in the first place.) No it still won't work because there is no guarantee to the order static initializer. For example, the following behavior is undefined. static int a = 0; static int b = a++; static int c = a++;
On 2013/02/15 17:59:07, oshima wrote: > On 2013/02/15 17:42:42, ynovikov wrote: > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > > File ui/gfx/display.cc (right): > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > > ui/gfx/display.cc:35: int64 internal_display_id_ = Display::kInvalidDisplayID; > > On 2013/02/14 21:01:33, oshima wrote: > > > Sorry for not catching this. This doesn't work (it depends on the order > > > that static variables are initialized). Can you change to something like > > > > > > const int64 kInternalInvalidDisplayID = -1; > > > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > > > // static > > > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > > > > > ? > > > > Done. > > (That's why I've split the anonymous namespace in the first place.) > > No it still won't work because there is no guarantee to the order static > initializer. > For example, the following behavior is undefined. > > static int a = 0; > static int b = a++; > static int c = a++; What won't work? The way you suggested, everything is initialized in compile-time, I've verified that the binary doesn't contain a static initializer. This is different from your example of undefined behavior, as we have const.
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. 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.
On 2013/02/15 18:09:32, ynovikov wrote: > On 2013/02/15 17:59:07, oshima wrote: > > On 2013/02/15 17:42:42, ynovikov wrote: > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > > > File ui/gfx/display.cc (right): > > > > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > > > ui/gfx/display.cc:35: int64 internal_display_id_ = > Display::kInvalidDisplayID; > > > On 2013/02/14 21:01:33, oshima wrote: > > > > Sorry for not catching this. This doesn't work (it depends on the order > > > > that static variables are initialized). Can you change to something like > > > > > > > > const int64 kInternalInvalidDisplayID = -1; > > > > > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > > > > > // static > > > > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > > > > > > > ? > > > > > > Done. > > > (That's why I've split the anonymous namespace in the first place.) > > > > No it still won't work because there is no guarantee to the order static > > initializer. > > For example, the following behavior is undefined. > > > > static int a = 0; > > static int b = a++; > > static int c = a++; > > What won't work? > The way you suggested, everything is initialized in compile-time, I've verified > that the binary doesn't contain a static initializer. > This is different from your example of undefined behavior, as we have const. kInvalidDisplayID is static const (which is different from just const) whose value is defined in .cc file, so it has to be initialized at runtime, which is basically the same issue as above.
On 2013/02/15 18:28:35, oshima wrote: > On 2013/02/15 18:09:32, ynovikov wrote: > > On 2013/02/15 17:59:07, oshima wrote: > > > On 2013/02/15 17:42:42, ynovikov wrote: > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > > > > File ui/gfx/display.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > > > > ui/gfx/display.cc:35: int64 internal_display_id_ = > > Display::kInvalidDisplayID; > > > > On 2013/02/14 21:01:33, oshima wrote: > > > > > Sorry for not catching this. This doesn't work (it depends on the order > > > > > that static variables are initialized). Can you change to something like > > > > > > > > > > const int64 kInternalInvalidDisplayID = -1; > > > > > > > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > > > > > > > // static > > > > > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > > > > > > > > > ? > > > > > > > > Done. > > > > (That's why I've split the anonymous namespace in the first place.) > > > > > > No it still won't work because there is no guarantee to the order static > > > initializer. > > > For example, the following behavior is undefined. > > > > > > static int a = 0; > > > static int b = a++; > > > static int c = a++; > > > > What won't work? > > The way you suggested, everything is initialized in compile-time, I've > verified > > that the binary doesn't contain a static initializer. > > This is different from your example of undefined behavior, as we have const. > > kInvalidDisplayID is static const (which is different from just const) > whose value is defined in .cc file, so it has to be initialized at runtime, > which is basically the same issue as above. Hmmm... Here is objdump -s ./chroot/var/cache/chromeos-chrome/chrome-src/src/out_link/Release/obj.target/ui/ui/gfx/display.o Contents of section .rodata._ZN3gfx7Display17kInvalidDisplayIDE: 0000 ffffffff ffffffff ........ Contents of section .data._ZN3gfx12_GLOBAL__N_120internal_display_id_E: 0000 ffffffff ffffffff ........ As you can see, both gfx::Display::kInvalidDisplayID and gfx::(anonymous namespace)::internal_display_id_ are initialized to -1 in the binary. I also see no reference to these variables in the disassembly of display.o. Do you think this is compiler-dependent behavior, and can be broken elsewhere? How do you suggest to fix it then?
On 2013/02/15 19:25:02, ynovikov wrote: > On 2013/02/15 18:28:35, oshima wrote: > > On 2013/02/15 18:09:32, ynovikov wrote: > > > On 2013/02/15 17:59:07, oshima wrote: > > > > On 2013/02/15 17:42:42, ynovikov wrote: > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > > > > > File ui/gfx/display.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > > > > > ui/gfx/display.cc:35: int64 internal_display_id_ = > > > Display::kInvalidDisplayID; > > > > > On 2013/02/14 21:01:33, oshima wrote: > > > > > > Sorry for not catching this. This doesn't work (it depends on the > order > > > > > > that static variables are initialized). Can you change to something > like > > > > > > > > > > > > const int64 kInternalInvalidDisplayID = -1; > > > > > > > > > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > > > > > > > > > // static > > > > > > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > > > > > > > > > > > ? > > > > > > > > > > Done. > > > > > (That's why I've split the anonymous namespace in the first place.) > > > > > > > > No it still won't work because there is no guarantee to the order static > > > > initializer. > > > > For example, the following behavior is undefined. > > > > > > > > static int a = 0; > > > > static int b = a++; > > > > static int c = a++; > > > > > > What won't work? > > > The way you suggested, everything is initialized in compile-time, I've > > verified > > > that the binary doesn't contain a static initializer. > > > This is different from your example of undefined behavior, as we have const. > > > > kInvalidDisplayID is static const (which is different from just const) > > whose value is defined in .cc file, so it has to be initialized at runtime, > > which is basically the same issue as above. > > Hmmm... > Here is objdump -s > ./chroot/var/cache/chromeos-chrome/chrome-src/src/out_link/Release/obj.target/ui/ui/gfx/display.o > > Contents of section .rodata._ZN3gfx7Display17kInvalidDisplayIDE: > 0000 ffffffff ffffffff ........ > Contents of section .data._ZN3gfx12_GLOBAL__N_120internal_display_id_E: > 0000 ffffffff ffffffff ........ > > As you can see, both gfx::Display::kInvalidDisplayID and gfx::(anonymous > namespace)::internal_display_id_ are initialized to -1 in the binary. I also see > no reference to these variables in the disassembly of display.o. Do you think > this is compiler-dependent behavior, and can be broken elsewhere? How do you > suggest to fix it then? My understanding is that this is just a result of compiler figuring out the dependency and putting -1, instead of assigning the value of kInvalidDisplayID. This may (and will) work on most compiler but not guaranteed by C++ spec, and that's why bot complained.
On 2013/02/15 19:48:41, oshima wrote: > On 2013/02/15 19:25:02, ynovikov wrote: > > On 2013/02/15 18:28:35, oshima wrote: > > > On 2013/02/15 18:09:32, ynovikov wrote: > > > > On 2013/02/15 17:59:07, oshima wrote: > > > > > On 2013/02/15 17:42:42, ynovikov wrote: > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > > > > > > File ui/gfx/display.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > > > > > > ui/gfx/display.cc:35: int64 internal_display_id_ = > > > > Display::kInvalidDisplayID; > > > > > > On 2013/02/14 21:01:33, oshima wrote: > > > > > > > Sorry for not catching this. This doesn't work (it depends on the > > order > > > > > > > that static variables are initialized). Can you change to something > > like > > > > > > > > > > > > > > const int64 kInternalInvalidDisplayID = -1; > > > > > > > > > > > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > > > > > > > > > > > // static > > > > > > > const int64 Display::kInvalidDisplayID = kInternalInvalidDisplayID; > > > > > > > > > > > > > > ? > > > > > > > > > > > > Done. > > > > > > (That's why I've split the anonymous namespace in the first place.) > > > > > > > > > > No it still won't work because there is no guarantee to the order static > > > > > initializer. > > > > > For example, the following behavior is undefined. > > > > > > > > > > static int a = 0; > > > > > static int b = a++; > > > > > static int c = a++; > > > > > > > > What won't work? > > > > The way you suggested, everything is initialized in compile-time, I've > > > verified > > > > that the binary doesn't contain a static initializer. > > > > This is different from your example of undefined behavior, as we have > const. > > > > > > kInvalidDisplayID is static const (which is different from just const) > > > whose value is defined in .cc file, so it has to be initialized at runtime, > > > which is basically the same issue as above. > > > > Hmmm... > > Here is objdump -s > > > ./chroot/var/cache/chromeos-chrome/chrome-src/src/out_link/Release/obj.target/ui/ui/gfx/display.o > > > > Contents of section .rodata._ZN3gfx7Display17kInvalidDisplayIDE: > > 0000 ffffffff ffffffff ........ > > Contents of section .data._ZN3gfx12_GLOBAL__N_120internal_display_id_E: > > 0000 ffffffff ffffffff ........ > > > > As you can see, both gfx::Display::kInvalidDisplayID and gfx::(anonymous > > namespace)::internal_display_id_ are initialized to -1 in the binary. I also > see > > no reference to these variables in the disassembly of display.o. Do you think > > this is compiler-dependent behavior, and can be broken elsewhere? How do you > > suggest to fix it then? > > My understanding is that this is just a result of compiler figuring out the > dependency and > putting -1, instead of assigning the value of kInvalidDisplayID. > > This may (and will) work on most compiler but not guaranteed by C++ spec, and > that's why > bot complained. No, no. This is the binary after patchset #4. Before that indeed there was a static initializer. So, can I proceed with committing this? BTW, is there a way to try the "sizes" bot before committing, I couldn't find it in the list?
On 2013/02/15 19:52:02, ynovikov wrote: > On 2013/02/15 19:48:41, oshima wrote: > > On 2013/02/15 19:25:02, ynovikov wrote: > > > On 2013/02/15 18:28:35, oshima wrote: > > > > On 2013/02/15 18:09:32, ynovikov wrote: > > > > > On 2013/02/15 17:59:07, oshima wrote: > > > > > > On 2013/02/15 17:42:42, ynovikov wrote: > > > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc > > > > > > > File ui/gfx/display.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/12217120/diff/17001/ui/gfx/display.cc#newcode35 > > > > > > > ui/gfx/display.cc:35: int64 internal_display_id_ = > > > > > Display::kInvalidDisplayID; > > > > > > > On 2013/02/14 21:01:33, oshima wrote: > > > > > > > > Sorry for not catching this. This doesn't work (it depends on the > > > order > > > > > > > > that static variables are initialized). Can you change to > something > > > like > > > > > > > > > > > > > > > > const int64 kInternalInvalidDisplayID = -1; > > > > > > > > > > > > > > > > int64 internal_display_id = kInternalInvalidDisplayID; > > > > > > > > > > > > > > > > // static > > > > > > > > const int64 Display::kInvalidDisplayID = > kInternalInvalidDisplayID; > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > Done. > > > > > > > (That's why I've split the anonymous namespace in the first place.) > > > > > > > > > > > > No it still won't work because there is no guarantee to the order > static > > > > > > initializer. > > > > > > For example, the following behavior is undefined. > > > > > > > > > > > > static int a = 0; > > > > > > static int b = a++; > > > > > > static int c = a++; > > > > > > > > > > What won't work? > > > > > The way you suggested, everything is initialized in compile-time, I've > > > > verified > > > > > that the binary doesn't contain a static initializer. > > > > > This is different from your example of undefined behavior, as we have > > const. > > > > > > > > kInvalidDisplayID is static const (which is different from just const) > > > > whose value is defined in .cc file, so it has to be initialized at > runtime, > > > > which is basically the same issue as above. > > > > > > Hmmm... > > > Here is objdump -s > > > > > > ./chroot/var/cache/chromeos-chrome/chrome-src/src/out_link/Release/obj.target/ui/ui/gfx/display.o > > > > > > Contents of section .rodata._ZN3gfx7Display17kInvalidDisplayIDE: > > > 0000 ffffffff ffffffff ........ > > > Contents of section .data._ZN3gfx12_GLOBAL__N_120internal_display_id_E: > > > 0000 ffffffff ffffffff ........ > > > > > > As you can see, both gfx::Display::kInvalidDisplayID and gfx::(anonymous > > > namespace)::internal_display_id_ are initialized to -1 in the binary. I also > > see > > > no reference to these variables in the disassembly of display.o. Do you > think > > > this is compiler-dependent behavior, and can be broken elsewhere? How do you > > > suggest to fix it then? > > > > My understanding is that this is just a result of compiler figuring out the > > dependency and > > putting -1, instead of assigning the value of kInvalidDisplayID. > > > > This may (and will) work on most compiler but not guaranteed by C++ spec, and > > that's why > > bot complained. > > No, no. This is the binary after patchset #4. Before that indeed there was a > static initializer. So, can I proceed with committing this? BTW, is there a way > to try the "sizes" bot before committing, I couldn't find it in the list? Ah, yes this is fine. Thank you for double checking. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ynovikov@chromium.org/12217120/30001
Message was sent while issue was closed.
Change committed as 183230 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
