Extension point for WorkManagerFactory(s).

Extension point for WorkManagerFactory(s).

I'm not quite happy that internal classes get into the extension point declaration.
But I guess that because of the nature of the work WorkManager(s) are doing, we have to have significant exposure to internals.

Also, onAnalysisOptionsChanged() and onSourceFactoryChanged()... these probably are better to implement as streams in InternalAnalysisContext.
Thoughts?

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

Committed: https://github.com/dart-lang/sdk/commit/597f189eb45af457c72ffa2b4e140a21f43c1914

[Paul Berry, 2015-08-24 20:13:55]

lgtm.  I don't really have a strong opinion about onAnalysisOptionsChanged() and
onSourceFactoryChanged().

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:814: for (WorkManager workManager in
workManagers) {
It seems unfortunate to have to create a loop here.  Would it be better to have
each WorkManager advertise what kinds of file extensions it supports so that we
can direct the getErrors() request to a single work manager?

Or would that be too limiting (i.e. maybe for some reason I don't understand it
would make sense for multiple work managers to support overlapping sets of file
extensions)?

[scheglov, 2015-08-24 20:21:10]

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:814: for (WorkManager workManager in
workManagers) {
On 2015/08/24 20:13:55, Paul Berry wrote:
> It seems unfortunate to have to create a loop here.  Would it be better to
have
> each WorkManager advertise what kinds of file extensions it supports so that
we
> can direct the getErrors() request to a single work manager?
> 
> Or would that be too limiting (i.e. maybe for some reason I don't understand
it
> would make sense for multiple work managers to support overlapping sets of
file
> extensions)?

Yes, actually the Angular plugin will report errors for both Dart and HTML
files.
So, we need to combine errors from DartWorkManager and AngularWorkManager for
Dart files (Angular specific warnings for Dart code and inline HTML template);
and HtmlWorkManager and AngularWorkManager for HTML files. I guess any
additional linter will work the same way.

Each WorkManager can filter which files it supports, and should not be
expensive, or at least not much more expensive than testing it externally.

[Paul Berry, 2015-08-24 20:29:28]

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:814: for (WorkManager workManager in
workManagers) {
On 2015/08/24 20:21:10, scheglov wrote:
> On 2015/08/24 20:13:55, Paul Berry wrote:
> > It seems unfortunate to have to create a loop here.  Would it be better to
> have
> > each WorkManager advertise what kinds of file extensions it supports so that
> we
> > can direct the getErrors() request to a single work manager?
> > 
> > Or would that be too limiting (i.e. maybe for some reason I don't understand
> it
> > would make sense for multiple work managers to support overlapping sets of
> file
> > extensions)?
> 
> Yes, actually the Angular plugin will report errors for both Dart and HTML
> files.
> So, we need to combine errors from DartWorkManager and AngularWorkManager for
> Dart files (Angular specific warnings for Dart code and inline HTML template);
> and HtmlWorkManager and AngularWorkManager for HTML files. I guess any
> additional linter will work the same way.
> 
> Each WorkManager can filter which files it supports, and should not be
> expensive, or at least not much more expensive than testing it externally.

Ok, sounds reasonable.

[Brian Wilkerson, 2015-08-24 21:43:09]

LGTM

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/plugin/tas...
File pkg/analyzer/lib/plugin/task.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/plugin/tas...
pkg/analyzer/lib/plugin/task.dart:13: import
'package:analyzer/src/task/driver.dart' show WorkManager;
The interface WorkManager needs to be moved into the public API, either in
pkg/analyzer/lib/task/model.dart or in a new file, because clients need to
implement it. That means that TargetedResult and WorkOrderPriority should also
become public. (I'd leave AnalysisError in src for now because it's too likely
to be a breaking change to move it.)

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:131: DartWorkManager dartWorkManager;
Why does the dart work manager need to be special?

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:5: library engine;
If there are any non-formatting changes in this file, I missed seeing them.

[scheglov, 2015-08-26 15:43:07]

PTAL

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/plugin/tas...
File pkg/analyzer/lib/plugin/task.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/plugin/tas...
pkg/analyzer/lib/plugin/task.dart:13: import
'package:analyzer/src/task/driver.dart' show WorkManager;
On 2015/08/24 21:43:09, Brian Wilkerson wrote:
> The interface WorkManager needs to be moved into the public API, either in
> pkg/analyzer/lib/task/model.dart or in a new file, because clients need to
> implement it. That means that TargetedResult and WorkOrderPriority should also
> become public. (I'd leave AnalysisError in src for now because it's too likely
> to be a breaking change to move it.)

Done.

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1311773005/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:131: DartWorkManager dartWorkManager;
On 2015/08/24 21:43:09, Brian Wilkerson wrote:
> Why does the dart work manager need to be special?

It is used 4 times.

1. In computeResolvedCompilationUnitAsync():
dartWorkManager.addPriorityResult(unitTarget, RESOLVED_UNIT);

2. In ensureResolvedDartUnits(): dartWorkManager.addPriorityResult(target,
RESOLVED_UNIT);

3. In getLibrariesContaining():
dartWorkManager.getLibrariesContainingPart(source);

4. In _tryPoorMansIncrementalResolution():
dartWorkManager.unitIncrementallyResolved(librarySource, unitSource);


In theory we could add WorkManager.addPriorityResult() to the WorkManager API,
so "unspecialize" items 1 and 2.

But 3 and 4 are still very Dart specific.
It is probably a sign that our AnalysisContext is historically specialized on
Dart.

[Brian Wilkerson, 2015-08-26 16:19:32]

LGTM

https://codereview.chromium.org/1311773005/diff/20001/pkg/analyzer/lib/task/m...
File pkg/analyzer/lib/task/model.dart (right):

https://codereview.chromium.org/1311773005/diff/20001/pkg/analyzer/lib/task/m...
pkg/analyzer/lib/task/model.dart:395: * A specification of the given [result]
for the given [target].
We should probably include the boilerplate for "clients are not expected to
subtype this class".

https://codereview.chromium.org/1311773005/diff/20001/pkg/analyzer/lib/task/m...
pkg/analyzer/lib/task/model.dart:564: * so they can request analysis results in
optimal order.
We should also have boilerplate here saying "only implement" this class.

https://codereview.chromium.org/1311773005/diff/20001/pkg/analyzer/lib/task/m...
pkg/analyzer/lib/task/model.dart:621: * The priorities of work orders returned
by [WorkManager]s.
Should we have a disclaimer about clients needing to tolerate additions to the
priorities so that we can change it without it being a breaking change, or are
we confident that the current list of priorities is sufficient?

[scheglov, 2015-08-26 16:45:24]

Committed patchset #2 (id:20001) manually as
597f189eb45af457c72ffa2b4e140a21f43c1914 (presubmit successful).