Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3928)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 43200043: Adding http support. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by pajamallama
Modified:
10 years, 3 months ago
Reviewers:
tapted
CC:
chrome-apps-internsyd_google.com
Base URL:
https://github.com/tapted/bleeding_edge.git@master
Visibility:
Public.

Description

Adding http support.

Patch Set 1 : #

Total comments: 22
Unified diffs Side-by-side diffs Delta from patch set Stats (+209 lines, -56 lines) Patch
M dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart View 1 chunk +4 lines, -1 line 4 comments Download
M dart/sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart View 1 chunk +1 line, -6 lines 2 comments Download
M dart/sdk/lib/_internal/pub/lib/src/source.dart View 3 chunks +19 lines, -19 lines 4 comments Download
M dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart View 5 chunks +31 lines, -21 lines 8 comments Download
M dart/sdk/lib/_internal/pub/lib/src/wrap/httpwrap.dart View 1 chunk +70 lines, -2 lines 0 comments Download
M dart/sdk/lib/_internal/pub/lib/src/wrap/iowrap.dart View 2 chunks +7 lines, -1 line 2 comments Download
M dart/sdk/lib/_internal/pub/lib/src/wrap/path_rep.dart View 1 chunk +2 lines, -3 lines 2 comments Download
M dart/sdk/lib/_internal/pub/lib/src/wrap/system_cache.dart View 1 chunk +71 lines, -2 lines 0 comments Download
M dart/sdk/lib/_internal/pub/manifest.json View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 3
pajamallama
10 years, 4 months ago (2013-12-17 06:36:30 UTC) #1
tapted
initial comments https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart File dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart (right): https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart#newcode26 dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:26: //TODO: Get a proper local cache here. ...
10 years, 4 months ago (2013-12-17 06:59:57 UTC) #2
pajamallama
10 years, 3 months ago (2014-01-02 04:13:36 UTC) #3 
Fixed all the issues noted here in the CL
https://codereview.appspot.com/45210043

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart (right):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:26: //TODO: Get a proper
local cache here.
On 2013/12/17 06:59:57, tapted wrote:
> nit: we always put a space between the '//' and the comment. Then TODOs go
> 
> // TODO(pajamallama): Get a proper local cache here.

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:28:
pathRep.fullPath()+'/cache');
On 2013/12/17 06:59:57, tapted wrote:
> nit: indent 4 spaces, and a space either side of any operator (except . or ->
or
> :: or .* or ->* or the sizeof operator... and maybe some other obscure ones).
> Also, Dart seems to prefer double-quotes for strings.

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:708: return;
On 2013/12/17 06:59:57, tapted wrote:
> Is there a way to do this by adding a wrapper that gets
> (pubspec.environment.sdkVersion.allows(..) to always return `true`?

The pubspec.environment.sdkVersion should always be VersionConstraint.Any so
this can just be left in.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source.dart:11: import 'wrap/iowrap.dart';
On 2013/12/17 06:59:57, tapted wrote:
> nit: sort imports alphabetically/lexicographically in each section. (sections
> are `dart:`, `package:`, others/mine)

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source.dart:94: //   return
systemCacheDirectory(id).then((packageDir) {
On 2013/12/17 06:59:57, tapted wrote:
> What needs to be wrapped here?
> 
> Do we just need to wrap systemCacheDirectory / fileExists?
> 
> Same below.

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart (left):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart:13: import '../io.dart';
On 2013/12/17 06:59:57, tapted wrote:
> io.dart already imports iowrap.. 

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart (right):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart:90: (blob) =>
io.saveBlobToFile(blob, destPath)
On 2013/12/17 06:59:57, tapted wrote:
> can you make io.saveBlobToFile take a `FileRep` instead of a String, and leave
> this code untouched, except for changing the `destPath` argument?
> 
> Probably also extractTarGz->extractArchive (which is wrapped appropriately to
> use a TarGz or .zip file)

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart:155: // if (error is
TimeoutException) {
On 2013/12/17 06:59:57, tapted wrote:
> We should be able to just provide dummy classes for TimeoutException, and
> (hopefully) lean on the io.SocketException in io.dart

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart:265: /// A pair of values.
On 2013/12/17 06:59:57, tapted wrote:
> Should try to move this to the appropriate foowrap.dart file too

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/iowrap.dart (right):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/iowrap.dart:84: static Map<String,
String> get environment => {"_PUB_TEST_SDK_VERSION": "1.0.0"};
On 2013/12/17 06:59:57, tapted wrote:
> nit: wrap as 
> 
>   static Map<String, String> get environment =>
>       {"_PUB_TEST_SDK_VERSION": "1.0.0"};

Done.

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/path_rep.dart (right):

https://codereview.appspot.com/43200043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/path_rep.dart:41: // TODO: use local
storage to retain access to this file
On 2013/12/17 06:59:57, tapted wrote:
> this needs to be reset back to master

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b