Received: by 10.223.132.216 with SMTP id c24mr465605fat.2.1274686036933; Mon, 24 May 2010 00:27:16 -0700 (PDT) X-BeenThere: google-gadgets-for-linux-dev@googlegroups.com Received: by 10.204.32.206 with SMTP id e14ls649902bkd.2.p; Mon, 24 May 2010 00:27:16 -0700 (PDT) Received: by 10.204.137.151 with SMTP id w23mr191053bkt.20.1274686035858; Mon, 24 May 2010 00:27:15 -0700 (PDT) Received: by 10.204.137.151 with SMTP id w23mr191052bkt.20.1274686035822; Mon, 24 May 2010 00:27:15 -0700 (PDT) Return-Path: Received: from mail-qy0-f201.google.com (mail-qy0-f201.google.com [209.85.221.201]) by gmr-mx.google.com with ESMTP id v26si2644380bkt.3.2010.05.24.00.27.14; Mon, 24 May 2010 00:27:14 -0700 (PDT) Received-SPF: pass (google.com: domain of james...@gmail.com designates 209.85.221.201 as permitted sender) client-ip=209.85.221.201; Received: by mail-qy0-f201.google.com with SMTP id 39so5409598qyk.8 for ; Mon, 24 May 2010 00:27:14 -0700 (PDT) Received: by 10.224.115.27 with SMTP id g27mr2803375qaq.311.1274686034114; Mon, 24 May 2010 00:27:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.241.16 with HTTP; Mon, 24 May 2010 00:26:54 -0700 (PDT) In-Reply-To: <20100524070856.31FD9240FC1@wxz.bej.corp.google.com> References: <20100524070856.31FD9240FC1@wxz.bej.corp.google.com> From: James Su Date: Mon, 24 May 2010 00:26:54 -0700 Message-ID: Subject: [ggl-dev] Re: A lice-sized code review (wangxianzhu localrev 1321) To: phnix...@gmail.com Cc: google-gadgets-for-linux-dev@googlegroups.com X-Original-Authentication-Results: gmr-mx.google.com; spf=pass (google.com: domain of james...@gmail.com designates 209.85.221.201 as permitted sender) smtp.mail=james...@gmail.com; dkim=pass (test mode) header...@gmail.com X-Original-Sender: james...@gmail.com Reply-To: google-gadgets-for-linux-dev@googlegroups.com Precedence: list Mailing-list: list google-gadgets-for-linux-dev@googlegroups.com; contact google-gadgets-for-linux-dev+owners@googlegroups.com List-ID: List-Post: , List-Help: , List-Archive: Sender: google-gadgets-for-linux-dev@googlegroups.com List-Unsubscribe: , Content-Type: multipart/alternative; boundary=00c09f9b09f5437c28048751f84e --00c09f9b09f5437c28048751f84e Content-Type: text/plain; charset=UTF-8 LGTM 2010/5/24 > Hello james.su, > > I'd like you to do a code review. Please review the following patch: > > ---------------------------------------------------------------------- > r1321: (no author) | 2010-05-24 15:07:39 +0800 > > 1. Avoid unexpected scrolling to top if ClearSelection() is called after > SetSelectedItem() in the same loop; > 2. Fix some build issues > ---------------------------------------------------------------------- > > === extensions/soup_xml_http_request/soup_xml_http_request.cc > ================================================================== > --- extensions/soup_xml_http_request/soup_xml_http_request.cc (revision > 1320) > +++ extensions/soup_xml_http_request/soup_xml_http_request.cc (revision > 1321) > @@ -857,6 +857,9 @@ > > static void MessageCompleteCallback(SoupSession *session, SoupMessage > *msg, > gpointer user_data) { > + GGL_UNUSED(session); > + GGL_UNUSED(msg); > + > XMLHttpRequest *request = static_cast(user_data); > #ifdef SOUP_XHR_VERBOSE > PrintMessageInfo(request, "MessageCompleteCallback", msg, NULL); > @@ -1073,6 +1076,11 @@ > static void LoggerPrinter(SoupLogger *logger, SoupLoggerLogLevel level, > char direction, const char *data, > gpointer user_data) { > + GGL_UNUSED(logger); > + GGL_UNUSED(level); > + GGL_UNUSED(direction); > + GGL_UNUSED(data); > + GGL_UNUSED(user_data); > DLOG("%c %s\n", direction, data); > } > #endif > @@ -1082,6 +1090,9 @@ > SoupAuth *auth, > gboolean retrying, > gpointer user_data) { > + GGL_UNUSED(session); > + GGL_UNUSED(user_data); > + > XMLHttpRequest *request = static_cast( > g_object_get_data(G_OBJECT(msg), kSoupMessageXHRKey)); > ASSERT(request); > @@ -1101,6 +1112,10 @@ > SoupMessage *msg, > SoupSocket *socket, > gpointer user_data) { > + GGL_UNUSED(session); > + GGL_UNUSED(socket); > + GGL_UNUSED(user_data); > + > XMLHttpRequest *request = static_cast( > g_object_get_data(G_OBJECT(msg), kSoupMessageXHRKey)); > #ifdef SOUP_XHR_VERBOSE > === extensions/soup_xml_http_request/tests/CMakeLists.txt > ================================================================== > --- extensions/soup_xml_http_request/tests/CMakeLists.txt (revision > 1320) > +++ extensions/soup_xml_http_request/tests/CMakeLists.txt (revision > 1321) > @@ -17,12 +17,14 @@ > IF(GGL_BUILD_SOUP_XML_HTTP_REQUEST) > > APPLY_CONFIG(GTK2) > +APPLY_CONFIG(PTHREAD) > > ADD_TEST_EXECUTABLE(soup_xml_http_request_test > soup_xml_http_request_test.cc > ) > TARGET_LINK_LIBRARIES(soup_xml_http_request_test > ${GTK2_LIBRARIES} > + ${PTHREAD_LIBRARIES} > ggadget${GGL_EPOCH} > ggadget-gtk${GGL_EPOCH} > gtest > === hosts/gtk/standalone_gtk_host.cc > ================================================================== > --- hosts/gtk/standalone_gtk_host.cc (revision 1320) > +++ hosts/gtk/standalone_gtk_host.cc (revision 1321) > @@ -227,6 +227,7 @@ > } > > void RemoveGadget(Gadget *gadget, bool save_data) { > + GGL_UNUSED(gadget); > GGL_UNUSED(save_data); > ASSERT(gadget && gadget == gadget_); > owner_->Exit(); > === ggadget/listbox_element.cc > ================================================================== > --- ggadget/listbox_element.cc (revision 1320) > +++ ggadget/listbox_element.cc (revision 1321) > @@ -136,6 +136,10 @@ > if (item->IsSelected()) { > result = true; > item->SetSelected(false); > + > + // Clear pending scroll flag to avoid unexpected scrolling to > + // the top of the list. > + pending_scroll_ = 0; > } > } else { > LOG(kErrorItemExpected); > === ggadget/gtk/view_widget_binder.cc > ================================================================== > --- ggadget/gtk/view_widget_binder.cc (revision 1320) > +++ ggadget/gtk/view_widget_binder.cc (revision 1321) > @@ -827,7 +827,10 @@ > guint info, guint time, > gpointer user_data) { > GGL_UNUSED(widget); > + GGL_UNUSED(x); > + GGL_UNUSED(y); > GGL_UNUSED(info); > + > Impl *impl = reinterpret_cast(user_data); > if (!impl->current_drag_event_) { > // There are some cases that multiple drag events are fired in one > event > > This is a semiautomated message from "svkmail". Complaints or suggestions? > Mail edy...@gmail.com. > --00c09f9b09f5437c28048751f84e Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable LGTM

2010/5/24 <phnix...@gmail.com>
Hello james.su,

I'd like you to do a code review. Please review the following patch:
----------------------------------------------------------------------
r1321: =C2=A0(no author) | 2010-05-24 15:07:39 +0800

1. Avoid unexpected scrolling to top if ClearSelection() is called after Se= tSelectedItem() in the same loop;
2. Fix some build issues
----------------------------------------------------------------------

=3D=3D=3D extensions/soup_xml_http_request/soup_xml_http_request.cc
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- extensions/soup_xml_http_request/soup_xml_http_request.cc =C2=A0 (revis= ion 1320)
+++ extensions/soup_xml_http_request/soup_xml_http_request.cc =C2=A0 (revis= ion 1321)
@@ -857,6 +857,9 @@

=C2=A0 static void MessageCompleteCallback(SoupSession *session, SoupMessa= ge *msg,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gpointer user_d= ata) {
+ =C2=A0 =C2=A0GGL_UNUSED(session);
+ =C2=A0 =C2=A0GGL_UNUSED(msg);
+
=C2=A0 =C2=A0 XMLHttpRequest *request =3D static_cast<XMLHttpRequest *&= gt;(user_data);
=C2=A0#ifdef SOUP_XHR_VERBOSE
=C2=A0 =C2=A0 PrintMessageInfo(request, "MessageCompleteCallback"= ;, msg, NULL);
@@ -1073,6 +1076,11 @@
=C2=A0 static void LoggerPrinter(SoupLogger *logger, SoupLoggerLogLevel le= vel,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 char direction, const char *data,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 gpointer user_data) {
+ =C2=A0 =C2=A0GGL_UNUSED(logger);
+ =C2=A0 =C2=A0GGL_UNUSED(level);
+ =C2=A0 =C2=A0GGL_UNUSED(direction);
+ =C2=A0 =C2=A0GGL_UNUSED(data);
+ =C2=A0 =C2=A0GGL_UNUSED(user_data);
=C2=A0 =C2=A0 DLOG("%c %s\n", direction, data);
=C2=A0 }
=C2=A0#endif
@@ -1082,6 +1090,9 @@
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SoupAuth *auth,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gboolean retrying,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpointer user_data) { + =C2=A0 =C2=A0GGL_UNUSED(session);
+ =C2=A0 =C2=A0GGL_UNUSED(user_data);
+
=C2=A0 =C2=A0 XMLHttpRequest *request =3D static_cast<XMLHttpRequest *&= gt;(
=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_object_get_data(G_OBJECT(msg), kSoupMessageX= HRKey));
=C2=A0 =C2=A0 ASSERT(request);
@@ -1101,6 +1112,10 @@
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SoupMessage *msg= ,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SoupSocket *sock= et,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpointer user_da= ta) {
+ =C2=A0 =C2=A0GGL_UNUSED(session);
+ =C2=A0 =C2=A0GGL_UNUSED(socket);
+ =C2=A0 =C2=A0GGL_UNUSED(user_data);
+
=C2=A0 =C2=A0 XMLHttpRequest *request =3D static_cast<XMLHttpRequest *&= gt;(
=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_object_get_data(G_OBJECT(msg), kSoupMessageX= HRKey));
=C2=A0#ifdef SOUP_XHR_VERBOSE
=3D=3D=3D extensions/soup_xml_http_request/tests/CMakeLists.txt
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- extensions/soup_xml_http_request/tests/CMakeLists.txt =C2=A0 =C2=A0 =C2= =A0 (revision 1320)
+++ extensions/soup_xml_http_request/tests/CMakeLists.txt =C2=A0 =C2=A0 =C2= =A0 (revision 1321)
@@ -17,12 +17,14 @@
=C2=A0IF(GGL_BUILD_SOUP_XML_HTTP_REQUEST)

=C2=A0APPLY_CONFIG(GTK2)
+APPLY_CONFIG(PTHREAD)

=C2=A0ADD_TEST_EXECUTABLE(soup_xml_http_request_test
=C2=A0 soup_xml_http_request_test.cc
=C2=A0)
=C2=A0TARGET_LINK_LIBRARIES(soup_xml_http_request_test
=C2=A0 ${GTK2_LIBRARIES}
+ =C2=A0${PTHREAD_LIBRARIES}
=C2=A0 ggadget${GGL_EPOCH}
=C2=A0 ggadget-gtk${GGL_EPOCH}
=C2=A0 gtest
=3D=3D=3D hosts/gtk/standalone_gtk_host.cc
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- hosts/gtk/standalone_gtk_host.cc =C2=A0 =C2=A0(revision 1320)
+++ hosts/gtk/standalone_gtk_host.cc =C2=A0 =C2=A0(revision 1321)
@@ -227,6 +227,7 @@
=C2=A0 }

=C2=A0 void RemoveGadget(Gadget *gadget, bool save_data) {
+ =C2=A0 =C2=A0GGL_UNUSED(gadget);
=C2=A0 =C2=A0 GGL_UNUSED(save_data);
=C2=A0 =C2=A0 ASSERT(gadget && gadget =3D=3D gadget_);
=C2=A0 =C2=A0 owner_->Exit();
=3D=3D=3D ggadget/listbox_element.cc
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- ggadget/listbox_element.cc =C2=A0(revision 1320)
+++ ggadget/listbox_element.cc =C2=A0(revision 1321)
@@ -136,6 +136,10 @@
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (item->IsSelected()) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 result =3D true;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 item->SetSelected(false);
+
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// Clear pending scroll flag to = avoid unexpected scrolling to
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// the top of the list.
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pending_scroll_ =3D 0;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOG(kErrorItemExpected);
=3D=3D=3D ggadget/gtk/view_widget_binder.cc
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- ggadget/gtk/view_widget_binder.cc =C2=A0 (revision 1320)
+++ ggadget/gtk/view_widget_binder.cc =C2=A0 (revision 1321)
@@ -827,7 +827,10 @@
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 guint info, gui= nt time,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gpointer user_d= ata) {
=C2=A0 =C2=A0 GGL_UNUSED(widget);
+ =C2=A0 =C2=A0GGL_UNUSED(x);
+ =C2=A0 =C2=A0GGL_UNUSED(y);
=C2=A0 =C2=A0 GGL_UNUSED(info);
+
=C2=A0 =C2=A0 Impl *impl =3D reinterpret_cast<Impl *>(user_data); =C2=A0 =C2=A0 if (!impl->current_drag_event_) {
=C2=A0 =C2=A0 =C2=A0 // There are some cases that multiple drag events are= fired in one event

This is a semiautomated message from "svkmail". =C2=A0Complaints = or suggestions?
Mail edy...@gmail.com.