Issue 50240043: Project now working in both Javascript & Dartium.

Issue 50240043: Project now working in both Javascript & Dartium. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 3 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.

More Reviews

Description

Project now working in both Javascript & Dartium. Updating Javascript resources. Manifest.json & pub.html are updated in app/. Added another git submodule for app/js/jszip. R=tapted@chromium.org Committed: https://github.com/tapted/bleeding_edge/commit/946dab3

Patch Set 1 #

Total comments: 12

Patch Set 2 : Adding make step to copy jszip to app/js. #

Patch Set 3 : Caught restoreEntry failure, and removed a compile warning. #

Patch Set 4 : No changes #

Patch Set 5 : Rebasing #

Created: 10 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+32 lines, -15 lines)			Patch
A	dart/sdk/lib/_internal/pub/.gitignore	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	dart/sdk/lib/_internal/pub/app/Makefile	View	1	1 chunk	+4 lines, -1 line	0 comments	Download
A +	dart/sdk/lib/_internal/pub/app/js/archive.js	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	dart/sdk/lib/_internal/pub/app/manifest.json	View		1 chunk	+5 lines, -1 line	0 comments	Download
M	dart/sdk/lib/_internal/pub/app/pub.html	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	dart/sdk/lib/_internal/pub/lib/src/io.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	dart/sdk/lib/_internal/pub/lib/src/pubspec.dart	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart	View	1 2	3 chunks	+10 lines, -9 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

tapted

https://codereview.appspot.com/50240043/diff/1/.gitmodules File .gitmodules (right): https://codereview.appspot.com/50240043/diff/1/.gitmodules#newcode4 .gitmodules:4: [submodule "dart/sdk/lib/_internal/pub/app/js/jszip"] I think it would be better just ...

10 years, 3 months ago (2014-01-10 01:10:03 UTC) #2

pajamallama

Some questions about the comments. https://codereview.appspot.com/50240043/diff/1/.gitmodules File .gitmodules (right): https://codereview.appspot.com/50240043/diff/1/.gitmodules#newcode4 .gitmodules:4: [submodule "dart/sdk/lib/_internal/pub/app/js/jszip"] On 2014/01/10 ...

10 years, 3 months ago (2014-01-10 02:01:01 UTC) #3

tapted

10 years, 3 months ago (2014-01-10 02:30:15 UTC) #4

https://codereview.appspot.com/50240043/diff/1/.gitmodules
File .gitmodules (right):

https://codereview.appspot.com/50240043/diff/1/.gitmodules#newcode4
.gitmodules:4: [submodule "dart/sdk/lib/_internal/pub/app/js/jszip"]
On 2014/01/10 02:01:02, pajamallama wrote:
> On 2014/01/10 01:10:04, tapted wrote:
> > I think it would be better just to (approximately) `cp -r` js/jszip to
> > app/js/jszip as a step in `make app`. I.e. just replicate what's done by the
> > `tar hc packages` voodoo
> 
> And then add the app/js/jszip to the .gitignore?

yep!

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/app...
File dart/sdk/lib/_internal/pub/app/js/archive.js (right):

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/app...
dart/sdk/lib/_internal/pub/app/js/archive.js:51: if (data.o != null)
On 2014/01/10 02:01:02, pajamallama wrote:
> On 2014/01/10 01:10:04, tapted wrote:
> > `if` -> `while`?
> 
> I don't think I've seen DartObject be wrapped in DartObjects before.. Would it
> be necessary?

Ah. oops - I thought dest was also `data`

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:46: return
chrome.fileSystem.restoreEntry(map[STORAGE_KEY]).then(
On 2014/01/10 02:01:02, pajamallama wrote:
> On 2014/01/10 01:10:04, tapted wrote:
> > does this need to handle failure? I think retained entries can be removed
via
> > chrome://extensions
> 
> It already does handle failure, if a valid file id string isn't found then
> obtainDirectory() is called further down.

I see that if map[STORAGE_KEY] is not a string, then there's no return from this
closure (I guess it defaults to null when there is no return - assuming that's
intentional ;)

But restoreEntry [1] says "This call will fail with a runtime error [when it
fails].". Where does that error go? (maybe try removing the retained file entry
via the permissions dialog at chrome://extensions - I think that's where the UI
for it is.)

[1] http://developer.chrome.com/apps/fileSystem.html#method-restoreEntry

pajamallama

10 years, 3 months ago (2014-01-12 23:27:17 UTC) #5

https://codereview.appspot.com/50240043/diff/1/.gitmodules
File .gitmodules (right):

https://codereview.appspot.com/50240043/diff/1/.gitmodules#newcode4
.gitmodules:4: [submodule "dart/sdk/lib/_internal/pub/app/js/jszip"]
On 2014/01/10 02:30:15, tapted wrote:
> On 2014/01/10 02:01:02, pajamallama wrote:
> > On 2014/01/10 01:10:04, tapted wrote:
> > > I think it would be better just to (approximately) `cp -r` js/jszip to
> > > app/js/jszip as a step in `make app`. I.e. just replicate what's done by
the
> > > `tar hc packages` voodoo
> > 
> > And then add the app/js/jszip to the .gitignore?
> 
> yep!

Done, I rebased and removed this extra submodule but the fix isn't showing up
here.

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/app...
File dart/sdk/lib/_internal/pub/app/js/archive.js (right):

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/app...
dart/sdk/lib/_internal/pub/app/js/archive.js:51: if (data.o != null)
On 2014/01/10 02:30:15, tapted wrote:
> On 2014/01/10 02:01:02, pajamallama wrote:
> > On 2014/01/10 01:10:04, tapted wrote:
> > > `if` -> `while`?
> > 
> > I don't think I've seen DartObject be wrapped in DartObjects before.. Would
it
> > be necessary?
> 
> Ah. oops - I thought dest was also `data`

Cool, done.

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/lib...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/50240043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:46: return
chrome.fileSystem.restoreEntry(map[STORAGE_KEY]).then(
On 2014/01/10 02:30:15, tapted wrote:
> On 2014/01/10 02:01:02, pajamallama wrote:
> > On 2014/01/10 01:10:04, tapted wrote:
> > > does this need to handle failure? I think retained entries can be removed
> via
> > > chrome://extensions
> > 
> > It already does handle failure, if a valid file id string isn't found then
> > obtainDirectory() is called further down.
> 
> I see that if map[STORAGE_KEY] is not a string, then there's no return from
this
> closure (I guess it defaults to null when there is no return - assuming that's
> intentional ;)
> 
> But restoreEntry [1] says "This call will fail with a runtime error [when it
> fails].". Where does that error go? (maybe try removing the retained file
entry
> via the permissions dialog at chrome://extensions - I think that's where the
UI
> for it is.)
> 
> [1] http://developer.chrome.com/apps/fileSystem.html#method-restoreEntry

Return null is accidentally intentional lol. Handled restoreEntry failure. Done.

Message was sent while issue was closed.

Committed patchset #5 manually as r87498f4.

Expand All Messages | Collapse All Messages