Bug 80922 - Separate NOTIFICATIONS and LEGACY_NOTIFICATIONS
: Separate NOTIFICATIONS and LEGACY_NOTIFICATIONS
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Jon Lee
: InRadar
Depends on:
Blocks: 80472
  Show dependency treegraph
 
Reported: 2012-03-12 18:28 PDT by Jon Lee
Modified: 2012-03-14 16:41 PDT (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-03-12 18:28:50 PDT
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 Radar WebKit Bug Importer 2012-03-12 18:29:09 PDT
<rdar://problem/11035082>
Comment 2 Jon Lee 2012-03-13 15:20:20 PDT
Created attachment 131724 [details]
Patch
Comment 3 Jon Lee 2012-03-13 15:22:30 PDT
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 Jon Lee 2012-03-13 16:17:06 PDT
Created attachment 131744 [details]
Patch
Comment 5 Jon Lee 2012-03-13 16:24:39 PDT
Created attachment 131745 [details]
Patch rebase
Comment 6 Build Bot 2012-03-13 16:40:14 PDT
Comment on attachment 131745 [details]
Patch rebase

Attachment 131745 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11944876
Comment 7 Jon Lee 2012-03-14 00:03:01 PDT
Created attachment 131795 [details]
Same patch again, to go through mac ews
Comment 8 Jian Li 2012-03-14 16:00:33 PDT
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.
Comment 9 Jon Lee 2012-03-14 16:38:54 PDT
(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!
Comment 10 Jon Lee 2012-03-14 16:41:37 PDT
Committed r110784: <http://trac.webkit.org/changeset/110784>