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.


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

-Edwin


>=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
>--- lib/canonical/launchpad/javascript/translations/pofile.js	2010-02-25 12:56:45 +0000
>+++ lib/canonical/launchpad/javascript/translations/pofile.js	2010-03-17 17:36:08 +0000
>@@ -55,10 +55,15 @@
>       node.setStyle('display', 'none');
>   });
>   hide_anim.run();
>-}
>+};
>
> self.updateNotificationBox = function(e) {
>   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)


>+    // We have no notice container on this page, this is why there is
>+    // nothing more to be done by this function.
>+    return;
>+  }
>   var balloon = notice.one('.important-notice-balloon');
>   var dismiss_notice_cookie = ('translation-docs-for-' +
>                                documentation_cookie);
>@@ -74,6 +79,10 @@
>       '.important-notice-cancel-button');
>   // Cancel button starts out hidden.  If user has JavaScript,
>   // then we want to show it.
>+  if (!cancel_button) {


Same here.


>+    // No cancel button was found to attach the action.
>+    return;
>+  }
>   cancel_button.setStyle('visibility', 'visible');
>   cancel_button.on('click', function(e) {
>       e.halt();
>@@ -202,30 +211,34 @@
> 	    }
>         // Shift+Alt+n - Go to next page in batch
> 	    if (e.shiftKey && e.altKey && e.keyCode == 78) {
>-	        if (link = Y.one('#batchnav_next')){
>-	            window.location.assign(link.get('href'))
>+	        link = Y.one('#batchnav_next');
>+	        if (link){


Same for these if-statements.


>+	            window.location.assign(link.get('href'));
> 	        }
> 	    }
>         // Shift+Alt+p - Go to previous page in batch
> 	    if (e.shiftKey && e.altKey && e.keyCode == 80) {
>-	        if (link = Y.one('#batchnav_previous')){
>-	            window.location.assign(link.get('href'))
>+	        link = Y.one('#batchnav_previous');
>+	        if (link){
>+	            window.location.assign(link.get('href'));
> 	        }
> 	    }
>         // Shift+Alt+a - Go to first page in batch
> 	    if (e.shiftKey && e.altKey && e.keyCode == 65) {
>-	        if (link = Y.one('#batchnav_first')){
>-	            window.location.assign(link.get('href'))
>+	        link = Y.one('#batchnav_first');
>+	        if (link){
>+	            window.location.assign(link.get('href'));
> 	        }
> 	    }
>         // Shift+Alt+l - Go to last page in batch
> 	    if (e.shiftKey && e.altKey && e.keyCode == 76) {
>-	        if (link = Y.one('#batchnav_last')){
>-	            window.location.assign(link.get('href'))
>+	        link = Y.one('#batchnav_last');
>+	        if (link){
>+	            window.location.assign(link.get('href'));
> 	        }
> 	    }
> 	});


Please replace all the tab characters with spaces.
You can make vim display tab characters differently than spaces
by adding the following config to your .vimrc file.
    au BufEnter *.py set list listchars=tab:>*,trail:*
I have no idea how to do that in emacs.


>-}
>+};
> 
> 
> var initializeSuggestionsKeyBindings = function(stem) {
>@@ -243,7 +256,7 @@
>                 Y, node.get('id'));
>         }
>     });
>-}
>+};
> 
> 
> var initializeFieldsKeyBindings = function (fields) {
>@@ -321,7 +334,7 @@
>                 singular_copy_text);
>         }
>     }
>-}
>+};
> 
> 
> /**
>
>=== modified file 'lib/lp/translations/templates/pofile-translate.pt'
>--- lib/lp/translations/templates/pofile-translate.pt	2010-02-25 00:47:17 +0000
>+++ lib/lp/translations/templates/pofile-translate.pt	2010-03-17 17:36:08 +0000
>@@ -22,13 +22,21 @@
>       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);
>-          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.


>           Y.on('domready', Y.lp.pofile.initializeKeyBindings);
>           Y.on('domready', function(e) {
>             Y.lp.pofile.setFocus(autofocus_field);
>           });
>       });
>+
>+
>     </script>
>     </div>
>     <div metal:fill-slot="main">
>