Bug 80922 – Separate NOTIFICATIONS and LEGACY_NOTIFICATIONS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jon Lee]

The future of notifications includes establishing a new JS API that is still under discussion within the web notifications wg. Ports who wish to support the legacy API must enable both NOTIFICATIONS and LEGACY_NOTIFICATIONS, but also end up including the new API. We should remove the dependency, so that ports can migrate when they choose.

The idea is to change ENABLE(NOTIFICATIONS) to ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS). Ports that wish to only support the legacy API should remove ENABLE(NOTIFICATIONS) from their feature defines.

[Radar WebKit Bug Importer]

<rdar://problem/11035082>

[Jon Lee]

Created attachment 131724 [details]
Patch

[Jon Lee]

Also, with this patch, the generated files look like such:

In EventTargetFactory.in, this line:
Notification conditional=NOTIFICATIONS|LEGACY_NOTIFICATIONS

generates in EventTargetHeaders.h:

#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
#include "Notification.h"
#if USE(JSC)
#include "JSNotification.h"
#elif USE(V8)
#include "V8Notification.h"
#endif

and in EventTargetInterfaces.h:

#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
#define DOM_EVENT_TARGET_INTERFACES_FOR_EACH_NOTIFICATIONS(macro) \
    macro(Notification) \
// End of DOM_EVENT_TARGET_INTERFACES_FOR_EACH_NOTIFICATIONS
#else
#define DOM_EVENT_TARGET_INTERFACES_FOR_EACH_NOTIFICATIONS(macro)
#endif
...
#define DOM_EVENT_TARGET_INTERFACES_FOR_EACH(macro) \
...
    DOM_EVENT_TARGET_INTERFACES_FOR_EACH_NOTIFICATIONS(macro) \
...

[Jon Lee]

Created attachment 131744 [details]
Patch

[Jon Lee]

Created attachment 131745 [details]
Patch rebase

[Build Bot]

Comment on attachment 131745 [details]
Patch rebase

Attachment 131745 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11944876

[Jon Lee]

Created attachment 131795 [details]
Same patch again, to go through mac ews

[Jian Li]

Comment on attachment 131795 [details]
Same patch again, to go through mac ews

View in context: https://bugs.webkit.org/attachment.cgi?id=131795&action=review

> Source/WebCore/bindings/scripts/InFilesCompiler.pm:174
> +    my @conditionals = split('\\|', $conditional);

It seems that we only handle '|', not '&' here. But since handling of '&' is not required for this patch, you should consider it in another patch or ask Adam to enhance it.

> Source/WebCore/bindings/scripts/InFilesCompiler.pm:224
> +

Remove empty line.

[Jon Lee]

(In reply to comment #8)
> (From update of attachment 131795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131795&action=review
> 
> > Source/WebCore/bindings/scripts/InFilesCompiler.pm:174
> > +    my @conditionals = split('\\|', $conditional);
> 
> It seems that we only handle '|', not '&' here. But since handling of '&' is not required for this patch, you should consider it in another patch or ask Adam to enhance it.
Bug 81169

> 
> > Source/WebCore/bindings/scripts/InFilesCompiler.pm:224
> > +
> 
> Remove empty line.
Done.

Thanks!

[Jon Lee]

Committed r110784: <http://trac.webkit.org/changeset/110784>