Hi,
> Hi Adi,
> 
> This is a good improvement. I ran into an annoying problem running the tests.
> The tests use the combined javascript file, but "make jsbuild" was not getting
> run automatically. I also noticed that translationmessage-translate.pt and
> pofile-translate.pt load pofile.js directly, when the <script> tag really
> should be added to lib/lp/app/templates/base-layout-macros.pt next to all
> the other js files in the ${lp_js} directory.

I have moved the pofile.js inclusing in base-layout-macros.pt.

> Since I was confused by some of your changes that I don't think were
> necessary, I'm marking this
>     needs-fixing

Thanks for the review

 
[snip]

> >   var notice = Y.one('.important-notice-container');
> >+  if (!notice) {
> 
> 
> Since Y.one() is expected to return null when the element is not found,
> the preferred if-statement is:
>     if (notice !== null)
Thanks for this remark.

I have changed the if statement to explicitly test agains null.

Same for the other cases.

[snip]
> >+      LPS.use('lp.pofile', function(Y) {
> >           Y.on('domready', Y.lp.pofile.setupSuggestionDismissal);
> >-          Y.on('domready', Y.lp.pofile.updateNotificationBox);
> >+      });
> >+
> >+      LPS.use('lp.pofile', function(Y) {
> 
> 
> Why are you calling LPS.use('lp.pofile', ...) repeatedly? This
> should have no effect unless there is something seriously wrong with
> lp.pofile.
I moved them back in the same sandbox.
I split independent functions in different sandboxes so that the failure of one function to not affect the others.

Here is the latest diff:

=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js	2010-03-17 13:11:23 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js	2010-03-17 22:18:24 +0000
@@ -59,7 +59,7 @@
 
 self.updateNotificationBox = function(e) {
   var notice = Y.one('.important-notice-container');
-  if (!notice) {
+  if (notice == null) {
     // We have no notice container on this page, this is why there is
     // nothing more to be done by this function.
     return;
@@ -71,7 +71,7 @@
   // Check the cookie to see if the user has already dismissed
   // the notification box for this session.
   var already_seen = Y.Cookie.get(dismiss_notice_cookie, Boolean);
-  if (already_seen) {
+  if (already_seen !== null) {
      notice.setStyle('display', 'none');
   }
 
@@ -79,7 +79,7 @@
       '.important-notice-cancel-button');
   // Cancel button starts out hidden.  If user has JavaScript,
   // then we want to show it.
-  if (!cancel_button) {
+  if (cancel_button == null) {
     // No cancel button was found to attach the action.
     return;
   }
@@ -94,7 +94,7 @@
 
 self.setFocus = function(field) {
     // if there is nofield, do nothing
-    if (Y.one('#' + field)) {
+    if (Y.one('#' + field) !== null) {
         Y.one('#' + field).focus();
     }
 };
@@ -151,7 +151,7 @@
         original_singular, translation_singular, singular_select);
 
     // Copy plural text if needed
-    if (Y.one('#' + translation_plural + '1')) {
+    if (Y.one('#' + translation_plural + '1') !== null) {
         copyOriginalTextPlural(
             original_plural, translation_plural, plural_forms);
     }
@@ -162,7 +162,7 @@
 
 var selectWidgetByID = function(widget) {
     var node = Y.one('#' + widget);
-    if (node) {
+    if (node !== null) {
         node.set('checked', true);
     }
 };
@@ -170,7 +170,7 @@
 
 var toggleWidget = function(widget) {
     var node = Y.one('#' + widget);
-    if (node) {
+    if (node !== null) {
         if (node.get('checked')) {
             node.set('checked', false);
         } else {
@@ -196,48 +196,48 @@
 
 var initializeGlobalKeyBindings = function(fields) {
 
-	Y.get('document').on("keyup", function(e) {
+    Y.get('document').on("keyup", function(e) {
         // Shift+Alt+s - Save form
-	    if (e.shiftKey && e.altKey && e.keyCode == 83) {
-	        Y.one('#save_and_continue_button').invoke('click');
-	    }
+        if (e.shiftKey && e.altKey && e.keyCode == 83) {
+            Y.one('#save_and_continue_button').invoke('click');
+        }
         // Shift+Alt+f - Go to search field
-	    if (e.shiftKey && e.altKey && e.keyCode == 70) {
-	        self.setFocus('search_box');
-	    }
+        if (e.shiftKey && e.altKey && e.keyCode == 70) {
+            self.setFocus('search_box');
+        }
         // Shift+Alt+b - Go to first translation field
-	    if (e.shiftKey && e.altKey && e.keyCode == 66) {
-	        self.setFocus(fields[0]);
-	    }
+        if (e.shiftKey && e.altKey && e.keyCode == 66) {
+            self.setFocus(fields[0]);
+        }
         // Shift+Alt+n - Go to next page in batch
-	    if (e.shiftKey && e.altKey && e.keyCode == 78) {
-	        link = Y.one('#batchnav_next');
-	        if (link){
-	            window.location.assign(link.get('href'));
-	        }
-	    }
+        if (e.shiftKey && e.altKey && e.keyCode == 78) {
+            link = Y.one('#batchnav_next');
+            if (link !== null){
+                window.location.assign(link.get('href'));
+            }
+        }
         // Shift+Alt+p - Go to previous page in batch
-	    if (e.shiftKey && e.altKey && e.keyCode == 80) {
-	        link = Y.one('#batchnav_previous');
-	        if (link){
-	            window.location.assign(link.get('href'));
-	        }
-	    }
+        if (e.shiftKey && e.altKey && e.keyCode == 80) {
+            link = Y.one('#batchnav_previous');
+            if (link !== null){
+                window.location.assign(link.get('href'));
+            }
+        }
         // Shift+Alt+a - Go to first page in batch
-	    if (e.shiftKey && e.altKey && e.keyCode == 65) {
-	        link = Y.one('#batchnav_first');
-	        if (link){
-	            window.location.assign(link.get('href'));
-	        }
-	    }
+        if (e.shiftKey && e.altKey && e.keyCode == 65) {
+            link = Y.one('#batchnav_first');
+            if (link !== null){
+                window.location.assign(link.get('href'));
+            }
+        }
         // Shift+Alt+l - Go to last page in batch
-	    if (e.shiftKey && e.altKey && e.keyCode == 76) {
-	        link = Y.one('#batchnav_last');
-	        if (link){
-	            window.location.assign(link.get('href'));
-	        }
-	    }
-	});
+        if (e.shiftKey && e.altKey && e.keyCode == 76) {
+            link = Y.one('#batchnav_last');
+            if (link !== null){
+                window.location.assign(link.get('href'));
+            }
+        }
+    });
 };
 
 

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-03-15 17:17:05 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-03-17 23:14:12 +0000
@@ -211,6 +211,9 @@
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/registry/team.js"></script>
 
+    <script type="text/javascript"
+            tal:attributes="src string:${lp_js}/translations/pofile.js"></script>
+
   </tal:devmode>
   <tal:production condition="not:devmode">
     <script type="text/javascript"

=== modified file 'lib/lp/translations/templates/pofile-translate.pt'
--- lib/lp/translations/templates/pofile-translate.pt	2010-03-17 12:00:59 +0000
+++ lib/lp/translations/templates/pofile-translate.pt	2010-03-17 22:12:53 +0000
@@ -14,22 +14,12 @@
     }
     </style>
 
-    <script type="text/javascript"
-      tal:condition="devmode"
-      tal:define="lp_js string:${icingroot}/build"
-      tal:attributes="src string:${lp_js}/translations/pofile.js"></script>
     <script type="text/javascript">
       registerLaunchpadFunction(insertAllExpansionButtons);
 
       LPS.use('lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.updateNotificationBox);
-      });
-
-      LPS.use('lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.setupSuggestionDismissal);
-      });
-
-      LPS.use('lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.initializeKeyBindings);
           Y.on('domready', function(e) {
             Y.lp.pofile.setFocus(autofocus_field);

=== modified file 'lib/lp/translations/templates/translationmessage-translate.pt'
--- lib/lp/translations/templates/translationmessage-translate.pt	2010-02-25 00:47:17 +0000
+++ lib/lp/translations/templates/translationmessage-translate.pt	2010-03-17 23:14:49 +0000
@@ -13,10 +13,6 @@
         color: lightgray;
     }
     </style>
-    <script type="text/javascript"
-      tal:condition="devmode"
-      tal:define="lp_js string:${icingroot}/build"
-      tal:attributes="src string:${lp_js}/translations/pofile.js"></script>
     <script type="text/javascript">
       LPS.use('node', 'lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.setupSuggestionDismissal);