Closed Bug 900243 Opened 8 years ago Closed 7 years ago

Convert BrowserFeedWriter to WebIDL

Categories

(SeaMonkey :: Feed Discovery and Preview, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.33

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
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
Flags: needinfo?(neil)
Summary: Remove nsIDOMGlobalObjectConstructor usage from SeaMonkey → Convert BrowserFeedWriter to WebIDL
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.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
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.
Flags: needinfo?(neil)
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).
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
By the way, I found rss feed view was already broken on SeaMonkey Nightly. Filed bug 998724.
This patch will enable BrowserFeedWriter unconditionally for the proof of the concept.

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

didn't work.
(In reply to neil@parkwaycc.co.uk 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.
Attachment #8409415 - Flags: feedback?(neil)
Comment on attachment 8409413 [details] [diff] [review]
Part 1: Enable WebIDL BrowserFeedWriter for SeaMonkey (WIP)

Please teach me how to add only SeaMonkey here.
Attachment #8409413 - Flags: feedback?(neil)
(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 on attachment 8409415 [details] [diff] [review]
Part 2: Port bug 983845: Convert BrowserFeedWriter to WebIDL

>+  init: function constructor(aWindow) {
[function init(aWindow) of course]
Attachment #8409415 - Flags: feedback?(neil) → feedback+
Looks like CONFIG['MOZ_SUITE'] is the right way to identify SeaMonkey in the build system.
Assignee: nobody → VYV03354
Attachment #8409413 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8409413 - Flags: feedback?(neil)
Attachment #8413156 - Flags: review?(neil)
Attachment #8409415 - Attachment is obsolete: true
Attachment #8413157 - Flags: review?(neil)
Attachment #8413156 - Flags: review?(neil) → review+
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 ?
(In reply to neil@parkwaycc.co.uk 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.
What happened here?
Attachment #8413157 - Flags: review?(neil) → review+
These patches still applies cleanly.
Unfortunately I can't build SeaMonkey locally again. Please land the c-c change.
Keywords: checkin-needed
Pushed Part 2 to comm-central: http://hg.mozilla.org/comm-central/rev/8b019ad4e361
comm-central changeset 8b019ad4e361
Target Milestone: --- → seamonkey2.33
https://hg.mozilla.org/mozilla-central/rev/ad08763cd543
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.