Changed the directory structure of the project.

Change the directory structure of the project.

Moved the js/ directory into app/js and test/js. When the tests
are compiled then the app/js contents will be copied to out/test/js
(along with test/js).

Cleaned up a couple parts of the Makefile (more to come).

Also moved third_party/qunit-1-* into test/deps/qunit/qunit-1-*.
When my package manager 'tavern' is complete then it will place
all external dependencies (like qunit) in a similar structure
(at least symlinks to a local cache). This will also have the
interpretation that external code is separated from internal code.

Both the app/ and test/ html files will now reference 'bitmapper.js' which
is the compiled output of the necessary JavaScript files. This means that
neither app/ nor test/ can be loaded as a packaged app, they must first
be compiled to out/app or out/test.

Moved closure compiler to the build/ directory. Later build/ will store
the closure compiler, the application's externs.js file, and also
a chrome_extensions.js (an externs file for the chrome.* APIs). Also this
means the compiler.jar will not be removed on clean (and clean can
be rm -rf out/ ). The .gitignore has been updated to handle the contents of
build/ that should not be under source control.

[pajamallama, 2013-11-27 23:22:52]

The first split-up commit after rewriting the Makefile.

[Matt Giuca, 2013-11-27 23:39:03]

Good change, nice and clean this time.

A couple of nits.

Also, could you please make the change description more verbose? It's ok to
write a paragraph or more if you need to, just keep it wrapped to 72 characters.
(Keep the top line as the summary, but then have a blank line followed by
details.) Please briefly summarize what you are moving, e.g., "Moved app files
into "app/js", test files into "test/js", etc. Also a quick comment like "Also,
cleaned up makefile."

https://codereview.appspot.com/34470043/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode6
Makefile:6: BUILDDIR := build
Is BUILDDIR just for the closure compiler? It doesn't look like any built
targets actually go in there. In which case why is it called "build"?

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode24
Makefile:24: # Compilation_level options:
Why did this comment disappear? I think it was useful advice for changing the
--compilation_level flag.

Also, while you're here, can you change "Compilation_level" to
"--compilation_level" so it's clear that these options are for that flag.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode99
Makefile:99: curl $(CLOSURE_URL) -o $(BUILDDIR)/compiler-latest.zip
What if BUILDDIR doesn't exist? Should you mkdir -p $(BUILDDIR) first?

https://codereview.appspot.com/34470043/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.appspot.com/34470043/diff/1/codereview.settings#newcode1
codereview.settings:1: CODE_REVIEW_SERVER: codereview.appspot.com
Please revert this change (calamity is doing it in another CL).

[calamity, 2013-11-28 00:23:16]

I thought we were going to leave the structure so that js/ was in the root,
letting us do source maps nicely?

https://codereview.appspot.com/34470043/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode59
Makefile:59: cp -r $(APPDIR)/* $(OUTDIR)/$(APPDIR)
Any reason to change this from $^?

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode79
Makefile:79: cp -r $(TESTDIR)/* $(OUTDIR)/$(TESTDIR)
Same here.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode80
Makefile:80: cp $(OUT_SRCS) $(OUTDIR)/$(TESTDIR)/$(SRCDIR)
Can we copy from $(APP_SRCS) instead?

[pajamallama, 2013-11-28 00:36:17]

I misinterpreted your suggestion: I thought you'd meant to copy js/ folders
up to current directory for compilation and copy them back afterwards.

Instead I chose the "cd out/Debug/ &&"  route

        cd $(OUT_DIR)/$(TEST_DIR) && \
        $(CLOSURE) $(CLOSURE_ARGS) \
                $(patsubst %, --js $(SRC_DIR)/%, $(notdir $(ALL_SRCS))) \
                --js_output_file $(PROJECT).js \
                --create_source_map $(PROJECT).js.map

Attached is the Makefile that I'm trying to commit my way up to.

Regards,


Pat


On Thu, Nov 28, 2013 at 11:23 AM, <calamity@chromium.org> wrote:

> I thought we were going to leave the structure so that js/ was in the
> root, letting us do source maps nicely?
>
>
>
> https://codereview.appspot.com/34470043/diff/1/Makefile
> File Makefile (right):
>
> https://codereview.appspot.com/34470043/diff/1/Makefile#newcode59
> Makefile:59: cp -r $(APPDIR)/* $(OUTDIR)/$(APPDIR)
> Any reason to change this from $^?
>
> https://codereview.appspot.com/34470043/diff/1/Makefile#newcode79
> Makefile:79: cp -r $(TESTDIR)/* $(OUTDIR)/$(TESTDIR)
> Same here.
>
> https://codereview.appspot.com/34470043/diff/1/Makefile#newcode80
> Makefile:80: cp $(OUT_SRCS) $(OUTDIR)/$(TESTDIR)/$(SRCDIR)
> Can we copy from $(APP_SRCS) instead?
>
> https://codereview.appspot.com/34470043/
>

[pajamallama, 2013-11-28 01:37:27]

Also Chris, to answer some other points you raised.

I hoped to remove the $< and $@ references that were scattered throughout
the original
Makefile. I felt it made the Makefile more unreadable, though I can put
them all back in
again if we all want.

Also, copying $(APP_SRCS) instead of $(OUT_SRCS) is something that I do in
the final
version that Jack didn't do. I forgot to change it here but I'll put that
in the next commit.

Regards,


Pat


On Thu, Nov 28, 2013 at 11:36 AM, Patrick Laub <pajamallama@google.com>wrote:

> I misinterpreted your suggestion: I thought you'd meant to copy js/
> folders up to current directory for compilation and copy them back
> afterwards.
>
> Instead I chose the "cd out/Debug/ &&"  route
>
>         cd $(OUT_DIR)/$(TEST_DIR) && \
>         $(CLOSURE) $(CLOSURE_ARGS) \
>                 $(patsubst %, --js $(SRC_DIR)/%, $(notdir $(ALL_SRCS))) \
>                 --js_output_file $(PROJECT).js \
>                 --create_source_map $(PROJECT).js.map
>
> Attached is the Makefile that I'm trying to commit my way up to.
>
> Regards,
>
>
> Pat
>
>
> On Thu, Nov 28, 2013 at 11:23 AM, <calamity@chromium.org> wrote:
>
>> I thought we were going to leave the structure so that js/ was in the
>> root, letting us do source maps nicely?
>>
>>
>>
>> https://codereview.appspot.com/34470043/diff/1/Makefile
>> File Makefile (right):
>>
>> https://codereview.appspot.com/34470043/diff/1/Makefile#newcode59
>> Makefile:59: cp -r $(APPDIR)/* $(OUTDIR)/$(APPDIR)
>> Any reason to change this from $^?
>>
>> https://codereview.appspot.com/34470043/diff/1/Makefile#newcode79
>> Makefile:79: cp -r $(TESTDIR)/* $(OUTDIR)/$(TESTDIR)
>> Same here.
>>
>> https://codereview.appspot.com/34470043/diff/1/Makefile#newcode80
>> Makefile:80: cp $(OUT_SRCS) $(OUTDIR)/$(TESTDIR)/$(SRCDIR)
>> Can we copy from $(APP_SRCS) instead?
>>
>> https://codereview.appspot.com/34470043/
>>
>
>

[calamity, 2013-11-28 01:38:37]

On 2013/11/28 00:36:17, pajamallama wrote:
> I misinterpreted your suggestion: I thought you'd meant to copy js/ folders
> up to current directory for compilation and copy them back afterwards.
> 
> Instead I chose the "cd out/Debug/ &&"  route
> 
>         cd $(OUT_DIR)/$(TEST_DIR) && \
>         $(CLOSURE) $(CLOSURE_ARGS) \
>                 $(patsubst %, --js $(SRC_DIR)/%, $(notdir $(ALL_SRCS))) \
>                 --js_output_file $(PROJECT).js \
>                 --create_source_map $(PROJECT).js.map
> 
> Attached is the Makefile that I'm trying to commit my way up to.
> 
> Regards,
> 
> 
> Pat
> 
> 
> On Thu, Nov 28, 2013 at 11:23 AM, <mailto:calamity@chromium.org> wrote:
> 
> > I thought we were going to leave the structure so that js/ was in the
> > root, letting us do source maps nicely?
> >
> >
> >
> > https://codereview.appspot.com/34470043/diff/1/Makefile
> > File Makefile (right):
> >
> > https://codereview.appspot.com/34470043/diff/1/Makefile#newcode59
> > Makefile:59: cp -r $(APPDIR)/* $(OUTDIR)/$(APPDIR)
> > Any reason to change this from $^?
> >
> > https://codereview.appspot.com/34470043/diff/1/Makefile#newcode79
> > Makefile:79: cp -r $(TESTDIR)/* $(OUTDIR)/$(TESTDIR)
> > Same here.
> >
> > https://codereview.appspot.com/34470043/diff/1/Makefile#newcode80
> > Makefile:80: cp $(OUT_SRCS) $(OUTDIR)/$(TESTDIR)/$(SRCDIR)
> > Can we copy from $(APP_SRCS) instead?
> >
> > https://codereview.appspot.com/34470043/
> >

Ah ok. cd-ing to the correct directories to get the right relative paths sgtm.

[pajamallama, 2013-11-28 01:52:28]

Matt: I've redone this commit with more doc explaining what it achieves. Will
upload it next.

https://codereview.appspot.com/34470043/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode6
Makefile:6: BUILDDIR := build
On 2013/11/27 23:39:04, Matt Giuca wrote:
> Is BUILDDIR just for the closure compiler? It doesn't look like any built
> targets actually go in there. In which case why is it called "build"?

It will store the closure compiler, the user's externs file, and also a curl'ed
chrome_extensions.js file which has the externs for the chrome.* APIs. No
building is done in there but it means that removing the out/ directory does not
delete the closure compiler.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode24
Makefile:24: # Compilation_level options:
On 2013/11/27 23:39:04, Matt Giuca wrote:
> Why did this comment disappear? I think it was useful advice for changing the
> --compilation_level flag.
> 
> Also, while you're here, can you change "Compilation_level" to
> "--compilation_level" so it's clear that these options are for that flag.

Done.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode59
Makefile:59: cp -r $(APPDIR)/* $(OUTDIR)/$(APPDIR)
On 2013/11/28 00:23:17, calamity wrote:
> Any reason to change this from $^?

I hoped to remove the $< and $@ references that were scattered throughout the
original 
Makefile. I felt it made the Makefile more unreadable, though I can put them all
back in
again if we all want.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode79
Makefile:79: cp -r $(TESTDIR)/* $(OUTDIR)/$(TESTDIR)
On 2013/11/28 00:23:17, calamity wrote:
> Same here.

Done.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode80
Makefile:80: cp $(OUT_SRCS) $(OUTDIR)/$(TESTDIR)/$(SRCDIR)
On 2013/11/28 00:23:17, calamity wrote:
> Can we copy from $(APP_SRCS) instead?

Yes, and that's what I do in the final Makefile, just forgot to put it in here.

https://codereview.appspot.com/34470043/diff/1/Makefile#newcode99
Makefile:99: curl $(CLOSURE_URL) -o $(BUILDDIR)/compiler-latest.zip
On 2013/11/27 23:39:04, Matt Giuca wrote:
> What if BUILDDIR doesn't exist? Should you mkdir -p $(BUILDDIR) first?

Done.

https://codereview.appspot.com/34470043/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.appspot.com/34470043/diff/1/codereview.settings#newcode1
codereview.settings:1: CODE_REVIEW_SERVER: codereview.appspot.com
On 2013/11/27 23:39:04, Matt Giuca wrote:
> Please revert this change (calamity is doing it in another CL).

Done.

[pajamallama, 2013-11-28 02:18:12]

Fixed issues noted by Matt & Chris.

Also (semi-accidentally) fixes some issues with the
imageFile.js and main.js scripts. A type definition
was wrong and it also generated duplicate license 
comments in compiled output.

https://codereview.appspot.com/34470043/diff/20001/.gitignore
File .gitignore (right):

https://codereview.appspot.com/34470043/diff/20001/.gitignore#newcode5
.gitignore:5: build/compiler.jar
Don't have files that are curl'd from a URL in source control.

https://codereview.appspot.com/34470043/diff/20001/app/js/imageFile.js
File app/js/imageFile.js (right):

https://codereview.appspot.com/34470043/diff/20001/app/js/imageFile.js#newcode2
app/js/imageFile.js:2: * Copyright 2013 Google Inc. All rights reserved.
Removing all but one @license so that the compiled javascript only shows one
copyright notice and the code.

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js
File app/js/main.js (right):

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js#newcode32
app/js/main.js:32: * @param {Entry=} entry
Fixing some types so that closure & chrome_extensions.js don't conflict.

[pajamallama, 2013-11-28 03:29:39]

Simplified the Makefile and the targets.

Removes the need to list every source file in the Makefile (it will instead use
the contents of app/js/ and test/js).

Removes the OUT_* targets, now make will copy app/* and test/* into out/.

Compiling now doesn't run gjslint. The lint target is for that purpose.

Added clean target.

Removed console.log definition from externs.js to fix a compile error.

https://codereview.appspot.com/34470043/diff/40001/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/40001/Makefile#newcode26
Makefile:26: CLOSURE_COMPILER := $(CD)/$(BUILD_DIR)/compiler.jar
Using an absolute path is required as now the compiler is being called from a
different directory.

[Matt Giuca, 2013-11-28 04:30:31]

Re the change description: You don't need to describe the individual patch sets
in the description (since they will all be squashed down together when this is
finally committed). It should just be a summary of the final patch set (against
master).

Also, you can remove the PATCH 3 part, since you postponed that to a follow-up
CL.

Thanks for making the description longer. I would say that even just your patch
1 & 2 part is a little too long (usually you would write 1 lead line + 1
optional paragraph). You don't need to change it again, but just FYI, it doesn't
need to be this detailed.

A couple of nits then we should be good to go. Thanks!

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js
File app/js/main.js (right):

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js#newcode32
app/js/main.js:32: * @param {Entry=} entry
What's the = for? Is that part of the type name? Can you find a different name
that doesn't include an =?

https://codereview.appspot.com/34470043/diff/100001/.gitignore
File .gitignore (right):

https://codereview.appspot.com/34470043/diff/100001/.gitignore#newcode10
.gitignore:10: 
nit: Remove empty line.

https://codereview.appspot.com/34470043/diff/100001/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/100001/Makefile#newcode24
Makefile:24: # Compilation_level options:
Remove this line.

https://codereview.appspot.com/34470043/diff/100001/Makefile#newcode27
Makefile:27: # Closure compiler and arguments.
Sorry to be a nit picky ninny, but could you move these two comment lines down
so they appear immediately above CLOSURE_ARGS?

https://codereview.appspot.com/34470043/diff/100001/Makefile#newcode96
Makefile:96: mkdir $(BUILDDIR)
Needs mkdir -p. Otherwise, if it already exists it will be an error.

https://codereview.appspot.com/34470043/diff/100001/app/js/imageFile.js
File app/js/imageFile.js (right):

https://codereview.appspot.com/34470043/diff/100001/app/js/imageFile.js#newco...
app/js/imageFile.js:13: * @param {function()} callback
Why did the param type disappear?

[pajamallama, 2013-11-28 04:48:58]

Fixed

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js
File app/js/main.js (right):

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js#newcode32
app/js/main.js:32: * @param {Entry=} entry
On 2013/11/28 04:30:32, Matt Giuca wrote:
> What's the = for? Is that part of the type name? Can you find a different name
> that doesn't include an =?

It shows that Entry is an optional argument. chrome.fileSystem.chooseEntry
actually has two optional arguments, either an Entry, or an Array<Entry>, and it
will use either one.

https://codereview.appspot.com/34470043/diff/100001/.gitignore
File .gitignore (right):

https://codereview.appspot.com/34470043/diff/100001/.gitignore#newcode10
.gitignore:10: 
On 2013/11/28 04:30:32, Matt Giuca wrote:
> nit: Remove empty line.

Done.

https://codereview.appspot.com/34470043/diff/100001/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/100001/Makefile#newcode24
Makefile:24: # Compilation_level options:
On 2013/11/28 04:30:32, Matt Giuca wrote:
> Remove this line.

Done.

https://codereview.appspot.com/34470043/diff/100001/Makefile#newcode27
Makefile:27: # Closure compiler and arguments.
On 2013/11/28 04:30:32, Matt Giuca wrote:
> Sorry to be a nit picky ninny, but could you move these two comment lines down
> so they appear immediately above CLOSURE_ARGS?

Done.

https://codereview.appspot.com/34470043/diff/100001/Makefile#newcode96
Makefile:96: mkdir $(BUILDDIR)
On 2013/11/28 04:30:32, Matt Giuca wrote:
> Needs mkdir -p. Otherwise, if it already exists it will be an error.

Done.

https://codereview.appspot.com/34470043/diff/100001/app/js/imageFile.js
File app/js/imageFile.js (right):

https://codereview.appspot.com/34470043/diff/100001/app/js/imageFile.js#newco...
app/js/imageFile.js:13: * @param {function()} callback
On 2013/11/28 04:30:32, Matt Giuca wrote:
> Why did the param type disappear?

I think this function is called later with no arguments (which gave a compile
error).. This is more Devina/Marie's problem tho lol.

[Matt Giuca, 2013-11-28 04:51:57]

LGTM

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js
File app/js/main.js (right):

https://codereview.appspot.com/34470043/diff/20001/app/js/main.js#newcode32
app/js/main.js:32: * @param {Entry=} entry
Oh OK, sorry.

https://codereview.appspot.com/34470043/diff/100001/app/js/imageFile.js
File app/js/imageFile.js (right):

https://codereview.appspot.com/34470043/diff/100001/app/js/imageFile.js#newco...
app/js/imageFile.js:13: * @param {function()} callback
OK.

[Matt Giuca, 2013-11-28 05:07:32]

Nit: Get rid of the second double blank line in the CL description :)

[calamity, 2013-11-28 05:48:02]

lgtm

https://codereview.appspot.com/34470043/diff/110001/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/110001/Makefile#newcode95
Makefile:95: # Download the closure compiler.
nit: trailing space

[Matt Giuca, 2013-11-28 05:57:27]

https://codereview.appspot.com/34470043/diff/110001/Makefile
File Makefile (right):

https://codereview.appspot.com/34470043/diff/110001/Makefile#newcode95
Makefile:95: # Download the closure compiler.
On 2013/11/28 05:48:02, calamity wrote:
> nit: trailing space

Done.

[Matt Giuca, 2013-11-28 06:10:18]

Change committed as git commit e2cf0a5.