Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Jan 4, 2019

No description provided.

@Hixie Hixie requested review from amirh and bkonyi January 4, 2019 23:26
@Hixie Hixie force-pushed the docs branch 2 times, most recently from 7dc8e5e to 84c349f Compare January 4, 2019 23:42
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM for isolate_name_server (I don't know it at all, just assuming what you wrote is correct 😄 ).
LGTM for the rest.


/// A handle to an image codec.
///
/// This class is created by the engine, and should not be instantiated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe worth making this a macro

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave that for the next person who wants to do some cleanup :-)

Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with comments.

abstract class IsolateNameServer {
/// Looks up the [SendPort] associated with a given name. Returns null
/// if the name does not exist.
/// Static methods to allow for simple sharing of [SendPort]s across [Isolates].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be [Isolate]s for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. good catch. fixed.

@override
int get hashCode => _handle;
bool operator ==(dynamic other) {
if (runtimeType != other.runtimeType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for this change here? Does this not accomplish the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code used is, which isn't correct in operator == (because it makes different subclasses compare equal).
We have a boilerplate pattern we use for all operator == throughout Flutter.

@chinmaygarde
Copy link
Contributor

Can you land this?

@Hixie
Copy link
Contributor Author

Hixie commented Jan 14, 2019

Updated per comments. This can land on green (feel free to land it for me if you see it's green).

@Hixie Hixie changed the title Documentation cleanup [HR] Documentation cleanup Jan 14, 2019
@Hixie Hixie merged commit 3996efb into flutter:master Jan 14, 2019
@Hixie Hixie deleted the docs branch January 14, 2019 20:46
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 14, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants