-
Notifications
You must be signed in to change notification settings - Fork 6k
Background Execution Implementation for iOS #5539
Changes from 1 commit
4aa0ace
59f4c05
2f3e0d8
ddbd5f3
b15e35c
9806222
3341e83
8373f88
f4d23c2
87d3d80
14e7aca
af25c66
2ff7690
48261fd
abcabee
2ec1ef4
8cc4394
81492d6
514df89
c6a955d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,28 +4,61 @@ | |
|
|
||
| part of dart.ui; | ||
|
|
||
| /// Functionality for Flutter plugin authors. | ||
| abstract class PluginUtilities { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you talk about why you need these?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are needed to lookup callback methods which are top-level functions or static methods in a class by name. The general flow is something like this:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concerned about this API. From most serious to least serious concerns:
Can you talk more broadly about what problem these methods solve? Maybe we can find a safer solution to this problem.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I feel like a lot of your concerns could be addressed by an opaque class with static methods that could:
This way, we can avoid having these strings visible and modifiable by the user, prevent abuse of what are basically Dart VM APIs, and make caching difficult. The only difficulty I see with this approach is passing said object over method channels since it would have to be serialized and deserialized in such a way that the data itself still remains private, but anyone can listen to a method channel if they know the name of the channel. I'd have to give this part some thought...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your latest update resolves my concerns. Thanks! |
||
| static Function getClosureByName({String name, Uri libraryUri, String className}) { | ||
| /// Get a closure instance for a specific static or top-level function given | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should warn that it will fail to find the closure and return null if the closure is not otherwise used in the program. That is, it may get tree-shaken away in an AOT build.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| /// a name. | ||
| /// | ||
| /// `name` is the name of the function we want a closure for. This is a | ||
| /// required parameter. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typical wording is 'This must not be null.'
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| /// | ||
| /// `libraryPat h` is the path which points to the library which contains the | ||
| /// function of interest. This path can be retrieved using | ||
| /// [PluginUtilities.getPathForFunctionLibrary]. If not specified, the root | ||
| /// library will be used for the function lookup. | ||
| /// | ||
| /// `className` is the name of the class in which function `name` is defined. | ||
| /// This parameter is required for retrieving closures for static methods. If | ||
| /// not provided, `name` is assumed to be referring to a top-level function. | ||
| /// | ||
| /// Returns a closure for `name` as a [Function] if the lookup was successful. | ||
| /// Otherwise, `null` is returned on error. | ||
| static Function getClosureByName({String name, String libraryPath, String className}) { | ||
| if (name == null) { | ||
| throw new ArgumentError.notNull('name'); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and elsewhere: for null checks, generally prefer: We typically reserve ArgumentError for malformed values -- e.g. we're passed a list of length 3 when we document it must be 4, etc.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| return _lookupClosure(name, libraryUri?.toString(), className); | ||
| return _lookupClosure(name, libraryPath, className); | ||
| } | ||
|
|
||
| /// Get the path for the library which contains a given method. | ||
| /// | ||
| /// `closure` is the closure of the function that we want to find the library | ||
| /// path for. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add: 'This must not be null.'
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| /// | ||
| /// Returns a [String] representing the path to the library which contains | ||
| /// `closure`. Returns `null` if an error occurs or `closure` is not found | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing period.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| static String getPathForFunctionLibrary(Function closure) { | ||
| if (closure == null) { | ||
| throw new ArgumentError.notNull('closure'); | ||
| } | ||
| return _getFunctionLibraryUrl(closure); | ||
| } | ||
|
|
||
| /// Get the name of a [Function] as a [String]. | ||
| /// | ||
| /// Returns a [String] representing the name of the function if the function | ||
| /// exists, otherwise `null` is returned. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document that closure must not be null.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| static String getNameOfFunction(Function closure) { | ||
| if (closure == null) { | ||
| throw new ArgumentError.notNull('closure'); | ||
| } | ||
| return _getFunctionName(closure); | ||
| } | ||
|
|
||
| /// Get the name of a class containing [Function] as a [String]. | ||
| /// | ||
| /// Returns a [String] representing the name of the function if the function | ||
| /// exists and is a member of a class, otherwise `null` is returned. | ||
| static String getNameOfFunctionClass(Function closure) { | ||
| if(closure == null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| throw new ArgumentError.notNull('closure'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| // Copyright 2018 The Chromium Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:io'; | ||
| import 'dart:ui'; | ||
|
|
||
| import 'package:test/test.dart'; | ||
|
|
||
| String top() => "top"; | ||
|
|
||
| class Foo { | ||
| const Foo(); | ||
| static int getInt() => 1; | ||
| double getDouble() => 1.0; | ||
| } | ||
|
|
||
| const Foo foo = const Foo(); | ||
|
|
||
| void main() { | ||
| test('PluginUtilities.getNameOf*', () { | ||
| expect(PluginUtilities.getNameOfFunction(top), "top"); | ||
| expect(PluginUtilities.getNameOfFunction(Foo.getInt), "getInt"); | ||
| expect(PluginUtilities.getNameOfFunction(foo.getDouble), 'getDouble'); | ||
| expect(() => PluginUtilities.getNameOfFunction(null), | ||
| throwsA(const isInstanceOf<ArgumentError>())); | ||
|
|
||
| expect(PluginUtilities.getNameOfFunctionClass(top), null); | ||
| expect(PluginUtilities.getNameOfFunctionClass(Foo.getInt), 'Foo'); | ||
| expect(() => PluginUtilities.getNameOfFunctionClass(null), | ||
| throwsA(const isInstanceOf<ArgumentError>())); | ||
|
|
||
| // Can't lookup class name for instance functions. | ||
| expect(PluginUtilities.getNameOfFunctionClass(foo.getDouble), isNull); | ||
| }); | ||
|
|
||
| test('PluginUtilities.getPathForFunctionLibrary', () { | ||
| const String file = 'plugin_utilities_test.dart'; | ||
| expect( | ||
| PluginUtilities.getPathForFunctionLibrary(top).endsWith(file), isTrue); | ||
| expect(PluginUtilities.getPathForFunctionLibrary(Foo.getInt).endsWith(file), | ||
| isTrue); | ||
| expect( | ||
| PluginUtilities.getPathForFunctionLibrary(foo.getDouble).endsWith(file), | ||
| isTrue); | ||
|
|
||
| expect(() => PluginUtilities.getPathForFunctionLibrary(null), | ||
| throwsA(const isInstanceOf<ArgumentError>())); | ||
| }); | ||
|
|
||
| test('PluginUtilities.getClosureByName', () { | ||
| final String getIntName = PluginUtilities.getNameOfFunction(Foo.getInt); | ||
| final String getDoubleName = | ||
| PluginUtilities.getNameOfFunction(foo.getDouble); | ||
| final String topName = PluginUtilities.getNameOfFunction(top); | ||
| final String fooName = PluginUtilities.getNameOfFunctionClass(Foo.getInt); | ||
| final String libName = PluginUtilities.getPathForFunctionLibrary(top); | ||
|
|
||
| // We should successfully get a closure for top level and static methods. | ||
| expect( | ||
| PluginUtilities.getClosureByName( | ||
| name: getIntName, libraryPath: libName, className: fooName)(), | ||
| 1); | ||
| expect( | ||
| PluginUtilities.getClosureByName(name: topName, libraryPath: libName)(), | ||
| 'top'); | ||
|
|
||
| // We don't support getting closures for instance methods. | ||
| expect( | ||
| PluginUtilities.getClosureByName( | ||
| name: getDoubleName, libraryPath: libName, className: fooName), | ||
| null); | ||
|
|
||
| // Try an invalid function name. | ||
| expect(PluginUtilities.getClosureByName(name: 'baz'), null); | ||
| expect(() => PluginUtilities.getClosureByName(), | ||
| throwsA(const isInstanceOf<ArgumentError>())); | ||
|
|
||
| // Lookup from root library. | ||
| expect( | ||
| PluginUtilities.getClosureByName( | ||
| name: getIntName, className: fooName)(), | ||
| 1); | ||
| expect(PluginUtilities.getClosureByName(name: topName)(), 'top'); | ||
| expect( | ||
| PluginUtilities.getClosureByName( | ||
| name: getDoubleName, className: fooName), | ||
| null); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to write tests of these methods? If so, I think we should have some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be. I'll go ahead and write some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests added in latest commit.