Code review comment for lp:~benji/juju-gui/bug-1104105-browser-compatability-warning-3

Land with changes, unless you argue against the changes.

Cool, thank you, and thanks for the source fix. Works well for me.
What was the make prod problem?

The box is still not wide enough for me on FF, but is fine on IE. could
you push ".centered-column div" and ".centered-column .panel
input[type="submit"]" out from 260px to 280px, and ".centered-column
.panel input" out to 270px, unless you disagree? I think the design
folks might want to tweak, but not breaking the header text across lines
is more important than the arguably less attractive proportions of the
wider box, IMO.

https://codereview.appspot.com/7299071/diff/3002/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode108
app/index.html:108: <script id="app-startup">
You could protect all of these functions in a function(){[CODE]}()
namespace hack to keep them out of the global namespace. startTheApp is
the only one that would need to be global.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode109
app/index.html:109: isBrowserSupported = function(agent) {
Suggest "var isBrowserSupported = function(agent) {"

This is nicer for variable definition, and would work as desired within
the namespace hack I mention.

OTOH, if you don't use var, "function isBrowserSupported(agent) {" is
identical in semantics, at least as clear, and more concise.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode114
app/index.html:114: getDocument = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode118
app/index.html:118: displayBrowserWarning = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode123
app/index.html:123: continueWithCurrentBrowser = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode130
app/index.html:130: startTheApp = function() {
Same var discussion, except that it needs to be global as you've
implemented it here.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode136
app/index.html:136: window.setTimeout('startTheApp', 100);
Heh. Yeah, I considered this approach too. My global flag was grody in
my book, but so was this. I leaned toward the global flag, then, but
<shrug>.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode139
app/index.html:139: go = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/7299071/diff/3002/lib/merge-files.js#newcode50
lib/merge-files.js:50: result.code += '\n//@ sourceMappingURL=' +
sourceMapName;
Good catch. Seems silly that we have to do this.

https://codereview.appspot.com/7299071/