Should go into js/attachment.js.

The template version of the Javascript has some conditional changes. e.g. [% IF patchviewerinstalled %] Looking at the existing files in js/, I could not see any examples of how that sort of conditional is handled. Is there a consensus? Is it OK fpr the template tp set some javascript global vars to be used in the attachment.js code, or is there another way?

IMO, patchviewerinstalled could be passed as an argument to the function.

Gavin, are you working on this or can I take it?

Thank-you for picking this up. I got about as far as you did with your patch. The problem is that there are other templates that need the attachment javascript, and they now need to have js/attachment.js included, but there is not a clean maintainable way of figuring out which need it.

(In reply to comment #5) > The problem is that there are other templates that need the attachment > javascript, and they now need to have js/attachment.js included Huh? Why? attachment/edit.html.tmpl will load the JS script itself.

(In reply to comment #6) > (In reply to comment #5) > > The problem is that there are other templates that need the attachment > > javascript, and they now need to have js/attachment.js included > > Huh? Why? attachment/edit.html.tmpl will load the JS script itself. May not be an issue. Perhaps I was getting ahead of myself and also moving the js out of attachment/list.html.tmpl

Comment on attachment 379841 [details] [diff] [review] Moved the js functions into js/attachment.js >Index: template/en/default/attachment/edit.html.tmpl > <script type="text/javascript"> > <!-- >+ var attachmentid = [% attachment.id %]; >+ var patchviewerinstalled = 0 > [% IF patchviewerinstalled %] >+ patchviewerinstalled = 1; > [% END %] > //--> > </script> This <script> block should go away. You don't need it, see below. >+<script src="js/attachment.js" type="text/javascript"></script> You should pass this script to global/header.html.tmpl, by using javascript_urls = ['js/attachment.js'] >+ document.write('<button type="button" id="editButton" onclick="editAsComment(patchviewerinstalled);">Edit Attachment As Comment<\/button>'); Define patchreader in this <script> block, before this document.write() call. >+ document.write('<button type="button" id="viewDiffButton" onclick="viewDiff(patchviewerinstalled);">View Attachment As Diff<\/button>'); viewDiff() is the single function which needs attachmentid. You should pass it explicitly as argument. You can define attachmentid in this <script> block too.

Comment on attachment 383062 [details] [diff] [review] fixed per the comments >Index: js/attachment.js >+function viewDiff(attachment_id) You must pass patchviewerinstalled as the 2nd argument. >+function viewRaw() And here as the first argument. >+ document.write('<button type="button" id="viewDiffButton" onclick="viewDiff(attachment_id, patchviewerinstalled);">View Attachment As Diff<\/button>'); > [% END %] >+ document.write('<button type="button" id="viewRawButton" onclick="viewRaw(patchviewerinstalled);" style="display: none;">View Attachment As Raw<\/button>'); Both can be fixed on checkin. r=LpSolit

Checking in js/attachment.js; /cvsroot/mozilla/webtools/bugzilla/js/attachment.js,v <-- attachment.js new revision: 1.6; previous revision: 1.5 done Checking in template/en/default/attachment/edit.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v <-- edit.html.tmpl new revision: 1.59; previous revision: 1.58 done

Comment on attachment 383062 [details] [diff] [review] fixed per the comments >Index: js/attachment.js >+var prev_mode = 'raw'; >+var current_mode = 'raw'; >+var has_edited = 0; >+var has_viewed_as_diff = 0; It's not really OK to put variables with such generic names into the global namespace in a JS file. It's OK on a single page, but in a JS file it can be very misleading.

attachment.js is specific to attachments. So no other page will load this JS script.