Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Establish standard mechanism for implementing static methods in DOM IDL #3514

Closed
vsmenon opened this issue Jun 11, 2012 · 17 comments
Closed
Labels
closed-duplicate Closed in favor of an existing report web-libraries Issues impacting dart:html, etc., libraries

Comments

@vsmenon
Copy link
Member

vsmenon commented Jun 11, 2012

We have static methods popping up in upstream IDL. We need a standard way to map these. Currently, we do not expose these methods on any public interface or class by default. With operator call, we can do the following:

class _DOMURL_createObjectURL {
  const _DOMURL_createObjectURL();
  String operator call() => ...;
}

interface DOMURL {
  static final Function createObjectURL = const _DOMURL_createObjectURL();
  ...
}

main() {
  ...
  String url = DOMURL.createObjectURL(blob);
  ...
}

@larsbak
Copy link

larsbak commented Jun 12, 2012

Why does DOMURL need to be an interface?
Could it be a class instead?
//Lars

@rakudrama
Copy link
Member

It can't be a class.

  1. We use the same interface with different implementation classes for JS, Dartium.
  2. The DOM libraries need to be able to distinguish real DOM objects from other classes that implement the interface - only the real DOM instances actually work when passed to DOM operations.

We also need a typedef for the Function type for each of the fake static function objects. Otherwise the editor will not be able to make good suggestions, e.g. the correct number and types of arguments.

@kasperl
Copy link

kasperl commented Jun 12, 2012

I didn't really think it through when I suggested this to Lars today, but my rough idea was to have an abstract class for DOMURL and use its implied interface like this:

  abstract class DOMURL {
    static String createObjectURL(blob) { ... }
    abstract foo();
  }

  class _DOMURL implements DOMURL {
    foo() { ... }
  }

That should solve 1 (as long as you don't need to use the default implementation of the interface) and maybe 2 still works? As I said, I didn't really think this through.

@vsmenon
Copy link
Member Author

vsmenon commented Jun 12, 2012

It's not clear to me how abstract classes would work in general.

The public interfaces make a deep tree. The private implementation classes (for either Dartium or Dart2JS) make a mirror tree. Then, pairwise, the implementation class implements its corresponding interface.

E.g., interfaces: CanvasElement extends Element extends Node extends EventTarget.

and classes: _CanvasElementImpl extends _ElementImpl extends ...

If we need to add a static method to CanvasElement, where do we put it?

@kasperl
Copy link

kasperl commented Jun 13, 2012

So you would have a tree of abstract "interface" classes like this:

    abstract class EventTarget ...
    abstract class Node extends EventTarget ...
    abstract class Element extends Node ...
    abstract class CanvasElement extends ...

and then you implement the implied interfaces for them in the parallel hierarchy:

   class _EventTargetImpl implements EventTarget ...
   class _NodeImpl extends _EventTargetImpl implements Node ...
   class _ElementImpl extends _NodeImpl implements Element ...
   class _CanvasElementImpl extends _ElementImpl implements CanvasElement ...

The static methods go into the abstract "interface" classes and you may need to add factory methods to them as well so if you try to construct a CanvasElement you really get a _CanvasElementImpl.

Still not quite sure if this works in general.

@gbracha
Copy link
Contributor

gbracha commented Jun 13, 2012

As far as I can tell, this scheme should work just fine (unless there are further constraints that have not been mentioned). In particular, the objections raised in comment 2 do not seem to apply:

Every class has an implied interface, so an abstract class can be used as an interface.
You can distinguish the actual instances by their implementation class, even if they inherit from the abstract class(es). The abstract classes serve as the public types, and also provide the static methods.

Instantiating an abstract class via a factory constructor is perfectly legal, so make sure that the constructors are declared as factories and create the desired implementation classes.

At the moment, I don't see why this won't work. It's growing on me, as it nicely leverages implicit interfaces and factory constructors.

@vsmenon
Copy link
Member Author

vsmenon commented Jun 13, 2012

Clever! Renaming the bug title to reflect other options. :-)

So, two possibilities:

(1). Use operator call as sketched out in my first comment.

This is blocked on implementation:
Analyzer issue #1355.
VM issue #1604.
Dart2JS issue #2932.

It may require a level of overhead at runtime as well (i.e., singleton "function" objects).

(2). Use Kasper's abstract class scheme.

It looks like everything we need is implemented. It's a somewhat more significant rework, but it should be straightforward:

  • public interfaces become public abstract classes
      - should the abstract Element class extend Node (the class) or implement Node (the implicit interface)? does it make a difference?
  • methods are explicitly declared abstract
  • "default" clauses are replaced by factory constructors

Any downsides to this approach?


Changed the title to: "Establish standard mechanism for implementing static methods in DOM IDL".

@DartBot
Copy link

DartBot commented Jun 14, 2012

This comment was originally written by antonm@google.com


The main problem is multiple inheritance which doesn't quite fit into Kasper's scheme. There is a workaround---just copy methods from other classes.

@vsmenon
Copy link
Member Author

vsmenon commented Jun 14, 2012

Note, you can modify Kasper's scheme to have the abstract classes just implement each other:

    abstract class EventTarget ...
    abstract class Node implements EventTarget ...
    abstract class Element implements Node ...
    abstract class CanvasElement implements ...

So, I think everything works out just as it does today. We'll of course still need the copy trick for the Impl classes.

@DartBot
Copy link

DartBot commented Jun 14, 2012

This comment was originally written by antonm@google.com


Yes, I know it, but the problem is with methods, the case:

IDL:

interface A { methodA(); }
interface B { methodB(); }
interface C extends A, B {}

Now we generate:

abstract class A implements B, C extends ?

Apparently whatever you put instead of ?, you'll have to copy another method for a second class.

@vsmenon
Copy link
Member Author

vsmenon commented Jun 14, 2012

Anton, why does A (actually, I think you meant C from IDL) need to extend anything?

IDL:

interface A { methodA(); }
interface B { methodB(); }
interface C extends A, B {}

Generate:

abstract class A {
  abstract methodA();
}

abstract class B {
  abstract methodB();
}

abstract class C implements A, B {
}

class _AImpl extends _DOMWrapperBase implements A {
  methodA() { ... }
}

class _BImpl extends _DOMWrapperBase implements B {
  methodB() { ... }
}

class _CImpl extends _AImpl implements C {
  // methodA() is inherited from _AImpl, methodB must be copied (as it is today).
  methodB() { ... }
}

It's not clear to me whether the abstract C needs to redeclare methodA or methodB. If I omit them, the editor doesn't give me a warning and my sample seems to run just fine.

@DartBot
Copy link

DartBot commented Jun 14, 2012

This comment was originally written by antonm@google.com


But now your abstract class doesn't implement promised interface which should trigger a warning, no?

@vsmenon
Copy link
Member Author

vsmenon commented Jun 14, 2012

But now your abstract class doesn't implement promised interface which should trigger a warning, no?

It currently doesn't trigger such a warning in the Dart editor & it (my simple test) works in Dartium and Dart2JS without warning or error. Surprisingly, the editor is even able to code complete methods on C that are not implemented in C (but are declared via implements and implemented in _CImpl).

Gilad / Kasper: should it be a warning if an abstract class does not implement the interfaces in its implements clause?


cc @gbracha.

@gbracha
Copy link
Contributor

gbracha commented Jun 15, 2012

No. If a concrete class does not implement methods, that will cause
warnings. No issue with abstract classes.

@DartBot
Copy link

DartBot commented Jun 18, 2012

This comment was originally written by antonm@google.com


Gilad, thanks for explanations, sorry I wrongly assumed that abstract classes must implement methods.

@iposva-google
Copy link
Contributor

Removed Area-DOM label.
Added Area-HTML label.

@vsmenon
Copy link
Member Author

vsmenon commented Aug 16, 2012

We're going with the abstract class route described here. Merging into 4525.


Added Duplicate label.
Marked as being merged into #4525.
Unmarked this as being blocked by #2931.

@vsmenon vsmenon added Type-Defect web-libraries Issues impacting dart:html, etc., libraries closed-duplicate Closed in favor of an existing report labels Aug 16, 2012
copybara-service bot pushed a commit that referenced this issue Aug 23, 2022
Changes:
```
> git log --format="%C(auto) %h %s" ac7db6c..ec35d46
 https://dart.googlesource.com/pub.git/+/ec35d462 Fix null-safety warning in dart.dart (#3514)
 https://dart.googlesource.com/pub.git/+/7d30cf0b Allow pubspec key 'funding' (#3529)
 https://dart.googlesource.com/pub.git/+/8ff63a8e Fix remaining usages of path.prettyUri on git uris (#3524)
 https://dart.googlesource.com/pub.git/+/bc32a30e Fix typo "exitcode" (#3511)

```

Diff: https://dart.googlesource.com/pub.git/+/ac7db6c07318efa4a8712110275eaf70f96a6d00~..ec35d46261b610e558dfd0d8525ca3fc8387b4b7/
Change-Id: I535f4fab8940ee8400ef75369d35fb4c1ee3adab
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/255990
Reviewed-by: Sarah Zakarias <zarah@google.com>
Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
copybara-service bot pushed a commit that referenced this issue Aug 23, 2022
This reverts commit 78636aa.

Reason for revert: This breaks the google3 roll. We need to roll frontend_server_client first.

Original change's description:
> Bump pub to ec35d46261b610e558dfd0d8525ca3fc8387b4b7
>
> Changes:
> ```
> > git log --format="%C(auto) %h %s" ac7db6c..ec35d46
>  https://dart.googlesource.com/pub.git/+/ec35d462 Fix null-safety warning in dart.dart (#3514)
>  https://dart.googlesource.com/pub.git/+/7d30cf0b Allow pubspec key 'funding' (#3529)
>  https://dart.googlesource.com/pub.git/+/8ff63a8e Fix remaining usages of path.prettyUri on git uris (#3524)
>  https://dart.googlesource.com/pub.git/+/bc32a30e Fix typo "exitcode" (#3511)
>
> ```
>
> Diff: https://dart.googlesource.com/pub.git/+/ac7db6c07318efa4a8712110275eaf70f96a6d00~..ec35d46261b610e558dfd0d8525ca3fc8387b4b7/
> Change-Id: I535f4fab8940ee8400ef75369d35fb4c1ee3adab
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/255990
> Reviewed-by: Sarah Zakarias <zarah@google.com>
> Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>

TBR=sigurdm@google.com,zarah@google.com

Change-Id: I9f46b5a63f8c3f0b11a9fe1d348696b1f988e5ca
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256080
Reviewed-by: Sigurd Meldgaard <sigurdm@google.com>
Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
Reviewed-by: Jonas Jensen <jonasfj@google.com>
copybara-service bot pushed a commit that referenced this issue Aug 25, 2022
This is a reland of commit 78636aa

Original change's description:
> Bump pub to ec35d46261b610e558dfd0d8525ca3fc8387b4b7
>
> Changes:
> ```
> > git log --format="%C(auto) %h %s" ac7db6c..ec35d46
>  https://dart.googlesource.com/pub.git/+/ec35d462 Fix null-safety warning in dart.dart (#3514)
>  https://dart.googlesource.com/pub.git/+/7d30cf0b Allow pubspec key 'funding' (#3529)
>  https://dart.googlesource.com/pub.git/+/8ff63a8e Fix remaining usages of path.prettyUri on git uris (#3524)
>  https://dart.googlesource.com/pub.git/+/bc32a30e Fix typo "exitcode" (#3511)
>
> ```
>
> Diff: https://dart.googlesource.com/pub.git/+/ac7db6c07318efa4a8712110275eaf70f96a6d00~..ec35d46261b610e558dfd0d8525ca3fc8387b4b7/
> Change-Id: I535f4fab8940ee8400ef75369d35fb4c1ee3adab
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/255990
> Reviewed-by: Sarah Zakarias <zarah@google.com>
> Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>

Change-Id: I9c50c7b5ea01fc24cc97b328766ad7d2b9c4b36e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256280
Reviewed-by: Sarah Zakarias <zarah@google.com>
Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-duplicate Closed in favor of an existing report web-libraries Issues impacting dart:html, etc., libraries
Projects
None yet
Development

No branches or pull requests

7 participants