Fix for subscribing for notifications for files in packages.

Fix for subscribing for notifications for files in packages.

Unfortunately this fix makes the server about 20% slower.

This is mostly because of the check in scheduleNotificationOperations(),
as required to fix dartbug.com/22210. I think it is much cheaper to send
duplicate notifications, like outline or navigation, than to check if
they should not be sent.


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

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

[Brian Wilkerson, 2015-03-06 15:04:27]

LGTM

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

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:442: Folder containingFolder =
null;
This is very similar to getContainingContext; it would be nice to not duplicate
the logic.

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:1185: class ContextSourcePair {
Comments?

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

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:615: if (context.sourceFactory
== null) {
This should never happen. We should minimally write a message to the log so that
we know how often it's happening.

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mock_sdk.dart (right):

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mock_sdk.dart:228: Uri dartUri =
Uri.parse(library.shortName);
I would have expected, based on the old code, that the short name did not
include the "dart:" scheme. I seems like we need to add that in order for the
URI to be correct.

[Paul Berry, 2015-03-06 19:18:33]

lgtm

[scheglov, 2015-03-06 21:47:03]

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

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:442: Folder containingFolder =
null;
On 2015/03/06 15:04:27, Brian Wilkerson wrote:
> This is very similar to getContainingContext; it would be nice to not
duplicate
> the logic.

Done.

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:1185: class ContextSourcePair {
On 2015/03/06 15:04:27, Brian Wilkerson wrote:
> Comments?

Done.

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mock_sdk.dart (right):

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mock_sdk.dart:228: Uri dartUri =
Uri.parse(library.shortName);
On 2015/03/06 15:04:27, Brian Wilkerson wrote:
> I would have expected, based on the old code, that the short name did not
> include the "dart:" scheme. I seems like we need to add that in order for the
> URI to be correct.

The comment in SdkLibrary says:

  /**
   * Return the short name of the library. This is the URI of the library,
including `dart:`.
   *
   * @return the short name of the library
   */
  String get shortName;

  MockSdk libraries follow this requirement.

  As I can see, SdkLibrariesReader_LibraryBuilder also appends _LIBRARY_PREFIX
(i.e. dart:) to the names it reads from libraries.dart file.

[Paul Berry, 2015-03-06 22:04:45]

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mock_sdk.dart (right):

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mock_sdk.dart:228: Uri dartUri =
Uri.parse(library.shortName);
On 2015/03/06 21:47:03, scheglov wrote:
> On 2015/03/06 15:04:27, Brian Wilkerson wrote:
> > I would have expected, based on the old code, that the short name did not
> > include the "dart:" scheme. I seems like we need to add that in order for
the
> > URI to be correct.
> 
> The comment in SdkLibrary says:
> 
>   /**
>    * Return the short name of the library. This is the URI of the library,
> including `dart:`.
>    *
>    * @return the short name of the library
>    */
>   String get shortName;
> 
>   MockSdk libraries follow this requirement.
> 
>   As I can see, SdkLibrariesReader_LibraryBuilder also appends _LIBRARY_PREFIX
> (i.e. dart:) to the names it reads from libraries.dart file.

I think Konstantin's analysis is correct.  However, there's a misleading comment
on SdkLibraryImpl._shortName, which claims that it *doesn't* include "dart:"  As
far as I can tell, it always does include "dart:".

[scheglov, 2015-03-06 22:10:34]

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

https://codereview.chromium.org/987663002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_manager.dart:615: if (context.sourceFactory
== null) {
On 2015/03/06 15:04:27, Brian Wilkerson wrote:
> This should never happen. We should minimally write a message to the log so
that
> we know how often it's happening.

Done.

You're right, it shouldn't.
I found it running tests, not all of the AnalysisContexts had SourceFactory set.
But now we don't need this check.

[scheglov, 2015-03-06 22:55:40]

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