729086 - Cannot review some bugs - no comment popup shown

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Splinter, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

While trying to do a splinter review on bug 725881, from the following link:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=725881&attachment=598282

I get shown the following when trying to add comments:
http://dl.dropbox.com/u/32496746/splinterbug.png

i.e. only a grey line, no box and no buttons.
Other bugs behave as expected (I get an edit box with 2 buttons).

Comment 1

[David Lawrence [:dkl]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #0)
> While trying to do a splinter review on bug 725881, from the following link:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=725881&attachment=598282
> 
> I get shown the following when trying to add comments:
> http://dl.dropbox.com/u/32496746/splinterbug.png
> 
> i.e. only a grey line, no box and no buttons.
> Other bugs behave as expected (I get an edit box with 2 buttons).

The javascript code is crashing for some reason once the line is double clicked and the error is not very informative. Will look into why this happening and try to find a fix.

dkl

Comment 2

[:glob ✱]

it's throwing an uncaught exception when the page is loaded:

  uncaught exception: Bad oldLine,newLine: 219,211

so it isn't being initialised correctly.

Comment 3

[:glob ✱]

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=707940&attachment=598834 is another problematic review, another uncaught exception at load.

Comment 4

[:glob ✱]

the problem is caused by any "changed" comment (where you create a comment by double-clicking on the middle column - the one with the line numbers for the right-hand diff).

this resulted in:

::: build/mobile/robocop/FennecMochitestAssert.java.in
@@ -210,2 +202,22 @@
> > -        boolean pass = a.equals(b);
> > +        boolean pass = checkObjectsEqual(a,b);
> > -        String diag = "got " + a.toString() + ", expected " + b.toString();
> > +        todo(pass, name, getEqualString(a,b,pass));
> > +    }
> > +
NaN more ...


note the "NaN more ...", it should be "> 2 more ...".

not only is the generation code broken, but the parsing code also has problems, and will require a bit of time to rectify.  it may be easier to disable this feature and kludge the parsing code to make it not break.

Comment 5

[:glob ✱]

Attachment: patch v1

this patch:
  - fixes the NaN creation issue
  - disables "changed" comments completely, until the other issues can be fixed
  - changes logging from YAHOO.log to console.log
  - collects malformed review comments and puts them at position 0 instead of
    dropping them 
  - removes commented out code

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 599589 [details] [diff] [review]
patch v1

Review of attachment 599589 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 7

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/Splinter/template/en/default/pages/splinter.html.tmpl
modified extensions/Splinter/web/splinter.js
Committed revision 8071.

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/Splinter/template/en/default/pages/splinter.html.tmpl
modified extensions/Splinter/web/splinter.js
Committed revision 8059.