Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9414634
or point your web browser to
http://mondrian/9414634
to review the following code:
Change 9414634 by stevebl...@steveblock-gears2 on 2008/12/15 16:27:49 *pending*
Fixes a bug where collision detection strings are not available on
WinCE. This is because LoadString on winCE does not support multiple languages.
- Manually provides the collision detection strings, rather than
loading from the string_table.
- Removes string_table from the build.
R=andreip
[email protected]
DELTA=115 (72 added, 38 deleted, 5 changed)
OCL=9414634
Affected files ...
... //depot/googleclient/gears/opensource/gears/Makefile#220 edit
...
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision.h#1
edit
...
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision_osx.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision_win32.cc#3
edit
... //depot/googleclient/gears/opensource/gears/factory/factory_impl.cc#20 edit
... //depot/googleclient/gears/opensource/gears/ui/ie/tools_menu_item.cc#5 edit
115 delta lines: 72 added, 38 deleted, 5 changed
Also consider running:
g4 lint -c 9414634
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 9414634 by stevebl...@steveblock-gears2 on 2008/12/15 16:27:49 *pending*
Fixes a bug where collision detection strings are not available on
WinCE. This is because LoadString on winCE does not support multiple languages.
- Manually provides the collision detection strings, rather than
loading from the string_table.
- Removes string_table from the build.
Affected files ...
... //depot/googleclient/gears/opensource/gears/Makefile#220 edit
...
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision.h#1
edit
...
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision_osx.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision_win32.cc#3
edit
... //depot/googleclient/gears/opensource/gears/factory/factory_impl.cc#20 edit
... //depot/googleclient/gears/opensource/gears/ui/ie/tools_menu_item.cc#5 edit
==== //depot/googleclient/gears/opensource/gears/Makefile#220 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/Makefile ====
# action=edit type=text
--- googleclient/gears/opensource/gears/Makefile 2008-12-12
14:22:25.000000000 +0000
+++ googleclient/gears/opensource/gears/Makefile 2008-12-15
16:24:32.000000000 +0000
@@ -1930,9 +1930,14 @@
ui/generated \
$(NULL)
+ifeq ($(OS),wince)
+# The string table uses multiple languages, which are not supported by
+# LoadString on WinCE.
+else
IE_LINK_EXTRAS += \
$(IE_OUTDIR)/string_table.res \
$(NULL)
+endif
VISTA_BROKER_LINK_EXTRAS += \
$(VISTA_BROKER_OUTDIR)/string_table.res \
@@ -2035,9 +2040,14 @@
$(IE_OUTDIR)/ui_resources.res \
$(NULL)
+ifeq ($(OS),wince)
+# The string table uses multiple languages, which are not supported by
+# LoadString on WinCE.
+else
IE_STABSRCS += \
string_table.rc.stab \
$(NULL)
+endif
# Additional files specific to Win32 or WinCE.
ifeq ($(OS),win32)
====
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision.h#1
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/detect_version_collision.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/detect_version_collision.h
2008-12-15 16:29:56.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/detect_version_collision.h
2008-12-15 16:17:29.000000000 +0000
@@ -67,6 +67,8 @@
#include <ctype.h>
+#include "gears/base/common/string16.h"
+
// Returns true if we detected a different version of Gears was running
// at startup. If a collision is detected, this instance of Gears will
// be crippled to prevent data corruption and general mayhem.
@@ -79,4 +81,8 @@
// Puts up a simple message box alerting the user about the problem
void NotifyUserOfVersionCollision();
+// Gets the string that describes the collision. Returns NULL if an error
+// occured getting the string.
+const char16 *GetVersionCollisionErrorString();
+
#endif // GEARS_BASE_COMMON_DETECT_VERSION_COLLISION_H__
====
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision_osx.cc#1
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/detect_version_collision_osx.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/base/common/detect_version_collision_osx.cc
2008-12-15 16:29:57.000000000 +0000
+++
googleclient/gears/opensource/gears/base/common/detect_version_collision_osx.cc
2008-12-15 15:59:17.000000000 +0000
@@ -226,10 +226,16 @@
// TODO(playmobil): Load from internationalized string table.
const char16 *kTitle = STRING16(L"Please restart your browser");
- const char16 *kMessage = STRING16(L"A " PRODUCT_FRIENDLY_NAME
- L" update has been downloaded.\n"
- L"\n"
- L"Please close all browser windows"
- L" to complete the upgrade process.\n");
+ const char16 *kMessage = GetVersionCollisionErrorString();
MessageBox(kTitle, kMessage);
}
+
+const char16 *GetVersionCollisionErrorString() {
+ // TODO(playmobil): Internationalize string.
+ static const char16 *error_text = STRING16(L"A " PRODUCT_FRIENDLY_NAME
+ L" update has been downloaded.\n"
+ L"\n"
+ L"Please close all browser windows"
+ L" to complete the upgrade process.\n");
+ return error_text;
+}
====
//depot/googleclient/gears/opensource/gears/base/common/detect_version_collision_win32.cc#3
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/detect_version_collision_win32.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/base/common/detect_version_collision_win32.cc
2008-12-15 16:29:57.000000000 +0000
+++
googleclient/gears/opensource/gears/base/common/detect_version_collision_win32.cc
2008-12-15 16:17:34.000000000 +0000
@@ -33,6 +33,12 @@
#include "gears/base/common/process_utils_win32.h"
#include "gears/ui/ie/string_table.h"
#include "genfiles/product_constants.h"
+
+// Gets the string for the title of the dialog. Returns NULL if an error
+// occured getting the string.
+static const char16 *GetVersionCollisionErrorTitle();
+
+static const int kMaxStringLength = 256;
// We run our detection code once at startup
static bool OneTimeDetectVersionCollision();
@@ -150,13 +156,47 @@
void NotifyUserOfVersionCollision() {
assert(detected_collision);
alerted_user = true;
- const int kMaxStringLength = 256;
- char16 title[kMaxStringLength];
- char16 text[kMaxStringLength];
- if (LoadString(GetGearsModuleHandle(),
- IDS_VERSION_COLLISION_TITLE, title, kMaxStringLength) &&
- LoadString(GetGearsModuleHandle(),
- IDS_VERSION_COLLISION_TEXT, text, kMaxStringLength)) {
+ const char16 *title = GetVersionCollisionErrorTitle();
+ const char16 *text = GetVersionCollisionErrorString();
+ if (title && text) {
MessageBox(NULL, text, title, MB_OK);
}
}
+
+const char16 *GetVersionCollisionErrorString() {
+#ifdef OS_WINCE
+ // On WinCE, LoadString does not support multiple languages.
+ // TODO(steveblock): Internationalize string.
+ static const char16 *error_text = STRING16(L"A " PRODUCT_FRIENDLY_NAME
+ L" update has been downloaded.\n"
+ L"\n"
+ L"Please restart your browser"
+ L" to complete the upgrade process.\n");
+ return error_text;
+#else
+ // TODO(playmobil): This code isn't threadsafe, refactor.
+ static char16 error_text[kMaxStringLength];
+
+ if (!LoadString(GetGearsModuleHandle(), IDS_VERSION_COLLISION_TEXT,
+ error_text, kMaxStringLength)) {
+ return NULL;
+ }
+ return error_text;
+#endif
+}
+
+static const char16 *GetVersionCollisionErrorTitle() {
+#ifdef OS_WINCE
+ // On WinCE, LoadString does not support multiple languages.
+ // TODO(steveblock): Internationalize string.
+ static const char16 *title = STRING16(L"Please restart your browser");
+ return title;
+#else
+ static char16 title[kMaxStringLength];
+ if (!LoadString(GetGearsModuleHandle(), IDS_VERSION_COLLISION_TITLE,
+ title, kMaxStringLength)) {
+ return NULL;
+ }
+ return title;
+#endif
+}
==== //depot/googleclient/gears/opensource/gears/factory/factory_impl.cc#20 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/factory/factory_impl.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/factory/factory_impl.cc 2008-12-15
16:29:58.000000000 +0000
+++ googleclient/gears/opensource/gears/factory/factory_impl.cc 2008-12-15
16:29:06.000000000 +0000
@@ -106,34 +106,6 @@
SetActiveUserFlag();
}
-#if BROWSER_FF
- // TODO(nigeltao): implement version collision UI for Firefox.
-#elif defined(WIN32) || defined(BROWSER_WEBKIT)
-// Returns NULL if an error occured loading the string.
-static const char16 *GetVersionCollisionErrorString() {
-#if defined(WIN32)
- // TODO(playmobil): This code isn't threadsafe, refactor.
- const int kMaxStringLength = 256;
- static char16 error_text[kMaxStringLength];
- if (!LoadString(GetGearsModuleHandle(), IDS_VERSION_COLLISION_TEXT,
- error_text, kMaxStringLength)) {
- return NULL;
- }
- return error_text;
-#elif defined(BROWSER_WEBKIT)
- //TODO(playmobil): Internationalize string.
- static const char16 *error_text = STRING16(L"A " PRODUCT_FRIENDLY_NAME
- L" update has been downloaded.\n"
- L"\n"
- L"Please close all browser windows"
- L" to complete the upgrade process.\n");
- return error_text;
-#else
- return NULL;
-#endif
-}
-#endif
-
void GearsFactoryImpl::Create(JsCallContext *context) {
#if BROWSER_FF
// TODO(nigeltao): implement version collision UI for Firefox.
==== //depot/googleclient/gears/opensource/gears/ui/ie/tools_menu_item.cc#5 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/ui/ie/tools_menu_item.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/ie/tools_menu_item.cc
2008-12-15 16:29:58.000000000 +0000
+++ googleclient/gears/opensource/gears/ui/ie/tools_menu_item.cc
2008-12-15 16:21:51.000000000 +0000
@@ -41,10 +41,10 @@
command_first_ = id_cmd_first;
+ // On WinCE, LoadString does not support multiple languages, so we can't load
+ // IDS_REGISTRY_MENU_TEXT.
InsertMenu(hmenu, index_menu, MF_BYPOSITION, command_first_,
- L"Gears Settings"); // TODO(andreip): [naming] looks like the i18n
- // strings are not built into the gears dll for
WinCE
- // so loading IDS_REGISTRY_MENU_TEXT does not work.
+ L"Gears Settings"); // [naming]
return MAKE_HRESULT(SEVERITY_SUCCESS, FACILITY_NULL, 1);
}
