A cockroach-sized code review (suzhe localrev 812)

[jame...@gmail.com]

Hello zhuang.dev, edyfox, phnixwxz,

I'd like you to do a code review. Please review the following patch:

----------------------------------------------------------------------
r812: suzhe | 2008-07-09 19:34:16 +0800

- Revert unexpected changes to run once code.
- Improve options dialog of rss gadget.
- Fix size issue of combobox element.
----------------------------------------------------------------------

=== hosts/qt/main.cc
==================================================================
--- hosts/qt/main.cc (revision 811)
+++ hosts/qt/main.cc (revision 812)
@@ -55,7 +55,7 @@
#endif

static ggadget::qt::QtMainLoop *g_main_loop;
-static const char kRunOnceSocketName[] = "qt-host-socket";
+static const char kRunOnceSocketName[] = "ggl-host-socket";

static const char *kGlobalExtensions[] = {
"default-framework",
@@ -236,7 +236,7 @@
}
#endif

-static void Handler(const std::string &data) {
+static void OnClientMessage(const std::string &data) {
ggadget::GetGadgetManager()->NewGadgetInstanceFromFile(data.c_str());
}

@@ -278,7 +278,7 @@
ggadget::BuildFilePath(profile_dir.c_str(),
kRunOnceSocketName,
NULL).c_str());
- run_once.ConnectOnMessage(ggadget::NewSlot(Handler));
+ run_once.ConnectOnMessage(ggadget::NewSlot(OnClientMessage));

ggadget::ConnectGlobalLogListener(ggadget::NewSlot(LogListener));

=== hosts/gtk/main.cc
==================================================================
--- hosts/gtk/main.cc (revision 811)
+++ hosts/gtk/main.cc (revision 812)
@@ -53,7 +53,7 @@
static ggadget::gtk::MainLoop g_main_loop;

static const char kOptionsName[] = "gtk-host-options";
-static const char kRunOnceSocketName[] = "gtk-host-socket";
+static const char kRunOnceSocketName[] = "ggl-host-socket";

static const char *kGlobalExtensions[] = {
// default framework must be loaded first, so that the default properties can
@@ -192,7 +192,7 @@
" If any gadgets are specified, they will be installed by using\n"
" GadgetManager.\n";

-static void Handler(const std::string &data) {
+static void OnClientMessage(const std::string &data) {
ggadget::GetGadgetManager()->NewGadgetInstanceFromFile(data.c_str());
}

@@ -218,7 +218,7 @@
ggadget::BuildFilePath(profile_dir.c_str(),
kRunOnceSocketName,
NULL).c_str());
- run_once.ConnectOnMessage(ggadget::NewSlot(Handler));
+ run_once.ConnectOnMessage(ggadget::NewSlot(OnClientMessage));

// Parse command line.
std::vector<std::string> gadget_paths;
=== ggadget/run_once.h
==================================================================
--- ggadget/run_once.h (revision 811)
+++ ggadget/run_once.h (revision 812)
@@ -31,7 +31,8 @@
* object destructs. So don't create it on stack. Please always use operator
* @c new to create the instance of this object and don't delete it manually.
*
- * @param path the socket path of the application.
+ * @param path the UNIX domain socket for the application. The caller has to
+ * ensure that the path is valid.
*/
RunOnce(const char *path);
~RunOnce();
=== ggadget/run_once.cc
==================================================================
--- ggadget/run_once.cc (revision 811)
+++ ggadget/run_once.cc (revision 812)
@@ -31,6 +31,8 @@

namespace ggadget {

+const size_t kBufferSize = 4096;
+
class RunOnce::Impl : public WatchCallbackInterface {
public:
struct Session {
@@ -43,6 +45,7 @@
is_running_(false),
watch_id_(-1),
fd_(-1) {
+ ASSERT(path);
int fd = RunAsServer();
if (fd == -1) {
fd = RunAsClient();
@@ -81,9 +84,6 @@
return is_running_;
}

- static void DoNothing(int) {
- }
-
size_t SendMessage(const std::string &data) {
if (!is_running_)
return 0;
@@ -91,8 +91,7 @@
if (fd_ == -1)
fd_ = RunAsClient();

- void (*old_proc)(int);
- old_proc = signal(SIGPIPE, DoNothing);
+ sig_t old_proc = signal(SIGPIPE, SIG_IGN);

fd_set fds;
FD_ZERO(&fds);
@@ -108,7 +107,9 @@
if (result == -1 || result == 0) {
goto end;
}
- int current = write(fd_, &data.c_str()[written], data.size() - written);
+ ssize_t current = write(fd_,
+ &data.c_str()[written],
+ data.size() - written);
if (current < 1) {
goto end;
}
@@ -124,33 +125,30 @@
}

Connection *ConnectOnMessage(Slot1<void, const std::string&> *slot) {
- return signal_.Connect(slot);
+ return on_message_.Connect(slot);
}

virtual bool Call(MainLoopInterface *main_loop, int watch_id) {
int fd;
- char buf;
+ char buf[kBufferSize];

fd = main_loop->GetWatchData(watch_id);

if (fd_ == fd) {
socklen_t len;
fd = accept(fd, NULL, &len);
- main_loop->AddIOReadWatch(fd, this);
- Session data = {
- watch_id,
- std::string()
- };
- connections_[fd] = data;
+ connections_[fd].watch_id =
+ main_loop->AddIOReadWatch(fd, this);
return true;
}

- if (read(fd, &buf, 1) > 0) {
- connections_[fd].data += buf;
+ ssize_t size = 0;
+ if ((size = read(fd, &buf, kBufferSize)) > 0) {
+ connections_[fd].data += std::string(buf, size);
} else {
std::map<int, Session>::iterator ite = connections_.find(fd);
if (ite != connections_.end()) {
- signal_(ite->second.data);
+ on_message_(ite->second.data);
main_loop->RemoveWatch(watch_id);
connections_.erase(ite);
}
@@ -196,7 +194,7 @@
int watch_id_;
int fd_;
std::map<int, Session> connections_;
- Signal1<void, const std::string &> signal_;
+ Signal1<void, const std::string &> on_message_;
};

RunOnce::RunOnce(const char *path)
=== ggadget/combobox_element.cc
==================================================================
--- ggadget/combobox_element.cc (revision 811)
+++ ggadget/combobox_element.cc (revision 812)
@@ -129,7 +129,7 @@
}

void SetListBoxHeight() {
- double elem_height = owner_->GetPixelHeight();
+ double elem_height = owner_->BasicElement::GetPixelHeight();
double max_height = max_items_ * item_pixel_height_;
double height = std::min(max_height, elem_height - item_pixel_height_);
if (height < 0) {
@@ -726,8 +726,8 @@
}

double ComboBoxElement::GetPixelHeight() const {
- return impl_->listbox_->IsVisible() ? BasicElement::GetPixelHeight() :
- impl_->item_pixel_height_;
+ return impl_->item_pixel_height_ +
+ (impl_->listbox_->IsVisible() ? impl_->listbox_->GetPixelHeight() : 0);
}

bool ComboBoxElement::IsChildInVisibleArea(const BasicElement *child) const {
=== gadgets/rss/options.xml
==================================================================
--- gadgets/rss/options.xml (revision 811)
+++ gadgets/rss/options.xml (revision 812)
@@ -18,31 +18,25 @@
<view width="350" height="405" resizable="false"
onok="OKClicked()" onopen="OnOpen()" caption="&GADGET_OPTIONS;">
<label y="5">&GADGET_ADDLABEL;</label>
- <div width="295" height="20" y="25">
- <img stretchMiddle="true" src="element_border.png"
- width="100%" height="100%"/>
- <edit width="293" height="18" x="1" y="1" name="feedname"
+ <div width="295" height="20" y="25" background="#7f9db9">
+ <edit width="293" height="18" x="1" y="1" name="feedname"
onchange="FeedBoxChanged()"/>
</div>
- <button x="300" y="25" width="50" name="addbutton" onclick="AddFeed()"
- caption="&GADGET_ADD;" downImage="button_down.png" image="button_up.png"
- overImage="button_over.png" stretchMiddle="true"/>
- <div width="350" height="300" y="50">
- <img stretchMiddle="true" src="element_border.png"
- width="100%" height="100%"/>
- <listbox name="feeds" width="348" height="298" x="1" y="1"
- background="#ffffff" autoscroll="true" itemHeight="35"
+ <button x="300" y="25" width="50" height="20" name="addbutton"
+ onclick="AddFeed()" caption="&GADGET_ADD;" downImage="button_down.png"
+ image="button_up.png" overImage="button_over.png" stretchMiddle="true"/>
+ <div width="350" height="300" y="50" background="#7f9db9">
+ <listbox name="feeds" width="348" height="298" x="1" y="1"
+ background="#ffffff" autoscroll="true" itemHeight="35"
itemWidth="100%" onchange="SelectedFeedChanged()"/>
</div>
- <button x="250" y="355" width="100" name="removebutton"
- onclick="RemoveFeed()" caption="&GADGET_REMOVE;"
+ <button x="250" y="355" width="100" height="20" name="removebutton"
+ onclick="RemoveFeed()" caption="&GADGET_REMOVE;"
downImage="button_down.png" image="button_up.png"
overImage="button_over.png" stretchMiddle="true"/>
<label y="380">&GADGET_MAXSHOW;</label>
- <div width="30" height="20" x="320" y="380">
- <img stretchMiddle="true" src="element_border.png"
- width="100%" height="100%"/>
- <edit x="1" y="1" width="28" height="18"
+ <div width="30" height="20" x="320" y="380" background="#7f9db9">
+ <edit x="1" y="1" width="28" height="18"
align="right" name="maxitems"/>
</div>
<script src="shared.js"/>
=== gadgets/rss/element_border.png
==================================================================
Cannot display: file marked as a binary type.

Property changes on: gadgets/rss/element_border.png
___________________________________________________________________
Name: svn:executable
-*
Name: svn:mime-type
-image/png

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

[Jim Zhuang]

looks good for the RSS gadget part

[Xianzhu Wang]

LGTM.

2008/7/9 Jim Zhuang <zhuan...@gmail.com>:

[Edward L. Fox]

Looks good.

On Wed, Jul 9, 2008 at 7:34 PM, <jame...@gmail.com> wrote: