Support for pubspec based contexts in subfolders.

Support for pubspec based contexts in subfolders.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=39036

[Brian Wilkerson, 2014-08-08 14:00:03]

LGTM

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:41: * The [path.Context] for
this manager.
I assume "path" was suppose to be "pathos".

As worded, this comment really doesn't tell me anything. It would be better if
you described what it's used for, such as

The context used to ...

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:113: Set<Folder> currentFolders
= _contexts.keys.toSet();
Are we making a copy to avoid modifying the keys while we're iterating over
them? I can't see where that would happen, but I might be missing something.

[scheglov, 2014-08-08 15:54:33]

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:41: * The [path.Context] for
this manager.
On 2014/08/08 14:00:02, Brian Wilkerson wrote:
> I assume "path" was suppose to be "pathos".
> 
> As worded, this comment really doesn't tell me anything. It would be better if
> you described what it's used for, such as
> 
> The context used to ...

Done.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:113: Set<Folder> currentFolders
= _contexts.keys.toSet();
On 2014/08/08 14:00:02, Brian Wilkerson wrote:
> Are we making a copy to avoid modifying the keys while we're iterating over
> them? I can't see where that would happen, but I might be missing something.

We use it twice in the code below.

[scheglov, 2014-08-08 15:55:12]

Committed patchset #2 manually as 39036 (presubmit successful).

[Paul Berry, 2014-08-08 16:07:10]

lgtm

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:113: Set<Folder> currentFolders
= _contexts.keys.toSet();
On 2014/08/08 14:00:02, Brian Wilkerson wrote:
> Are we making a copy to avoid modifying the keys while we're iterating over
> them? I can't see where that would happen, but I might be missing something.

Agreed, but if we go with my suggestions below, then the keys will be modified
while we're iterating over them, so I'd say let's keep this line as is.

One nit, though: the name "currentFolders" isn't very meaningful to me--I keep
forgetting that it is the set of folders associated with current *contexts*. 
How about if we call it "contextFolders"?

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:121:
oldFolders.add(currentFolder);
I think it would be clearer to just do:

  _destroyContext(currentFolder);

immediately, and drop the "oldFolders" set.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:129:
newFolders.add(includedFolder);
Similarly, I think it would be clearer to just to:

  _createContexts(includedFolder, false);

immediately, and drop the "newFolders" set.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:188: */
Can you document the meaning of "withPubspecOnly" here?

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:232: Map<String, Source>
extractSources = new HashMap<String, Source>();
Rename to "extractedSources"

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:361: _addSourceFiles(changeSet,
info.folder, parentInfo);
It looks like this will re-read the directory contents.  Would it be better to
just extract the sources from the old context and merge them into the parent, as
we do in _extractContext?

[scheglov, 2014-08-08 16:34:23]

Comments addressed in https://codereview.chromium.org/454063002

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/context_manager.dart (right):

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:113: Set<Folder> currentFolders
= _contexts.keys.toSet();
On 2014/08/08 16:07:10, Paul Berry wrote:
> On 2014/08/08 14:00:02, Brian Wilkerson wrote:
> > Are we making a copy to avoid modifying the keys while we're iterating over
> > them? I can't see where that would happen, but I might be missing something.
> 
> Agreed, but if we go with my suggestions below, then the keys will be modified
> while we're iterating over them, so I'd say let's keep this line as is.
> 
> One nit, though: the name "currentFolders" isn't very meaningful to me--I keep
> forgetting that it is the set of folders associated with current *contexts*. 
> How about if we call it "contextFolders"?

Done.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:121:
oldFolders.add(currentFolder);
On 2014/08/08 16:07:10, Paul Berry wrote:
> I think it would be clearer to just do:
> 
>   _destroyContext(currentFolder);
> 
> immediately, and drop the "oldFolders" set.

Done.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:129:
newFolders.add(includedFolder);
On 2014/08/08 16:07:10, Paul Berry wrote:
> Similarly, I think it would be clearer to just to:
> 
>   _createContexts(includedFolder, false);
> 
> immediately, and drop the "newFolders" set.

Done.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:188: */
On 2014/08/08 16:07:10, Paul Berry wrote:
> Can you document the meaning of "withPubspecOnly" here?

Done.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:232: Map<String, Source>
extractSources = new HashMap<String, Source>();
On 2014/08/08 16:07:10, Paul Berry wrote:
> Rename to "extractedSources"

Done.

https://codereview.chromium.org/450103002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_manager.dart:361: _addSourceFiles(changeSet,
info.folder, parentInfo);
On 2014/08/08 16:07:10, Paul Berry wrote:
> It looks like this will re-read the directory contents.  Would it be better to
> just extract the sources from the old context and merge them into the parent,
as
> we do in _extractContext?

Done.