900243 - Convert BrowserFeedWriter to WebIDL

Status: RESOLVED FIXED

(SeaMonkey :: Feed Discovery and Preview, defect)

Comment 1

[Masatoshi Kimura [:emk]]

Neil, is nsIDOMGlobalPropertyInitializer unusable? init() will be called eagerly for JS-implemented WebIDL.
https://mxr.mozilla.org/comm-central/source/mozilla/dom/bindings/BindingUtils.cpp?rev=1b7eef53c08a#2049

Comment 2

[[e-mail]]

The only difference between the two interfaces is that XPCOM makes the distinction between the global property and the global constructor, but when global constructors were implemented in WebIDL they were implemented using nsIDOMGlobalPropertyInitializer by mistake, and we're stuck with it.

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to [e-mail] from comment #2)
> XPCOM makes the distinction between the global property and the global constructor

I don't understand the difference, honestly. Why the distinction is important?

Comment 4

[[e-mail]]

It looks like nsIDOMGlobalObjectConstructor is actually a placeholder interface; what happens is that bug 723206 made it so that calling new BrowserFeedWriter(args) passes those args to the construct method, while bug 779626 added the window as the first argument. (Although ideally it should have updated the IDL to make the change clearer.) Obviously it doesn't make sense to pass constructor arguments to a global property. (I don't know whether WebIDL even supports this.)

Of course, the distinction is only important to SeaMonkey in that it has to implement the interface that DOMCI is looking for.

Comment 5

[Masatoshi Kimura [:emk]]

When a JS-implemented WebIDL interface is constructed, two methods will be called:
1. init(window) will be called if the interface implements nsIDOMGlobalPropertyInitializer. The first argument is the windows object. (Why the "property" initializer is called for the constructor? I don't know the reason, but it is definitely called.) Implemented by bug 865544.
2. __init(...args) will be called (beware the leading underscores). |args| will be the constructor arguments. Implemented by bug 723206.
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_a_WebIDL_object_in_JavaScript

Why 1. does not satisfy your demand? Are you saying bindings should have used nsIDOMGlobalObjectConstructor::constructor instead of nsIDOMGlobalPropertyInitializer::init for 1.? Although it might have been true when bug 865544 was ongoing (bug 865544 comment 9 even suggested that), I don't understand the benefit of breaking the compatibility with existing JS-implemented WebIDL interfaces at this point. Two methods have the exact same signature (modulo the method name, of course).

Comment 6

[[e-mail]]

(In reply to Masatoshi Kimura from comment #5)
> Why 1. does not satisfy your demand? Are you saying bindings should have
> used nsIDOMGlobalObjectConstructor::constructor instead of
> nsIDOMGlobalPropertyInitializer::init for 1.? Although it might have been
> true when bug 865544 was ongoing (bug 865544 comment 9 even suggested that),
> I don't understand the benefit of breaking the compatibility with existing
> JS-implemented WebIDL interfaces at this point. Two methods have the exact
> same signature (modulo the method name, of course).

Sorry, I think we're at cross purposes here. The only reason I can't use nsIDOMGlobalPropertyInitializer is that never gets called for my XPCOM component.

Comment 7

[Masatoshi Kimura [:emk]]

By the way, I found rss feed view was already broken on SeaMonkey Nightly. Filed bug 998724.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Enable WebIDL BrowserFeedWriter for SeaMonkey (WIP)

This patch will enable BrowserFeedWriter unconditionally for the proof of the concept.

  if CONFIG['MOZ_BUILD_APP'] in ['browser', 'suite', 'xulrunner']:

didn't work.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Port bug 983845: Convert BrowserFeedWriter to WebIDL

(In reply to [e-mail] from comment #6)
> The only reason I can't use
> nsIDOMGlobalPropertyInitializer is that never gets called for my XPCOM
> component.

Actually it IS called. I'm running out of a way how to explain it works with my poor English, so I wrote a proof-of-concept patch.
I tested this patch (with a patch from bug 998724) locally and worked perfectly for me.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 8409413 [details] [diff] [review]
Part 1: Enable WebIDL BrowserFeedWriter for SeaMonkey (WIP)

Please teach me how to add only SeaMonkey here.

Comment 11

[[e-mail]]

(In reply to Masatoshi Kimura from comment #8)
>   if CONFIG['MOZ_BUILD_APP'] in ['browser', 'suite', 'xulrunner']:
> 
> didn't work.

Well, that depends on the state of the c-c/m-c merge proposal.

With the original proposal, that code will work.

With the slightly-less-than-optimal proposal, the build app will be 'comm/suite'.

Without the proposal, or with the terrible counter-proposal, I think you need '../suite'.

Comment 12

[[e-mail]]

Comment on attachment 8409415 [details] [diff] [review]
Part 2: Port bug 983845: Convert BrowserFeedWriter to WebIDL

>+  init: function constructor(aWindow) {
[function init(aWindow) of course]

Comment 13

[Masatoshi Kimura [:emk]]

Looks like CONFIG['MOZ_SUITE'] is the right way to identify SeaMonkey in the build system.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Enable WebIDL BrowserFeedWriter for SeaMonkey

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Port bug 983845: Convert BrowserFeedWriter to WebIDL

Comment 16

[[e-mail]]

Comment on attachment 8413157 [details] [diff] [review]
Part 2: Port bug 983845: Convert BrowserFeedWriter to WebIDL

What should we do about the unnecessary (in SeaMonkey) writeContent and close methods in BrowserFeedWriter.idl ?

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to [e-mail] from comment #16)
> Comment on attachment 8413157 [details] [diff] [review]
> Part 2: Port bug 983845: Convert BrowserFeedWriter to WebIDL
> 
> What should we do about the unnecessary (in SeaMonkey) writeContent and
> close methods in BrowserFeedWriter.idl ?

You need to do nothing. Unimplemented WebIDL methods will just throw when called and nobody will ever call them on SeaMonkey.

Comment 18

[Tom S [:evilpie]]

What happened here?

Comment 19

[Masatoshi Kimura [:emk]]

These patches still applies cleanly.
Unfortunately I can't build SeaMonkey locally again. Please land the c-c change.

Comment 20

[Philip Chee]

Pushed Part 2 to comm-central: http://hg.mozilla.org/comm-central/rev/8b019ad4e361
comm-central changeset 8b019ad4e361

Comment 21

[Philip Chee]

Pushed Part 1 to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad08763cd543

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad08763cd543