Modify the dartdoc code rendering. Syntax highlightning

Modify the dartdoc code rendering. Syntax highlightning
is now performed on the client side.

[Bob Nystrom, 2011-12-19 23:17:34]

Just a couple of minor points.

One thing that worries me a bit is that interact.js is a good bit bigger now,
but let's not worry about that for now.

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart
File dart/utils/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
dart/utils/dartdoc/dartdoc.dart:573:
writeln(md.escapeHtml('${unindentCode(span)}'));
You don't need to interpolate here.

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
dart/utils/dartdoc/dartdoc.dart:702: * Unindents a bunch of Dart code lines
This needs a better comment. Just take the "Remove leading..." comment from the
body and use that.

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/interact.dart
File dart/utils/dartdoc/interact.dart (right):

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/interact.dar...
dart/utils/dartdoc/interact.dart:8: #import('../../frog/lang.dart');
I tend to place relative #imports after "dart:" ones. Move these after
dart:html?

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/interact.dar...
dart/utils/dartdoc/interact.dart:28: // Syntax highlight
Add a period. Comments should always be full sentences.

[michael.haubenwallner, 2011-12-20 08:54:54]

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart
File dart/utils/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
dart/utils/dartdoc/dartdoc.dart:573:
writeln(md.escapeHtml('${unindentCode(span)}'));
On 2011/12/19 23:17:34, Bob Nystrom wrote:
> You don't need to interpolate here.

Small problem on the client side: when the code string is accessed through
'pre.text' everything element-like gets stripped, e.g. 'List<String> getNames()
{}' results in 'List getNames() {}'. Accessing through pre.innerHTML rewrites it
to 'List<string> getNames() {} </string>'.
Escaping the code string seems reasonable, what do you think?

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
dart/utils/dartdoc/dartdoc.dart:702: * Unindents a bunch of Dart code lines
On 2011/12/19 23:17:34, Bob Nystrom wrote:
> This needs a better comment. Just take the "Remove leading..." comment from
the
> body and use that.

Done.

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/interact.dart
File dart/utils/dartdoc/interact.dart (right):

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/interact.dar...
dart/utils/dartdoc/interact.dart:8: #import('../../frog/lang.dart');
On 2011/12/19 23:17:34, Bob Nystrom wrote:
> I tend to place relative #imports after "dart:" ones. Move these after
> dart:html?

Done.

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/interact.dar...
dart/utils/dartdoc/interact.dart:28: // Syntax highlight
On 2011/12/19 23:17:34, Bob Nystrom wrote:
> Add a period. Comments should always be full sentences.

Done.

[Bob Nystrom, 2011-12-20 18:46:36]

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart
File dart/utils/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
dart/utils/dartdoc/dartdoc.dart:573:
writeln(md.escapeHtml('${unindentCode(span)}'));
On 2011/12/20 08:54:54, michael.haubenwallner wrote:
> On 2011/12/19 23:17:34, Bob Nystrom wrote:
> > You don't need to interpolate here.
> 
> Small problem on the client side: when the code string is accessed through
> 'pre.text' everything element-like gets stripped, e.g. 'List<String>
getNames()
> {}' results in 'List getNames() {}'. Accessing through pre.innerHTML rewrites
it
> to 'List<string> getNames() {} </string>'.
> Escaping the code string seems reasonable, what do you think?

Right, sorry if I wasn't clear. Escaping is definitely good. What you don't need
(as far as I can tell) is to *interpolate*. You just be able to do:

writeln(md.escapeHtml(unindentCode(span)));

A string literal whose entire contents are just an interpolated variable
shouldn't be needed. (If it is, it's a bug.)

http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
dart/utils/dartdoc/dartdoc.dart:702: * Unindents a bunch of Dart code lines
On 2011/12/20 08:54:54, michael.haubenwallner wrote:
> On 2011/12/19 23:17:34, Bob Nystrom wrote:
> > This needs a better comment. Just take the "Remove leading..." comment from
> the
> > body and use that.
> 
> Done.

When you've made changes during a review, you have to explicitly upload those
too so that the review sees them. There are instructions here:
http://dev.chromium.org/developers/contributing-code

The relevant bits are:


You will likely get email back from the reviewer with comments. Fix these and
update the patch set in the issue by uploading again.  The upload will explain
that it is updating the current CL and ask you for a message explaining the
change.

GCL

Add any new files that need to be added to your checkout

gcl change changelist_name

gcl upload changelist_name

GIT CL

If you need to update code the code on an already uploaded CL, simply
edit the code, commit it again locally, and then run git cl upload again e.g.

echo "" > GOATS
git add GOATS
git commit -m 'add newline fix to GOATS'
git cl upload


Once you're ready for another review, use Publish+Mail Comments again to send
another notification (you can say something like "new snapshot uploaded").  When
the reviewer is happy with your patch, they will say "LGTM" ("Looks Good To
Me").

:)

[Bob Nystrom, 2011-12-20 18:47:51]

On 2011/12/20 18:46:36, Bob Nystrom wrote:
> http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart
> File dart/utils/dartdoc/dartdoc.dart (right):
> 
>
http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
> dart/utils/dartdoc/dartdoc.dart:573:
> writeln(md.escapeHtml('${unindentCode(span)}'));
> On 2011/12/20 08:54:54, michael.haubenwallner wrote:
> > On 2011/12/19 23:17:34, Bob Nystrom wrote:
> > > You don't need to interpolate here.
> > 
> > Small problem on the client side: when the code string is accessed through
> > 'pre.text' everything element-like gets stripped, e.g. 'List<String>
> getNames()
> > {}' results in 'List getNames() {}'. Accessing through pre.innerHTML
rewrites
> it
> > to 'List<string> getNames() {} </string>'.
> > Escaping the code string seems reasonable, what do you think?
> 
> Right, sorry if I wasn't clear. Escaping is definitely good. What you don't
need
> (as far as I can tell) is to *interpolate*. You just be able to do:
> 
> writeln(md.escapeHtml(unindentCode(span)));
> 
> A string literal whose entire contents are just an interpolated variable
> shouldn't be needed. (If it is, it's a bug.)
> 
>
http://codereview.chromium.org/8974006/diff/1/dart/utils/dartdoc/dartdoc.dart...
> dart/utils/dartdoc/dartdoc.dart:702: * Unindents a bunch of Dart code lines
> On 2011/12/20 08:54:54, michael.haubenwallner wrote:
> > On 2011/12/19 23:17:34, Bob Nystrom wrote:
> > > This needs a better comment. Just take the "Remove leading..." comment
from
> > the
> > > body and use that.
> > 
> > Done.
> 
> When you've made changes during a review, you have to explicitly upload those
> too so that the review sees them. There are instructions here:
> http://dev.chromium.org/developers/contributing-code
> 
> The relevant bits are:
> 
> 
> You will likely get email back from the reviewer with comments. Fix these and
> update the patch set in the issue by uploading again.  The upload will explain
> that it is updating the current CL and ask you for a message explaining the
> change.
> 
> GCL
> 
> Add any new files that need to be added to your checkout
> 
> gcl change changelist_name
> 
> gcl upload changelist_name
> 
> GIT CL
> 
> If you need to update code the code on an already uploaded CL, simply
> edit the code, commit it again locally, and then run git cl upload again e.g.
> 
> echo "" > GOATS
> git add GOATS
> git commit -m 'add newline fix to GOATS'
> git cl upload
> 
> 
> Once you're ready for another review, use Publish+Mail Comments again to send
> another notification (you can say something like "new snapshot uploaded"). 
When
> the reviewer is happy with your patch, they will say "LGTM" ("Looks Good To
> Me").
> 
> :)

Heh, disregard that the last message looks like an LG-TM. It's picking up that
string in the review instructions. Oops!

[michael.haubenwallner, 2011-12-20 19:14:15]

Uploaded the new changes.

Sorry, its been my mistake - thought 'escape' when i read 'interpolate'.

[Bob Nystrom, 2011-12-20 19:50:33]

On 2011/12/20 19:14:15, michael.haubenwallner wrote:
> Uploaded the new changes.
> 
> Sorry, its been my mistake - thought 'escape' when i read 'interpolate'.

LGTM. Committed:

https://code.google.com/p/dart/source/detail?r=2657
https://code.google.com/p/dart/source/detail?r=2658

Welcome to Dart! :)