Bug 80922 - Separate NOTIFICATIONS and LEGACY_NOTIFICATIONS
: Separate NOTIFICATIONS and LEGACY_NOTIFICATIONS
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
: 80472
  Show dependency treegraph
 
Reported: 2012-03-12 18:28 PST by
Modified: 2012-03-14 16:41 PST (History)


Attachments
Patch (53.73 KB, patch)
2012-03-13 15:20 PST, Jon Lee
no flags Review Patch | Details | Formatted Diff | Diff
Patch (92.77 KB, patch)
2012-03-13 16:17 PST, Jon Lee
no flags Review Patch | Details | Formatted Diff | Diff
Patch rebase (92.80 KB, patch)
2012-03-13 16:24 PST, Jon Lee
no flags Review Patch | Details | Formatted Diff | Diff
Same patch again, to go through mac ews (92.72 KB, patch)
2012-03-14 00:03 PST, Jon Lee
jianli: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-12 18:28:50 PST
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.
------- Comment #1 From 2012-03-12 18:29:09 PST -------
<rdar://problem/11035082>
------- Comment #2 From 2012-03-13 15:20:20 PST -------
Created an attachment (id=131724) [details]
Patch
------- Comment #3 From 2012-03-13 15:22:30 PST -------
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) \
...
------- Comment #4 From 2012-03-13 16:17:06 PST -------
Created an attachment (id=131744) [details]
Patch
------- Comment #5 From 2012-03-13 16:24:39 PST -------
Created an attachment (id=131745) [details]
Patch rebase
------- Comment #6 From 2012-03-13 16:40:14 PST -------
(From update of attachment 131745 [details])
Attachment 131745 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11944876
------- Comment #7 From 2012-03-14 00:03:01 PST -------
Created an attachment (id=131795) [details]
Same patch again, to go through mac ews
------- Comment #8 From 2012-03-14 16:00:33 PST -------
(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.

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

Remove empty line.
------- Comment #9 From 2012-03-14 16:38:54 PST -------
(In reply to comment #8)
> (From update of attachment 131795 [details] [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!
------- Comment #10 From 2012-03-14 16:41:37 PST -------
Committed r110784: <http://trac.webkit.org/changeset/110784>