Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 8604262
or point your web browser to
http://mondrian/8604262
to review the following code:
Change 8604262 by [EMAIL PROTECTED] on 2008/10/15 13:59:37 *pending*
Simplifies method used in dialogs to hide buttons on WinCE Smartphone.
Also modifies shortcuts dialog to use a link for 'never allow' on WinCE
Pocket PC, to match all other platforms.
R=andreip
[EMAIL PROTECTED]
DELTA=173 (43 added, 106 deleted, 24 changed)
OCL=8604262
Affected files ...
... //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.css#5 edit
... //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.js#11 edit
...
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4#3
edit
...
//depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4#2
edit
...
//depot/googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4#3
edit
173 delta lines: 43 added, 106 deleted, 24 changed
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 8604262 by [EMAIL PROTECTED] on 2008/10/15 13:59:37 *pending*
Simplifies method used in dialogs to hide buttons on WinCE Smartphone.
Also modifies shortcuts dialog to use a link for 'never allow' on WinCE
Pocket PC, to match all other platforms.
Affected files ...
... //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.css#5 edit
... //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.js#11 edit
...
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4#3
edit
...
//depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4#2
edit
...
//depot/googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4#3
edit
==== //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.css#5 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/ui/common/html_dialog.css
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/html_dialog.css
2008-09-17 13:14:21.000000000 +0100
+++ googleclient/gears/opensource/gears/ui/common/html_dialog.css
2008-10-15 14:35:12.000000000 +0100
@@ -96,6 +96,14 @@
margin:1em;
}
+#buttons-wince {
+ /*
+ On Windows Mobile, we hide the div containing the buttons by default, as
+ they are only used on touchscreen devices.
+ */
+ display:none;
+}
+
.accesskey {
text-decoration: underline;
==== //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.js#11 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/ui/common/html_dialog.js
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/html_dialog.js
2008-10-15 14:57:05.000000000 +0100
+++ googleclient/gears/opensource/gears/ui/common/html_dialog.js
2008-10-15 13:42:22.000000000 +0100
@@ -30,14 +30,11 @@
if (!browser.ie_mobile) {
dom.addEvent(document, "keyup", handleKeyUp);
} else {
- var buttonRowElem = null;
- if (window.pie_dialog.IsSmartPhone()) {
- buttonRowElem = dom.getElementById("button-row-smartphone");
- } else {
- buttonRowElem = dom.getElementById("button-row");
- }
- if (buttonRowElem) {
- buttonRowElem.style.display = 'block';
+ if (!window.pie_dialog.IsSmartPhone()) {
+ var buttonRowElem = dom.getElementById("buttons-wince");
+ if (buttonRowElem) {
+ buttonRowElem.style.display = 'block';
+ }
}
}
if (browser.ie_mobile) {
====
//depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4#3
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4
2008-09-17 13:14:21.000000000 +0100
+++ googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4
2008-10-15 14:50:37.000000000 +0100
@@ -44,32 +44,9 @@
padding-right:1em;
}
-m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- /*
- * On Windows Mobile, we hide the div containing the buttons
- * by default, to only show the correct one (ie smartphone or not)
- */
-
- #button-row {
- display:none;
- }
-
- #button-row-smartphone {
- margin-left:2em;
- display:none;
- }
-~,~~)
-
#checkbox-row {
margin-top:1em;
-m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- margin-bottom:0.5em;
-~,
-~
margin-bottom:1em;
-~)
}
.header-icon {
@@ -240,7 +217,7 @@
<div id="string-location-privacy-statement" style="display:none;
padding-top:3px;"></div>
- <p id="checkbox-row" style="display:none">
+ <div id="checkbox-row" style="display:none">
<table width="100%" cellspacing="0" cellpadding="0" border="0">
<tr>
<td valign="middle">
@@ -254,31 +231,11 @@
</td>
</tr>
</table>
- </p>
+ </div>
m4_ifelse(PRODUCT_OS,~wince~,~~,m4_ifelse(PRODUCT_OS,~android~,~~,~<br>~))
</div>
<div id="foot">
-m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- <!--
- On Windows Mobile, we use the softkey bar in addition to HTML buttons.
- On Windows Mobile smartphone, we only display this div and hide the
- button-row containing the HTML buttons (as screen estate is expensive
- and we already have the buttons through the softkey bar)
- -->
-
- <div id="button-row-smartphone">
- <table cellpadding="0" cellspacing="0" border="0">
- <tr>
- <td valign="middle">
- <a href="#" onclick="denyAccessPermanently(); return false;">
- <span id="string-never-allow-link"></span></a>
- </td>
- </tr>
- </table>
- </div>
-~,~~)
<div id="button-row">
<table cellpadding="0" cellspacing="0" border="0">
<tr>
@@ -305,10 +262,10 @@
-->
<td width="50%" align="right" valign="middle">
- <form>
+ <div id="buttons-wince">
<input type="BUTTON" id="allow-button"
onclick="allowAccessPermanently(); return false;"></input>
<input type="BUTTON" id="deny-button"
onclick="denyAccessTemporarily(); return false;"></input>
- </form>
+ </div>
</td>
~,~
<td nowrap="true" align="right" valign="middle">
@@ -343,6 +300,19 @@
<script>
var debug = false;
+ if (debug) {
+ // Handy for debugging layout:
+ if (browser.ie_mobile) {
+ window.pie_dialog = new Object();
+ window.pie_dialog.IsSmartPhone = function() { return false; };
+ window.pie_dialog.SetCancelButton = function() {};
+ window.pie_dialog.SetButton = function() {};
+ window.pie_dialog.SetButtonEnabled = function() {};
+ window.pie_dialog.SetScriptContext = function() {};
+ window.pie_dialog.ResizeDialog = function() {};
+ }
+ }
+
initDialog();
initWarning();
====
//depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4#2
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4
2008-10-15 14:57:05.000000000 +0100
+++ googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4
2008-10-15 14:30:53.000000000 +0100
@@ -121,17 +121,6 @@
.app-origin {
width: 70px;
}
-
-m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- /*
- * On Windows Mobile, we hide the div containing the buttons
- * by default, to only show the correct one (ie smartphone or not)
- */
- #button-row {
- display:none;
- }
-~,~~)
td.origin-name {
font-weight: bold;
@@ -212,17 +201,19 @@
<table width="100%" cellpadding="0" cellspacing="0" border="0">
<tr>
m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- <div id="div-buttons">
- <td width="50%" valign="middle">
- <input type="BUTTON" id="cancel-button"
- onclick="saveAndClose(null); return false;"></input>
- </td>
- <td width="50%" align="right" valign="middle">
- <input type="BUTTON" id="confirm-button"
- onclick="saveAndClose(result); return false;"></input>
- </td>
- </div>
+~ <!--
+ We use form input buttons instead of button elements as PIE
+ does not support them.
+ -->
+
+ <td width="50%" valign="middle">
+ <input type="BUTTON" id="cancel-button"
+ onclick="saveAndClose(null); return false;"></input>
+ </td>
+ <td width="50%" align="right" valign="middle">
+ <input type="BUTTON" id="confirm-button"
+ onclick="saveAndClose(result); return false;"></input>
+ </td>
~,m4_dnl
~
<td nowrap="true" align="right" valign="middle">
====
//depot/googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4#3
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4
====
# action=edit type=text
--- googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4
2008-09-17 13:14:21.000000000 +0100
+++ googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4
2008-10-15 14:30:32.000000000 +0100
@@ -86,21 +86,6 @@
margin-top:-2px;
margin-left:0.45em;
}
-m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- /*
- * On Windows Mobile, we hide the div containing the buttons
- * by default, to only show the correct one (ie smartphone or not)
- */
-
- #button-row {
- display:none;
- }
-
- #button-row-smartphone {
- display:none;
- }
-~,~~)
</style>
</head>
<body>
@@ -191,44 +176,32 @@
</div>
<div id="foot">
-m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
-~
- <!-- On SmartPhone, we don't use the regular buttons. We just use this
link,
- and softkeys for the other buttons. -->
- <div id="button-row-smartphone">
- <table cellpadding="0" cellspacing="0" border="0">
- <tr>
- <td valign="middle">
- <a href="#" id="deny-permanently-link"
onclick="denyShortcutPermanently(); return false;"></a>
- </td>
- </tr>
- </table>
- </div>
-~,~~)
-
<div id="button-row">
<table cellpadding="0" cellspacing="0" border="0">
<tr>
+m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
+~
+ <td width="50%" valign="middle">
+~,~
+ <td width="100%" valign="middle">
+~)
+ <a href="#" onclick="denyAccessPermanently(); return false;"
id="deny-permanently-link"></a>
+ </td>
+
+
m4_ifelse(PRODUCT_OS,~wince~,m4_dnl
~ <!--
We use form input buttons instead of buttons elements as PIE
does not support them.
-->
- <td width="50%" valign="middle">
- <input type="BUTTON" id="deny-permanently-button"
onclick="denyShortcutPermanently(); return false;"></input>
- </td>
-
- <div id="div-buttons">
- <td width="50%" align="right" valign="middle">
+ <td width="50%" align="right" valign="middle">
+ <div id="buttons-wince">
<input type="BUTTON" id="allow-button"
onclick="allowShortcutsTemporarily(); return false;"></input>
<input type="BUTTON" id="deny-button"
onclick="denyShortcutsTemporarily(); return false;"></input>
- </td>
- </div>
+ </div>
+ </td>
~,~
- <td width="100%" valign="middle">
- <a href="#" onclick="denyShortcutPermanently(); return false;"
id="deny-permanently-link"></a>
- </td>
<td nowrap="true" align="right" valign="middle">
<button id="allow-button" class="custom"
onclick="allowShortcutsTemporarily(); return false;"></button
@@ -345,18 +318,6 @@
// For PIE, we don't set a window title.
setElementContents("string-header-wince", "header");
- if (window.pie_dialog.IsSmartPhone()) {
- // On softkey-only devices we only use a regular link to trigger
- // the "never-allow" action. For "allow" and "deny" we use only
- // the softkey lables.
- setElementContents("string-never-allow-wince", "deny-permanently-link");
- } else {
- // For touchscreen devices, we use buttons for all actions. Additionally,
- // we also set the sofkey labels.
- setButtonLabel("string-ok", "allow-button", "string-ok-accesskey");
- setButtonLabel("string-cancel", "deny-button",
"string-cancel-accesskey");
- setButtonLabel("string-never-allow-wince", "deny-permanently-button");
- }
// Set the softkey labels for both softkey-only and touchscreen UIs.
window.pie_dialog.SetButton(dom.getElementById("string-ok").innerText,
"allowShortcutsTemporarily();");
@@ -364,6 +325,16 @@
// We need to call enableButton() on WinCE because the softkey is disabled
// by default.
enableButton(dom.getElementById("allow-button"));
+
+ // On all devices we use a regular link to trigger the "never-allow"
+ // action.
+ setElementContents("string-never-allow-wince", "deny-permanently-link");
+
+ if (!window.pie_dialog.IsSmartPhone()) {
+ // For touchscreen devices, we also use buttons for "allow" and "deny".
+ setButtonLabel("string-ok", "allow-button", "string-ok-accesskey");
+ setButtonLabel("string-cancel", "deny-button",
"string-cancel-accesskey");
+ }
}
function initDefaultStyle() {
