Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12047111:
Set scale/orientation property to aura root window (Closed)
|
Created:
7 years, 11 months ago by oshima Modified:
7 years, 10 months ago CC:
chromium-reviews, sadrul, ben+watch_chromium.org, adlr Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSet scale/orientation property to aura root window.
These will be used by X/touch/mouse driver to adjust
mouse movement.
BUG=119268
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180053
Patch Set 1 #Patch Set 2 : #
Total comments: 24
Patch Set 3 : #
Total comments: 10
Patch Set 4 : updated comment #Patch Set 5 : use AcceleratedWidget #
Total comments: 4
Patch Set 6 : fix comments #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... File ash/display/display_controller.cc (left): https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:280: root->SetHostBounds(primary_candidate->bounds_in_pixel()); the bound is already set in AddRootWnidowForDisplay, so this wasn't necessary.
ping?
I never received the original message asking for a review request -- did you add me as a reviewer after sending it instead of before? Taking a look now, in any case.
On 2013/01/30 20:38:42, Daniel Erat wrote: > I never received the original message asking for a review request -- did you add > me as a reviewer after sending it instead of before? Taking a look now, in any > case. I added you between 1st and 2nd message (1st one is sent to adlr for proof of concept), so you must have received the 2nd message only, but the full description. That might have caused confusion?
https://codereview.chromium.org/12047111/diff/11004/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/12047111/diff/11004/ash/ash_switches.cc#newco... ash/ash_switches.cc:103: // Overrides the all display's oientation. The value should be one of 0=(normal) nit: fixing some typos and grammar: // Overrides all displays' orientation. The value should be // one of 0 (normal), 1 (90 degrees clockwise), 2 (180 // degrees), or 3 (270 degrees clockwise / 90 degrees // counter-clockwise). but it'd be better to add an enum somewhere and use the values from that instead, e.g. enum DisplayOrientation { DISPLAY_ORIENTATION_NORMAL, DISPLAY_ORIENTATION_90_DEGREES_CW, DISPLAY_ORIENTATION_180_DEGREES, DISPLAY_ORIENTATION_90_DEGREES_CCW, }; or something similar. or maybe having constants with descriptive strings would be even better. https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:112: const char kRotationProp[] = "CHROME_DISPLAY_ROTATION"; either move these to the top of the file or into the method that uses them they should also start with underscores, e.g. _CHROME_DISPLAY_ROTATION. if we have other custom atoms, they should also start with underscores -- see http://tronche.com/gui/x/icccm/sec-1.html#s-1.2.3 ("The protocol specification recommends that atoms used for private vendor-specific reasons should begin with an underscore.") https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:115: void SetDisplayPropertiesToHostWindow(aura::RootWindow* root, nit: s/To/On/ https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:116: const gfx::Display& display) { nit: fix indenting https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... File ui/aura/remote_root_window_host_win.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... ui/aura/remote_root_window_host_win.cc:190: int value) { nit: NOTIMPLEMENTED() https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window.h File ui/aura/root_window.h (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window.h#new... ui/aura/root_window.h:108: // Sets the hosting platform specific property. nits: "Sets a property on the host window using platform-specific means." https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window.h#new... ui/aura/root_window.h:109: void SetHostWindowProperty(const std::string& name, int value); if you plan to store something other than ints later, SetHostWindowIntProperty() or SetIntPropertyOnHostWindow() may be better (or perhaps it'd be better to just store strings to begin with) https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host.h File ui/aura/root_window_host.h (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host.... ui/aura/root_window_host.h:121: virtual void SetHostWindowProperty(const std::string& name, int value) = 0; nit: the other methods don't refer to "host windows"; i'd probably just name the RootWindowHost method something like SetProperty() https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host_... File ui/aura/root_window_host_linux.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host_... ui/aura/root_window_host_linux.cc:841: name.c_str(), i think that the type should be CARDINAL instead https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host_... File ui/aura/root_window_host_win.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host_... ui/aura/root_window_host_win.cc:251: } nit: NOTIMPLEMENTED() https://codereview.chromium.org/12047111/diff/11004/ui/views/widget/desktop_a... File ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/views/widget/desktop_a... ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc:845: int value) { nit: NOTIMPLEMENTED() https://codereview.chromium.org/12047111/diff/11004/ui/views/widget/desktop_a... File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/views/widget/desktop_a... ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:443: int value) { nit: NOTIMPLEMENTED()
https://codereview.chromium.org/12047111/diff/11004/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/12047111/diff/11004/ash/ash_switches.cc#newco... ash/ash_switches.cc:103: // Overrides the all display's oientation. The value should be one of 0=(normal) On 2013/01/30 21:04:27, Daniel Erat wrote: > nit: fixing some typos and grammar: > > // Overrides all displays' orientation. The value should be > // one of 0 (normal), 1 (90 degrees clockwise), 2 (180 > // degrees), or 3 (270 degrees clockwise / 90 degrees > // counter-clockwise). > > but it'd be better to add an enum somewhere and use the values from that > instead, e.g. > > enum DisplayOrientation { > DISPLAY_ORIENTATION_NORMAL, > DISPLAY_ORIENTATION_90_DEGREES_CW, > DISPLAY_ORIENTATION_180_DEGREES, > DISPLAY_ORIENTATION_90_DEGREES_CCW, > }; > > or something similar. or maybe having constants with descriptive strings would > be even better. Updated the comment. I agree that it should be enum, but i want to do it when I work on real implementation (it will be in ui/gfx/display.h). For now, i want to keep it just int as all we want is to be able to pass this integer value from command line to X/driver. (and enum wont be used anywhere now) https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:112: const char kRotationProp[] = "CHROME_DISPLAY_ROTATION"; On 2013/01/30 21:04:27, Daniel Erat wrote: > either move these to the top of the file or into the method that uses them > > they should also start with underscores, e.g. _CHROME_DISPLAY_ROTATION. if we > have other custom atoms, they should also start with underscores -- see > http://tronche.com/gui/x/icccm/sec-1.html#s-1.2.3 ("The protocol specification > recommends that atoms used for private vendor-specific reasons should begin with > an underscore.") Done. https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:115: void SetDisplayPropertiesToHostWindow(aura::RootWindow* root, On 2013/01/30 21:04:27, Daniel Erat wrote: > nit: s/To/On/ Done. https://codereview.chromium.org/12047111/diff/11004/ash/display/display_contr... ash/display/display_controller.cc:116: const gfx::Display& display) { On 2013/01/30 21:04:27, Daniel Erat wrote: > nit: fix indenting Done. https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... File ui/aura/remote_root_window_host_win.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... ui/aura/remote_root_window_host_win.cc:190: int value) { On 2013/01/30 21:04:27, Daniel Erat wrote: > nit: NOTIMPLEMENTED() I used to put NOTIMPLEMENTED for blank method, but I'm a bit annoyed by the fact that we're doing it for the method that doesn't have to be implemented (which print out unnecessary warning message). I understand that that's necessary for the method that should/will be implemented (like OnDeviceScaleFactorChanged), but i think NOTIMPLEMENTED isn't necessary for this case. WDTY? https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window.h File ui/aura/root_window.h (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window.h#new... ui/aura/root_window.h:108: // Sets the hosting platform specific property. On 2013/01/30 21:04:27, Daniel Erat wrote: > nits: "Sets a property on the host window using platform-specific means." Done. https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window.h#new... ui/aura/root_window.h:109: void SetHostWindowProperty(const std::string& name, int value); On 2013/01/30 21:04:27, Daniel Erat wrote: > if you plan to store something other than ints later, SetHostWindowIntProperty() > or SetIntPropertyOnHostWindow() may be better (or perhaps it'd be better to just > store strings to begin with) I have no plan to add other than ints, but agree that it's better to include int for clarify. Done. https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host.h File ui/aura/root_window_host.h (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host.... ui/aura/root_window_host.h:121: virtual void SetHostWindowProperty(const std::string& name, int value) = 0; On 2013/01/30 21:04:27, Daniel Erat wrote: > nit: the other methods don't refer to "host windows"; i'd probably just name the > RootWindowHost method something like SetProperty() Renamed to SetIntProperty. https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host_... File ui/aura/root_window_host_linux.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/root_window_host_... ui/aura/root_window_host_linux.cc:841: name.c_str(), On 2013/01/30 21:04:27, Daniel Erat wrote: > i think that the type should be CARDINAL instead Done.
lgtm https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... File ui/aura/remote_root_window_host_win.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... ui/aura/remote_root_window_host_win.cc:190: int value) { On 2013/01/30 23:45:53, oshima wrote: > On 2013/01/30 21:04:27, Daniel Erat wrote: > > nit: NOTIMPLEMENTED() > > I used to put NOTIMPLEMENTED for blank method, but I'm a bit annoyed by > the fact that we're doing it for the method that doesn't have to be > implemented (which print out unnecessary warning message). > > I understand that that's necessary for the method that should/will be > implemented (like OnDeviceScaleFactorChanged), but i think NOTIMPLEMENTED > isn't necessary for this case. WDTY? I think it makes sense to use NOTIMPLEMENTED() even if you don't have plans to implement it (or maybe even NOTREACHED() if it should really never be called). If someone calls this method on a platform where it isn't implemented, it's useful for them to see an error. https://codereview.chromium.org/12047111/diff/9010/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/12047111/diff/9010/ash/ash_switches.cc#newcod... ash/ash_switches.cc:103: // Overrides all display's orientation. The value should be one of 0 nit: s/display's/displays'/ (since displays is plural) https://codereview.chromium.org/12047111/diff/9010/ash/ash_switches.cc#newcod... ash/ash_switches.cc:104: // (normal), 1 (90 degrees clock wise), 2 (180 degrees) or 3 (270 nit: s/clock wise/clockwise/ https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... ash/display/display_controller.cc:111: // (normal), 1 (90 degrees clockwise), 2 (180 degree) or 3 (270 nit: s/180 degree/180 degrees/ https://codereview.chromium.org/12047111/diff/9010/ui/aura/root_window_host_l... File ui/aura/root_window_host_linux.cc (right): https://codereview.chromium.org/12047111/diff/9010/ui/aura/root_window_host_l... ui/aura/root_window_host_linux.cc:839: xwindow_, name.c_str(), XGetAtomName(xdisplay_, XA_CARDINAL), value); hmm, don't use XGetAtomName() here; it does a round trip to the server. i think that it's safe to use "CARDINAL"; this is a built-in atom in X.
https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... File ui/aura/remote_root_window_host_win.cc (right): https://codereview.chromium.org/12047111/diff/11004/ui/aura/remote_root_windo... ui/aura/remote_root_window_host_win.cc:190: int value) { On 2013/01/30 23:52:45, Daniel Erat wrote: > On 2013/01/30 23:45:53, oshima wrote: > > On 2013/01/30 21:04:27, Daniel Erat wrote: > > > nit: NOTIMPLEMENTED() > > > > I used to put NOTIMPLEMENTED for blank method, but I'm a bit annoyed by > > the fact that we're doing it for the method that doesn't have to be > > implemented (which print out unnecessary warning message). > > > > I understand that that's necessary for the method that should/will be > > implemented (like OnDeviceScaleFactorChanged), but i think NOTIMPLEMENTED > > isn't necessary for this case. WDTY? > > I think it makes sense to use NOTIMPLEMENTED() even if you don't have plans to > implement it (or maybe even NOTREACHED() if it should really never be called). > If someone calls this method on a platform where it isn't implemented, it's > useful for them to see an error. Ok, NOTREACHED makes sense for this case. I just thought that a method that is intentionally left blank shouldn't be considered an error or marked as "should be implemented". https://codereview.chromium.org/12047111/diff/9010/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/12047111/diff/9010/ash/ash_switches.cc#newcod... ash/ash_switches.cc:103: // Overrides all display's orientation. The value should be one of 0 On 2013/01/30 23:52:46, Daniel Erat wrote: > nit: s/display's/displays'/ (since displays is plural) Done. https://codereview.chromium.org/12047111/diff/9010/ash/ash_switches.cc#newcod... ash/ash_switches.cc:104: // (normal), 1 (90 degrees clock wise), 2 (180 degrees) or 3 (270 On 2013/01/30 23:52:46, Daniel Erat wrote: > nit: s/clock wise/clockwise/ Done. https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... ash/display/display_controller.cc:111: // (normal), 1 (90 degrees clockwise), 2 (180 degree) or 3 (270 On 2013/01/30 23:52:46, Daniel Erat wrote: > nit: s/180 degree/180 degrees/ Done. https://codereview.chromium.org/12047111/diff/9010/ui/aura/root_window_host_l... File ui/aura/root_window_host_linux.cc (right): https://codereview.chromium.org/12047111/diff/9010/ui/aura/root_window_host_l... ui/aura/root_window_host_linux.cc:839: xwindow_, name.c_str(), XGetAtomName(xdisplay_, XA_CARDINAL), value); On 2013/01/30 23:52:46, Daniel Erat wrote: > hmm, don't use XGetAtomName() here; it does a round trip to the server. i think > that it's safe to use "CARDINAL"; this is a built-in atom in X. Ah, sorry I misunderstood what you were asking. done. That's being said, I believe this is arbitrary for custom property right? Other places are using the same value for name and type...
+ben for OWNERS review
https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... ash/display/display_controller.cc:128: root->SetHostWindowIntProperty(kRotationProp, rotation); Where is this property read, since you don't provide a getter on RWH?
https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/9010/ash/display/display_contro... ash/display/display_controller.cc:128: root->SetHostWindowIntProperty(kRotationProp, rotation); On 2013/01/31 16:57:13, Ben Goodger (Google) wrote: > Where is this property read, since you don't provide a getter on RWH? This will be read by touchpad/mouse driver to control the speed and direction of mouse pointer movement.
I see... since this is very platform specific, would it be possible to do this in an #if defined(OS_CHROMEOS) block in DisplayController, instead of wiring through this API? It's not clear to me that anyone else would use it.
Specifically, you can just grab the XWindow from RootWindow by calling GetAcceleratedWidget().
On 2013/01/31 22:11:41, Ben Goodger (Google) wrote: > Specifically, you can just grab the XWindow from RootWindow by calling > GetAcceleratedWidget(). Ah, I didn't know I can get XWindow from RootWindow. I'll fix it right away, thanks.
uploaded new patch. Much smaller now:) PTAL
lgtm https://codereview.chromium.org/12047111/diff/28001/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/12047111/diff/28001/ash/ash_switches.cc#newco... ash/ash_switches.cc:101: // (normal), 1 (90 degrees clock wise), 2 (180 degrees) or 3 (270 same nits as before: s/display's/displays'/ s/clock wise/clockwise/ https://codereview.chromium.org/12047111/diff/28001/ash/display/display_contr... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/28001/ash/display/display_contr... ash/display/display_controller.cc:120: const char CARDINAL[] = "CARDINAL"; kCardinal
since there is no change in aura, i'll go ahead to CQ. https://codereview.chromium.org/12047111/diff/28001/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/12047111/diff/28001/ash/ash_switches.cc#newco... ash/ash_switches.cc:101: // (normal), 1 (90 degrees clock wise), 2 (180 degrees) or 3 (270 On 2013/01/31 22:38:29, Daniel Erat wrote: > same nits as before: > > s/display's/displays'/ > s/clock wise/clockwise/ Hmm, I thought I fixed them... Thank you for the catch. https://codereview.chromium.org/12047111/diff/28001/ash/display/display_contr... File ash/display/display_controller.cc (right): https://codereview.chromium.org/12047111/diff/28001/ash/display/display_contr... ash/display/display_controller.cc:120: const char CARDINAL[] = "CARDINAL"; On 2013/01/31 22:38:29, Daniel Erat wrote: > kCardinal Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/12047111/14030
Message was sent while issue was closed.
Change committed as 180053
Message was sent while issue was closed.
great. lgtm. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
