Element.fromEngine() constructor.

Element.fromEngine() constructor.

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2014-06-25 13:50:08]

LGTM

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/computer/element.dart (right):

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/element.dart:122: var sb = new
StringBuffer();
This duplicates code in engine.ExecutableElement. We should extract the code in
engine.ExecutableElement so that we have a single implementation of it.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/element.dart:158: if (element is
engine.ClassElement) {
Code with lots of 'is' expressions is always suspect, even though it's sometimes
the better option. But in this case I think it makes sense to define
engine.Element.isAbstract() to return 'false' so that we can use polymorphism
rather than 'is' tests. Similarly for isConst, isFinal, and isStatic.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/element.dart:245: static ElementKind
valueOfEngine(engine.ElementKind kind) {
What's the value of having two implementations of ElementKind?

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
File pkg/analysis_server/test/computer/element_test.dart (right):

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
pkg/analysis_server/test/computer/element_test.dart:38:
expect(ElementKind.valueOf('CLASS'), ElementKind.CLASS);
Wouldn't this be better as

expect(ElementKind.valueOf(ElementKind.CLASS.name), ElementKind.CLASS);

so that if the names change the test doesn't have to?

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
pkg/analysis_server/test/computer/element_test.dart:55:
expect(ElementKind.valueOf('CLASS'), ElementKind.CLASS);
Duplicate.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
pkg/analysis_server/test/computer/element_test.dart:93: engine.ClassElement
engineElement = new MockClassElement();
Given how light-weight elements are, why not just use ElementFactory and create
real instances?

[scheglov, 2014-06-25 17:20:47]

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/computer/element.dart (right):

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/element.dart:122: var sb = new
StringBuffer();
On 2014/06/25 13:50:08, Brian Wilkerson wrote:
> This duplicates code in engine.ExecutableElement. We should extract the code
in
> engine.ExecutableElement so that we have a single implementation of it.

I've added TODO.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/element.dart:158: if (element is
engine.ClassElement) {
On 2014/06/25 13:50:08, Brian Wilkerson wrote:
> Code with lots of 'is' expressions is always suspect, even though it's
sometimes
> the better option. But in this case I think it makes sense to define
> engine.Element.isAbstract() to return 'false' so that we can use polymorphism
> rather than 'is' tests. Similarly for isConst, isFinal, and isStatic.

I've added TODO.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/element.dart:245: static ElementKind
valueOfEngine(engine.ElementKind kind) {
On 2014/06/25 13:50:08, Brian Wilkerson wrote:
> What's the value of having two implementations of ElementKind?

Well, there isn't.
But ElementKind here is the one the server API defines.
Plus UNIT_TEST_CASE is not a part of Engine's ElementKind.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
File pkg/analysis_server/test/computer/element_test.dart (right):

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
pkg/analysis_server/test/computer/element_test.dart:38:
expect(ElementKind.valueOf('CLASS'), ElementKind.CLASS);
On 2014/06/25 13:50:08, Brian Wilkerson wrote:
> Wouldn't this be better as
> 
> expect(ElementKind.valueOf(ElementKind.CLASS.name), ElementKind.CLASS);
> 
> so that if the names change the test doesn't have to?

Done.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
pkg/analysis_server/test/computer/element_test.dart:55:
expect(ElementKind.valueOf('CLASS'), ElementKind.CLASS);
On 2014/06/25 13:50:08, Brian Wilkerson wrote:
> Duplicate.

Done.

https://codereview.chromium.org/356563006/diff/1/pkg/analysis_server/test/com...
pkg/analysis_server/test/computer/element_test.dart:93: engine.ClassElement
engineElement = new MockClassElement();
On 2014/06/25 13:50:08, Brian Wilkerson wrote:
> Given how light-weight elements are, why not just use ElementFactory and
create
> real instances?

Yes, I'd like to.
But ElementFactory is not a part of the analyzer package/lib folder, it is in
its test folder.
Should we move it to analyzer/lib and make it an API?

[scheglov, 2014-06-25 17:22:00]

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