launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06927
[Merge] lp:~sinzui/launchpad/obsolete-js into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/obsolete-js into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #967210 in Launchpad itself: "obsolete JavaScript functions sent with every page"
https://bugs.launchpad.net/launchpad/+bug/967210
Bug #968310 in Launchpad itself: "javascript error editing a branch's related blueprints"
https://bugs.launchpad.net/launchpad/+bug/968310
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/obsolete-js/+merge/99978
Pre-implementation: no one
While investigating a forbidden error during an ajax request, I was
surprised to see comments by kiko about obsolete code. While the
forbidden error should have been handled gracefully by the ajax request,
there is a separate issue that every page in Lp is serving obsolete
JavaScript functions via base-layout-macros.pt.
All the functions in <metal:page-javascript define-macro="page-javascript"...
need review:
* VOID_URL is already inlined in it's only callsite, remove it.
* registerLaunchpadFunction() is only used by codeimport-new.pt; inline
the function.
* updateField() is only used by codeimport-new.pt; inline the function.
* setBetaRedirect() is used by the deprecated edge server; remove it.
* popup_window() has no callsites; remove it.
* switchBugBranchFormAndWhiteboard() has no callsites; remove it.
* switchSpecBranchFormAndSummary() is used by branch-related-bug-specs.pt
and branch-macros.pt. It could be inlined but it might belong in a
code/javascript module.
--------------------------------------------------------------------
RULES
* Delete and move until happiness is achieved.
ADDENDUM
* Bug #968310 javascript error editing a branch's related blueprints
switchSpecBranchFormAndSummary() errors on the branch page because
the form/markup that it used was removed. The function is 5 years
old an probably broke in the 2x or 3x redesign.
DOUBLE ADDENDUM
* WTF. The switchSpecBranchFormAndSummary() function is not only
causing scripting errors on the branch page, it was disabled
on the merge page years ago because it just does not work.
* delete switchSpecBranchFormAndSummary()
QA
* Visit https://code.qastaging.launchpad.net/+code-imports/+new
* Verify that when a repo type is selectected, its url field is
is enabled and the other are disabled.
* Visit https://code.qastaging.launchpad.net/~sinzui/gdp/incubation
* Verify there is not a green 'Edit' text after the blueprint link.
* Verify there is not a JS error when the edit link is followed.
LINT
lib/lp/app/templates/base-layout-macros.pt
lib/lp/code/templates/branch-macros.pt
lib/lp/code/templates/branch-related-bugs-specs.pt
lib/lp/code/templates/codeimport-new.pt
TEST
None. These were untested functions though I do know exactly how I will
QA the moved functions.
IMPLEMENTATION
Inlined updateWidgets() and registerLaunchpadFunction()'s LPJS setup. I
changed the call to run on DOMREADY and I chose to make it clear that
that the functions are global vars.
lib/lp/code/templates/codeimport-new.pt
I deleted the uses of switchSpecBranchFormAndSummary() which broke the
branch page (branch-related-bugs-specs) and was disabled on the merge
proposal page (branch-macros). In the former case I chose to keep the
edit link which is what my browser failed over to use. In the later
case, the edit links were never generated, so I choose to just deleted.
lib/lp/code/templates/branch-macros.pt
lib/lp/code/templates/branch-related-bugs-specs.pt
I removed all the unused functions. The two remaining functions are used
by LaunchpadForm and Widgets to set the field that has focus and ensure
that keyboard commands are not misinterpreted by <select>.
lib/lp/app/templates/base-layout-macros.pt
--
https://code.launchpad.net/~sinzui/launchpad/obsolete-js/+merge/99978
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/obsolete-js into lp:launchpad.
=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt 2012-03-08 14:47:15 +0000
+++ lib/lp/app/templates/base-layout-macros.pt 2012-03-29 17:03:10 +0000
@@ -84,7 +84,7 @@
<tal:js-loader condition="request/features/js.combo_loader.enabled">
<script type="text/javascript"
- tal:attributes="src string:${combo_url}/?yui/yui/yui-min.js&lp/meta.js&yui/loader/loader-min.js"></script>
+ tal:attributes="src string:${combo_url}/?yui/yui/yui-min.js&lp/meta.js&yui/loader/loader-min.js"></script>
<script type="text/javascript" tal:content="string:
YUI.GlobalConfig = {
combine: true,
@@ -190,37 +190,6 @@
//<![CDATA[
// This code is pulled from lp.js that needs to be available on every
// request. Pulling here to get it outside the scope of the YUI block.
- // Lint-safe scripting URL.
- var VOID_URL = '_:void(0);'.replace('_', 'javascript');
-
- function registerLaunchpadFunction(func) {
- // registers a function to fire onload.
- // Use this for initilaizing any javascript that should fire once the page
- // has been loaded.
- LPJS.use('node', function(Y) {
- Y.on('load', function(e) {
- func();
- }, window);
- });
- }
-
- // Enable or disable the beta.launchpad.net redirect
- function setBetaRedirect(enable) {
- var expire = new Date();
- if (enable) {
- expire.setTime(expire.getTime() + 1000);
- document.cookie = ('inhibit_beta_redirect=0; Expires=' +
- expire.toGMTString() + cookie_scope);
- alert('Redirection to the beta site has been enabled');
- } else {
- expire.setTime(expire.getTime() + 2 * 60 * 60 * 1000);
- document.cookie = ('inhibit_beta_redirect=1; Expires=' +
- expire.toGMTString() + cookie_scope);
- alert('You will not be redirected to the beta site for 2 hours');
- }
- return false;
- }
-
function setFocusByName(name) {
// Focus the first element matching the given name which can be focused.
var nodes = document.getElementsByName(name);
@@ -244,24 +213,6 @@
}
}
- function popup_window(url, name, width, height) {
- var iframe = document.getElementById('popup_iframe_' + name);
- if (!iframe.src || iframe.src === VOID_URL) {
- // The first time this handler runs the window may not have been
- // set up yet; sort that out.
- iframe.style.width = width + 'px';
- iframe.style.height = height + 'px';
- iframe.style.position = 'absolute';
- iframe.style.background = 'white';
- iframe.src = url;
- }
- iframe.style.display = 'inline';
- // I haven't found a way of making the search form focus again when
- // the popup window is redisplayed. I tried doing an
- // iframe.contentDocument.searchform.search.focus()
- // but nothing happens.. -- kiko, 2007-03-12
- }
-
function selectWidget(widget_name, event) {
if (event && (event.keyCode === 9 || event.keyCode === 13)) {
// Avoid firing if user is tabbing through or simply pressing
@@ -270,65 +221,6 @@
}
document.getElementById(widget_name).checked = true;
}
-
- function switchBugBranchFormAndWhiteboard(id) {
- var div = document.getElementById('bugbranch' + id);
- var wb = document.getElementById('bugbranch' + id + '-wb');
-
- if (div.style.display === "none") {
- /* Expanding the form */
- if (wb !== null) {
- wb.style.display = "none";
- }
- div.style.display = "block";
- /* Use two focus calls to get the browser to scroll to the end of the
- * form first, then focus back to the first field of the form.
- */
- document.getElementById('field'+id+'.actions.update').focus();
- document.getElementById('field'+id+'.whiteboard').focus();
- } else {
- if (wb !== null) {
- wb.style.display = "block";
- }
- div.style.display = "none";
- }
- return false;
- }
-
- function switchSpecBranchFormAndSummary(id) {
- /* The document has two identifiable elements for each
- * spec-branch link:
- * 'specbranchX' which is the div containing the edit form
- * 'specbranchX-summary' which is the div contining the sumary
- * where X is the database id of the link.
- */
- var div = document.getElementById('specbranch' + id);
- var wb = document.getElementById('specbranch' + id + '-summary');
-
- if (div.style.display === "none") {
- /* Expanding the form */
- if (wb !== null) {
- wb.style.display = "none";
- }
- div.style.display = "block";
- /* Use two focus calls to get the browser to scroll to the end of the
- * form first, then focus back to the first field of the form.
- */
- document.getElementById('field' + id + '.actions.change').focus();
- document.getElementById('field' + id + '.summary').focus();
- } else {
- if (wb !== null) {
- wb.style.display = "block";
- }
- div.style.display = "none";
- }
- return false;
- }
-
- function updateField(field, enabled)
- {
- field.disabled = !enabled;
- }
//]]>
</script>
</metal:page-javascript>
=== modified file 'lib/lp/code/templates/branch-macros.pt'
--- lib/lp/code/templates/branch-macros.pt 2012-03-01 21:50:42 +0000
+++ lib/lp/code/templates/branch-macros.pt 2012-03-29 17:03:10 +0000
@@ -150,12 +150,6 @@
<tal:link replace="structure spec/fmt:link:blueprints" />
(<span tal:attributes="class string:specpriority${spec/priority/name}"
tal:content="spec/priority/title">High</span>)
- <tal:show-edit condition="show_edit|nothing">
- -
- <a tal:attributes="href spec_branch/fmt:url;
- onclick string:return switchSpecBranchFormAndSummary('${spec_branch/id}')">
- edit <img src="/@@/bug-status-expand"/></a>
- </tal:show-edit>
</div>
<div tal:condition="show_edit|nothing"
tal:attributes="id string:specbranch${spec_branch/id}"
=== modified file 'lib/lp/code/templates/branch-related-bugs-specs.pt'
--- lib/lp/code/templates/branch-related-bugs-specs.pt 2012-03-01 21:50:42 +0000
+++ lib/lp/code/templates/branch-related-bugs-specs.pt 2012-03-29 17:03:10 +0000
@@ -52,10 +52,8 @@
class="spec-branch-summary">
<tal:link replace="structure spec/fmt:link:blueprints" />
<tal:show-edit condition="show_edit|nothing">
- <a tal:attributes="href spec_branch/fmt:url;
- onclick string:return switchSpecBranchFormAndSummary('${spec_branch/id}')">
- edit<img src="/@@/edit"/>
- </a>
+ <a class="sprite edit"
+ tal:attributes="href spec_branch/fmt:url;"> </a>
</tal:show-edit>
</li>
=== modified file 'lib/lp/code/templates/codeimport-new.pt'
--- lib/lp/code/templates/codeimport-new.pt 2011-09-28 10:28:47 +0000
+++ lib/lp/code/templates/codeimport-new.pt 2012-03-29 17:03:10 +0000
@@ -128,8 +128,12 @@
<script type="text/javascript">
//<![CDATA[
- function updateWidgets() {
+ var updateWidgets = function() {
/* which rcs type radio button has been selected? */
+ var updateField = function (field, enabled) {
+ field.disabled = !enabled;
+ };
+
var rcs_types = document.getElementsByName('field.rcs_type');
var form = rcs_types[0].form;
var rcs_type = 'None';
@@ -150,8 +154,13 @@
updateField(form['field.svn_branch_url'], rcs_type == 'BZR_SVN');
// BZR
updateField(form['field.bzr_branch_url'], rcs_type == 'BZR');
- }
- registerLaunchpadFunction(updateWidgets);
+ };
+
+ LPJS.use('node', function(Y) {
+ Y.on('domready', function(e) {
+ updateWidgets();
+ });
+ });
//]]>
</script>
Follow ups
