Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8733014:
New-style Disconnect window for Linux Chromoting host. (Closed)
|
Created:
9 years ago by Lambros Modified:
9 years ago Reviewers:
Jamie CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNew-style Disconnect window for Linux Chromoting host.
BUG=None
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112831
Patch Set 1 #
Total comments: 23
Patch Set 2 : Fixed some nits, and a memory-leak. #Messages
Total messages: 6 (0 generated)
http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... File remoting/host/disconnect_window_linux.cc (right): http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:66: GtkWindow* window = GTK_WINDOW(disconnect_window_); Do you ever not want to see disconnect_window_ as a GtkWindow*? Might it be better to declare the class member as that type and cast before storing? http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:68: g_signal_connect(disconnect_window_, "delete-event", ...For example, will this still compile if disconnect_window_ is of type GtkWindow*? http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:70: gtk_window_set_title(window, UTF16ToUTF8(ui_strings.product_name).c_str()); What's the purpose of setting the title? http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:93: current_height_ = current_width_ = 0; I don't think this is necessary, as we regenerate bitmaps if the size has changed anyway, and if it hasn't then the old bitmaps can be reused. Also, this sort of indirect indication that the bitmap needs to be regenerated makes me nervous--can we reset the bitmap itself instead, or is that owned by the window? http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:105: // window movement gripper. Will this work for bidi languages? If not, then that can be fixed in a separate CL. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:191: GdkPixmap* shape_mask = gdk_pixmap_new(NULL, current_width_, current_height_, I assume that cairo_destroy also destroys this pixmap? It might be worth checking to see if there's a scoped_ptr equivalent for this, which would save a couple of calls to cairo_destroy. Not really worth creating one if not though. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:193: cairo_t* cr = gdk_cairo_create(shape_mask); Can this have a more descriptive name, please? http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:198: // sizes just in case. What happens if we don't? If it just looks crappy then I don't think we care (it's going to be unusable if resized too small anyway). http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:222: cairo_set_source_rgb(cr, 233 / 256.0, 233 / 256.0, 233 / 256.0); Seems an odd way of specifying the colour. I think the Mac version also uses fractional RGB values, but specifies them as fractions. To mimic the values, the Windows version multiplies them up, but you seem to be doing the opposite here. I recommend you just copy the fractional values from the Mac source directly. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:235: int gripper_bottom = current_height_ / 2 + 10.5; I don't understand this. Why is one of these a double and the other an int? And why do the y-coordinates need to be fractional? I think the comment is intended to explain this, but I'm still in the dark. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:239: cairo_set_source_rgb(cr, 194 / 256.0, 194 / 256.0, 194 / 256.0); Seems an odd way of specifying the colour. I think the Mac version also uses fractional RGB values, but specifies them as fractions. To mimic the values, the Windows version multiplies them up, but you seem to be doing the opposite here.
Addressed most comments, PTAL. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... File remoting/host/disconnect_window_linux.cc (right): http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:66: GtkWindow* window = GTK_WINDOW(disconnect_window_); On 2011/11/30 02:48:56, Jamie wrote: > Do you ever not want to see disconnect_window_ as a GtkWindow*? Might it be > better to declare the class member as that type and cast before storing? The Chrome examples I saw elsewhere used GtkWidget* for storing the window, and it is the declared return type of gtk_window_new(), which does seem unusual :) We do need it as a GtkWidget* in a couple of places at least. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:68: g_signal_connect(disconnect_window_, "delete-event", On 2011/11/30 02:48:56, Jamie wrote: > ...For example, will this still compile if disconnect_window_ is of type > GtkWindow*? That particular line would still compile (g_signal_connect is a macro), but others won't, for example gtk_widget_set_app_paintable() and gtk_widget_show_all(). http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:70: gtk_window_set_title(window, UTF16ToUTF8(ui_strings.product_name).c_str()); On 2011/11/30 02:48:56, Jamie wrote: > What's the purpose of setting the title? Might as well keep it. Theoretically, a user might run a WM that dishonors the request to hide the title (and other window decorations). And even if the title were invisible on the window itself, it might appear elsewhere (taskbar, window-switcher, a11y tool etc). There doesn't seem to be any harm keeping it in. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:93: current_height_ = current_width_ = 0; On 2011/11/30 02:48:56, Jamie wrote: > I don't think this is necessary, as we regenerate bitmaps if the size has > changed anyway, and if it hasn't then the old bitmaps can be reused. Also, this > sort of indirect indication that the bitmap needs to be regenerated makes me > nervous--can we reset the bitmap itself instead, or is that owned by the window? Actually, we were leaking the bitmaps! I've fixed this to deref the bitmaps after handing them to the window. We should still reset stored sizes to 0 (or at least do something to cause the bitmaps to be regenerated). Unfortunately, the problem it fixes can't be reproduced any more - it only happened when Disconnect Window was shown in Me2Me mode, when you reconnected after disconnecting. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:105: // window movement gripper. On 2011/11/30 02:48:56, Jamie wrote: > Will this work for bidi languages? If not, then that can be fixed in a separate > CL. Haven't tried it. I expect GTK will display the controls (button and text-label) the other way round (button on the left). But the gripper will still be on the left. Coming to a CL near you... http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:191: GdkPixmap* shape_mask = gdk_pixmap_new(NULL, current_width_, current_height_, On 2011/11/30 02:48:56, Jamie wrote: > I assume that cairo_destroy also destroys this pixmap? It might be worth > checking to see if there's a scoped_ptr equivalent for this, which would save a > couple of calls to cairo_destroy. Not really worth creating one if not though. I'm pretty sure cairo_destroy() doesn't destroy the pixmap. There is no scoped wrapper for cairo_destroy() that I can find in Chrome. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:193: cairo_t* cr = gdk_cairo_create(shape_mask); On 2011/11/30 02:48:56, Jamie wrote: > Can this have a more descriptive name, please? Done (groan!). http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:198: // sizes just in case. On 2011/11/30 02:48:56, Jamie wrote: > What happens if we don't? If it just looks crappy then I don't think we care > (it's going to be unusable if resized too small anyway). Yeah, it just looks very slightly strange at the corners. Done. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:222: cairo_set_source_rgb(cr, 233 / 256.0, 233 / 256.0, 233 / 256.0); On 2011/11/30 02:48:56, Jamie wrote: > Seems an odd way of specifying the colour. I think the Mac version also uses > fractional RGB values, but specifies them as fractions. To mimic the values, the > Windows version multiplies them up, but you seem to be doing the opposite here. > I recommend you just copy the fractional values from the Mac source directly. Done. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:235: int gripper_bottom = current_height_ / 2 + 10.5; On 2011/11/30 02:48:56, Jamie wrote: > I don't understand this. Why is one of these a double and the other an int? And > why do the y-coordinates need to be fractional? I think the comment is intended > to explain this, but I'm still in the dark. Doh! They should both be doubles. Fixed. http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:239: cairo_set_source_rgb(cr, 194 / 256.0, 194 / 256.0, 194 / 256.0); On 2011/11/30 02:48:56, Jamie wrote: > Seems an odd way of specifying the colour. I think the Mac version also uses > fractional RGB values, but specifies them as fractions. To mimic the values, the > Windows version multiplies them up, but you seem to be doing the opposite here. Done.
lgtm http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... File remoting/host/disconnect_window_linux.cc (right): http://codereview.chromium.org/8733014/diff/1/remoting/host/disconnect_window... remoting/host/disconnect_window_linux.cc:193: cairo_t* cr = gdk_cairo_create(shape_mask); On 2011/12/02 01:41:55, Lambros wrote: > On 2011/11/30 02:48:56, Jamie wrote: > > Can this have a more descriptive name, please? > > Done (groan!). I think |context| would be sufficient if you think it's too long now. I just didn't know what cr was supposed to stand for, so wasn't sure if this was just an opaque context or something else.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/8733014/4001
Change committed as 112831 |
