Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(324)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 125123002: try.dartlang.org version 5. (Closed)

Created:
6 years, 11 months ago by ahe
Modified:
6 years, 11 months ago
Reviewers:
kasperl
CC:
reviews_dartlang.org
Base URL:
/Users/ahe/Dart/all@master
Visibility:
Public.

Description

try.dartlang.org version 5. Benchmarks excluded. Compiled with r22037 and additional patches (https://codereview.chromium.org/116043007/). R=kasperl@google.com Committed: https://code.google.com/p/dart/source/detail?r=31539

Patch Set 1 : #

Total comments: 36
Unified diffs Side-by-side diffs Delta from patch set Stats (+43519 lines, -5 lines) Patch
A dart/site/try/app.yaml View 1 chunk +39 lines, -0 lines 2 comments Download
A dart/site/try/compiler_isolate.dart View 1 chunk +118 lines, -0 lines 0 comments Download
A dart/site/try/create_manifest.sh View 1 chunk +30 lines, -0 lines 0 comments Download
A dart/site/try/dart-icon.png View Binary file 0 comments Download
A dart/site/try/dart-iphone5.png View Binary file 0 comments Download
A dart/site/try/dartlang-style.css View 1 chunk +38 lines, -0 lines 0 comments Download
A dart/site/try/decoration.dart View 1 chunk +101 lines, -0 lines 0 comments Download
A dart/site/try/deploy.sh View 1 chunk +6 lines, -0 lines 2 comments Download
A dart/site/try/extract_theme.dart View 1 chunk +83 lines, -0 lines 0 comments Download
A dart/site/try/extracted_themes.dart View 1 chunk +1475 lines, -0 lines 0 comments Download
A + dart/site/try/favicon.ico View Binary file 0 comments Download
A dart/site/try/iframe.html View 1 chunk +15 lines, -0 lines 0 comments Download
A dart/site/try/iframe.js View 1 chunk +45 lines, -0 lines 6 comments Download
A dart/site/try/index.html View 1 chunk +150 lines, -0 lines 0 comments Download
A dart/site/try/jsonify.dart View 1 chunk +174 lines, -0 lines 0 comments Download
A dart/site/try/leap.dart View 1 chunk +1267 lines, -0 lines 20 comments Download
A dart/site/try/leap.dart.js View 1 chunk +15134 lines, -0 lines 0 comments Download
A dart/site/try/nossl.appcache View 1 chunk +22 lines, -0 lines 2 comments Download
A dart/site/try/part.js View 1 chunk +24734 lines, -0 lines 0 comments Download
A dart/site/try/theme_default.dart View 1 chunk +79 lines, -0 lines 2 comments Download
A + dart/site/try/themes.dart View 1 chunk +4 lines, -5 lines 0 comments Download
A dart/site/try/try-dart-screenshot.png View Binary file 0 comments Download
A dart/site/try/update_howto.txt View 1 chunk +5 lines, -0 lines 2 comments Download

Messages

Total messages: 4 (0 generated)
ahe
6 years, 11 months ago (2014-01-06 15:55:47 UTC) #1
kasperl
LGTM. As we talked about, it makes sense to address comments in new CLs. https://codereview.chromium.org/125123002/diff/420001/dart/site/try/app.yaml ...
6 years, 11 months ago (2014-01-07 07:18:42 UTC) #2
ahe
Committed patchset #1 manually as r31539 (presubmit successful).
6 years, 11 months ago (2014-01-07 11:31:21 UTC) #3
ahe
6 years, 11 months ago (2014-01-07 14:06:22 UTC) #4
Message was sent while issue was closed. 
Thank you, Kasper.

I have addressed your comments in CL 126003002.

Cheers,
Peter

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/app.yaml
File dart/site/try/app.yaml (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/app.yaml#...
dart/site/try/app.yaml:1: application: try-dart-lang
On 2014/01/07 07:18:43, kasperl wrote:
> Can you add a comment about the purpose of this file? Is it an AppCache
thingy?

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/deploy.sh
File dart/site/try/deploy.sh (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/deploy.sh...
dart/site/try/deploy.sh:1: old=$1
On 2014/01/07 07:18:43, kasperl wrote:
> Add comment that explains what this is for.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/iframe.js
File dart/site/try/iframe.js (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/iframe.js...
dart/site/try/iframe.js:22: return;
On 2014/01/07 07:18:43, kasperl wrote:
> Do you really need this return?

No, but it makes it easier to add new cases.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/iframe.js...
dart/site/try/iframe.js:31: window.parent.postMessage(["scrollHeight",
document.documentElement.scrollHeight], "*");
On 2014/01/07 07:18:43, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/iframe.js...
dart/site/try/iframe.js:34: var observer = new
(window.MutationObserver||window.WebKitMutationObserver||window.MozMutationObserver)(function(mutations)
{
On 2014/01/07 07:18:43, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart
File dart/site/try/leap.dart (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:12: import
'../sdk/lib/_internal/compiler/implementation/scanner/scannerlib.dart' show
StringScanner, EOF_TOKEN;
On 2014/01/07 07:18:43, kasperl wrote:
> Long lines.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:34: String codeFont = ((x) => x == null ? '' :
x)(window.localStorage['codeFont']);
On 2014/01/07 07:18:43, kasperl wrote:
> Maybe just add an extra rawCodeFont variable? This is clever, but maybe overly
> so.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:61: onMutation(List<MutationRecord> mutations,
MutationObserver observer) {
On 2014/01/07 07:18:43, kasperl wrote:
> This method is very long. Maybe break it into a few smaller methods with names
> that help the reader understand what's going on?

Totally agree. I'll add a TODO for now.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:77: case 'characterData':
On 2014/01/07 07:18:43, kasperl wrote:
> Indent cases.

OK. That was hard:

(setq my-dart-style
      '((c-basic-offset . 2)
        (c-offsets-alist (case-label . +))))

(defun my-dart-mode-hook ()
  (c-add-style "my-dart-style" my-dart-style t))

(add-hook 'dart-mode-hook 'my-dart-mode-hook)

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:143: //   var n, a=[],
walk=document.createTreeWalker(root,NodeFilter.SHOW_TEXT,null,false);
On 2014/01/07 07:18:43, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:158: while(child != null) {
On 2014/01/07 07:18:43, kasperl wrote:
> DANGER. DANGER. You know the drill!

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:226: void walk4(Node node) {
On 2014/01/07 07:18:43, kasperl wrote:
> Could this be refactored somehow? You have walk4 in multiple places.

This might be fixed by using TreeWalker.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:274: observer.observe(inputPre, childList: true,
characterData: true, subtree: true);
On 2014/01/07 07:18:43, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:366: case 'done': return onDone(data);
On 2014/01/07 07:18:43, kasperl wrote:
> I'd indent all the cases.

Done.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/leap.dart...
dart/site/try/leap.dart:898: fieldSet.append(
On 2014/01/07 07:18:43, kasperl wrote:
> Maybe it would make sense to have an abstraction over these "flags" that can
be
> stored in local storage and checked by the user?

Added TODO.

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/nossl.app...
File dart/site/try/nossl.appcache (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/nossl.app...
dart/site/try/nossl.appcache:1: CACHE MANIFEST
On 2014/01/07 07:18:43, kasperl wrote:
> Maybe you should investigate whether or not the ServiceWorker API would be
> useful instead of AppCache?
> 
> See https://github.com/slightlyoff/ServiceWorker/.

I don't think ServiceWorkers are available in Dart, and they seemed to be
designed in a way that isn't optimal for Dart. See
https://github.com/slightlyoff/ServiceWorker/blob/master/explainer.md#a-quick....

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/theme_def...
File dart/site/try/theme_default.dart (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/theme_def...
dart/site/try/theme_default.dart:36: Decoration get
filteredSearchResultIndication =>
On 2014/01/07 07:18:43, kasperl wrote:
> Nit: I'm probably the only one, but I tend to try to format all the getters
the
> same way. Since more than one of them do not fit one one line, I'd go ahead
and
> break *before* => for all of them. YMMV.

My preference would be to allow longer lines ;-)

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/update_ho...
File dart/site/try/update_howto.txt (right):

https://codereview.chromium.org/125123002/diff/420001/dart/site/try/update_ho...
dart/site/try/update_howto.txt:1: $ git pull --ff-only
On 2014/01/07 07:18:43, kasperl wrote:
> Maybe add a comment here to explain what this is used for?

Deleted this file and added comments to deploy.sh.

Powered by Google App Engine
This is Rietveld 408576698