[bindings] Avoid using custom bindings for handling clipboardData on Event idl interface.

[bindings] Avoid using custom bindings for handling clipboardData on Event idl interface.

This CL introduces a separate idl file for ClipboardEvent derived from Event interface and avoids using the
custom binding required for handling of clipboardData attribute.

R=haraken@chromium.org

BUG=345519
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187069

[vivekg, 2014-12-11 16:00:34]

vivekg@chromium.org changed reviewers:
+ jl@opera.com, vivekg@chromium.org

[vivekg, 2014-12-11 16:00:35]

Hi @haraken and @jl, please take a look.

[vivekg, 2014-12-11 16:02:34]

Although there one failure. Now instead of returning 'undefined', its returning
'null' for 'clipboardData' property.

Any idea how do we go fixing the same? 
(Should we have something like: TreatReturnedNullAs=Undefined on the similar
lines of TreatReturnedNullStringAs=Undefined)

The generated code looks like this

static void clipboardDataAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
{
    v8::Handle<v8::Object> holder = info.Holder();
    Event* impl = V8Event::toImpl(holder);
    RefPtrWillBeRawPtr<DataTransfer> cppValue(impl->clipboardData());
    if (cppValue && DOMDataStore::setReturnValue(info.GetReturnValue(),
cppValue.get()))
        return;
    v8::Handle<v8::Value> wrapper = toV8(cppValue.get(), holder,
info.GetIsolate());
    if (!wrapper.IsEmpty()) {
        V8HiddenValue::setHiddenValue(info.GetIsolate(), holder,
v8AtomicString(info.GetIsolate(), "clipboardData"), wrapper);
        v8SetReturnValue(info, wrapper);
    }
}

[Jens Widell, 2014-12-11 16:10:11]

On 2014/12/11 16:02:34, vivekg_ wrote:
> Although there one failure. Now instead of returning 'undefined', its
returning
> 'null' for 'clipboardData' property.
> 
> Any idea how do we go fixing the same? 
> (Should we have something like: TreatReturnedNullAs=Undefined on the similar
> lines of TreatReturnedNullStringAs=Undefined)

Shouldn't clipboard events rather use a separate interface?

interface ClipboardEvent : Event {
  [MeasureAs=EventClipboardData] readonly attribute DataTransfer clipboardData;
};

There's a FIXME in ClipboardEvent::interfaceName() about adding a
ClipboardEvent.idl.

[vivekg, 2014-12-11 16:12:22]

On 2014/12/11 at 16:10:11, jl wrote:
> On 2014/12/11 16:02:34, vivekg_ wrote:
> > Although there one failure. Now instead of returning 'undefined', its
returning
> > 'null' for 'clipboardData' property.
> > 
> > Any idea how do we go fixing the same? 
> > (Should we have something like: TreatReturnedNullAs=Undefined on the similar
> > lines of TreatReturnedNullStringAs=Undefined)
> 
> Shouldn't clipboard events rather use a separate interface?
> 
> interface ClipboardEvent : Event {
>   [MeasureAs=EventClipboardData] readonly attribute DataTransfer
clipboardData;
> };
> 
> There's a FIXME in ClipboardEvent::interfaceName() about adding a
ClipboardEvent.idl.

Oh yeah, totally missed it. Will apply the right approach of adding a new idl
file. Thank you.

[vivekg, 2014-12-12 07:21:21]

Added new IDL file ClipboardEvent. PTAL, thanks.

[Jens Widell, 2014-12-12 07:42:34]

On 2014/12/12 07:21:21, vivekg_ wrote:
> Added new IDL file ClipboardEvent. PTAL, thanks.

LGTM

I guess an API OWNER should sign off on this as well, since we're exposing a new
global constructor.

[sof, 2014-12-12 07:52:20]

On 2014/12/12 07:42:34, Jens Widell wrote:
> On 2014/12/12 07:21:21, vivekg_ wrote:
> > Added new IDL file ClipboardEvent. PTAL, thanks.
> 
> LGTM
> 
> I guess an API OWNER should sign off on this as well, since we're exposing a
new
> global constructor.

("clipboardData" in anyEvent) will no longer return true, which is more
accurate, but an observable change should anyone rely on client detection using
it.

[vivekg, 2014-12-12 07:59:22]

On 2014/12/12 at 07:52:20, sigbjornf wrote:
> On 2014/12/12 07:42:34, Jens Widell wrote:
> > On 2014/12/12 07:21:21, vivekg_ wrote:
> > > Added new IDL file ClipboardEvent. PTAL, thanks.
> > 
> > LGTM
> > 
> > I guess an API OWNER should sign off on this as well, since we're exposing a
new
> > global constructor.
> 
> ("clipboardData" in anyEvent) will no longer return true, which is more
accurate, but an observable change should anyone rely on client detection using
it.

Yes. In such cases we may need the usage data to see the overall usage of the
'clipboardData' property.

[sof, 2014-12-12 08:01:24]

On 2014/12/12 07:59:22, vivekg_ wrote:
> On 2014/12/12 at 07:52:20, sigbjornf wrote:
> > On 2014/12/12 07:42:34, Jens Widell wrote:
> > > On 2014/12/12 07:21:21, vivekg_ wrote:
> > > > Added new IDL file ClipboardEvent. PTAL, thanks.
> > > 
> > > LGTM
> > > 
> > > I guess an API OWNER should sign off on this as well, since we're exposing
a
> new
> > > global constructor.
> > 
> > ("clipboardData" in anyEvent) will no longer return true, which is more
> accurate, but an observable change should anyone rely on client detection
using
> it.
> 
> Yes. In such cases we may need the usage data to see the overall usage of the
> 'clipboardData' property.

https://www.chromestatus.com/metrics/feature/timeline/popularity/346 has that,
but we don't know at what Event type of course.

[Jens Widell, 2014-12-12 08:02:20]

On 2014/12/12 07:59:22, vivekg_ wrote:
> On 2014/12/12 at 07:52:20, sigbjornf wrote:
> > On 2014/12/12 07:42:34, Jens Widell wrote:
> > > On 2014/12/12 07:21:21, vivekg_ wrote:
> > > > Added new IDL file ClipboardEvent. PTAL, thanks.
> > > 
> > > LGTM
> > > 
> > > I guess an API OWNER should sign off on this as well, since we're exposing
a
> new
> > > global constructor.
> > 
> > ("clipboardData" in anyEvent) will no longer return true, which is more
> accurate, but an observable change should anyone rely on client detection
using
> it.
> 
> Yes. In such cases we may need the usage data to see the overall usage of the
> 'clipboardData' property.

It's used rather a lot (2 % of page loads) but I'm guessing a lot of that is
using "event.clipboardData" as a condition (casual browsing on GitHub suggests
that) and that won't change for either non-clipboard events or clipboard events,
since the observed attribute value is the same in all cases.

And '"clipboardData" in event' won't trigger the use counter anyway, since it
doesn't actually call the getter...

[vivekg, 2014-12-12 08:06:01]

On 2014/12/12 at 08:02:20, jl wrote:

> It's used rather a lot (2 % of page loads) but I'm guessing a lot of that is
using "event.clipboardData" as a condition (casual browsing on GitHub suggests
that) and that won't change for either non-clipboard events or clipboard events,
since the observed attribute value is the same in all cases.
> 
> And '"clipboardData" in event' won't trigger the use counter anyway, since it
doesn't actually call the getter...

Yes indeed, its hard to say by just looking at the UsageCounter values.

[haraken, 2014-12-12 08:34:04]

What are behaviors of other browsers?

[haraken, 2014-12-12 08:34:32]

On 2014/12/12 08:34:04, haraken wrote:
> What are behaviors of other browsers?

I mean, are Firefox and IE exposing ClipboardEvent to window?

[vivekg, 2014-12-12 08:37:06]

On 2014/12/12 at 08:34:32, haraken wrote:
> On 2014/12/12 08:34:04, haraken wrote:
> > What are behaviors of other browsers?
> 
> I mean, are Firefox and IE exposing ClipboardEvent to window?

According to [1], looks like at least IE is exposing this event to window.

[1] http://msdn.microsoft.com/en-us/library/ie/ms535220%28v=vs.85%29.aspx

[Jens Widell, 2014-12-12 08:45:46]

On 2014/12/12 08:34:32, haraken wrote:
> On 2014/12/12 08:34:04, haraken wrote:
> > What are behaviors of other browsers?
> 
> I mean, are Firefox and IE exposing ClipboardEvent to window?

Firefox (34) exposes it.

[haraken, 2014-12-12 08:50:13]

On 2014/12/12 08:45:46, Jens Widell wrote:
> On 2014/12/12 08:34:32, haraken wrote:
> > On 2014/12/12 08:34:04, haraken wrote:
> > > What are behaviors of other browsers?
> > 
> > I mean, are Firefox and IE exposing ClipboardEvent to window?
> 
> Firefox (34) exposes it.

Thanks for the info! LGTM.

Let's wait for an API owner approval.

[vivekg, 2014-12-12 08:54:54]

On 2014/12/12 at 08:50:13, haraken wrote:
> Thanks for the info! LGTM.
> 
> Let's wait for an API owner approval.

Sure, thank you.

[vivekg, 2014-12-12 13:09:06]

vivekg@chromium.org changed reviewers:
+ jochen@chromium.org, philipj@opera.com

[vivekg, 2014-12-12 13:09:07]

+@jochen and +@philipj for API OWNERS review.

[philipj_slow, 2014-12-12 14:25:35]

Thank you, this is great cleanup!

I feel like this is borderline trivial
<http://www.chromium.org/blink#trivial-changes>, it's only the 2% usage that
makes me hesitate. There may be code that does silly stuff like "if
(event.clipboardData !== null) { assumeItIsAnObject(); }" that would break when
event.clipboardData becomes undefined.

Would you please send an "Intent to Implement and Ship: ClipboardEvent
interface" to blink-dev noting that this is already implemented in Firefox but
that there's a small chance of trouble? I'll say L-G-T-M to that but it's good
to have some visibility.

(A more conservative approach would be to keep an always-null clipboardData
attrive on Event and continue to measure usage, similar to Node.namespaceURI and
Node.localName. I think that's overly cautious in this case, though.)

https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Clipb...
File Source/core/events/ClipboardEvent.idl (right):

https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Clipb...
Source/core/events/ClipboardEvent.idl:6: [MeasureAs=EventClipboardData] readonly
attribute DataTransfer clipboardData;
If you want to measure here, add a new UseCounter feature
ClipboardEventClipboardData. I don't think there's any reason to, though, the
existing one should simply be removed since it was added to evaluate the
feasibility of this very change.

https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Event...
File Source/core/events/Event.idl (left):

https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Event...
Source/core/events/Event.idl:74: [Custom=Getter, MeasureAs=EventClipboardData]
readonly attribute DataTransfer clipboardData;
This has rather high usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/346

It's possible that stuff could break when clipboardData goes from null to
undefined on events other than ClipboardEvent.

[Jens Widell, 2014-12-12 14:35:07]

On 2014/12/12 14:25:35, philipj wrote:
> Thank you, this is great cleanup!
> 
> I feel like this is borderline trivial
> <http://www.chromium.org/blink#trivial-changes>, it's only the 2% usage that
> makes me hesitate. There may be code that does silly stuff like "if
> (event.clipboardData !== null) { assumeItIsAnObject(); }" that would break
when
> event.clipboardData becomes undefined.

It was already undefined on non-clipboard events.

> Would you please send an "Intent to Implement and Ship: ClipboardEvent
> interface" to blink-dev noting that this is already implemented in Firefox but
> that there's a small chance of trouble? I'll say L-G-T-M to that but it's good
> to have some visibility.
> 
> (A more conservative approach would be to keep an always-null clipboardData
> attrive on Event and continue to measure usage, similar to Node.namespaceURI
and
> Node.localName. I think that's overly cautious in this case, though.)
> 
>
https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Clipb...
> File Source/core/events/ClipboardEvent.idl (right):
> 
>
https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Clipb...
> Source/core/events/ClipboardEvent.idl:6: [MeasureAs=EventClipboardData]
readonly
> attribute DataTransfer clipboardData;
> If you want to measure here, add a new UseCounter feature
> ClipboardEventClipboardData. I don't think there's any reason to, though, the
> existing one should simply be removed since it was added to evaluate the
> feasibility of this very change.
> 
>
https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Event...
> File Source/core/events/Event.idl (left):
> 
>
https://codereview.chromium.org/797583002/diff/40001/Source/core/events/Event...
> Source/core/events/Event.idl:74: [Custom=Getter, MeasureAs=EventClipboardData]
> readonly attribute DataTransfer clipboardData;
> This has rather high usage:
> https://www.chromestatus.com/metrics/feature/timeline/popularity/346
> 
> It's possible that stuff could break when clipboardData goes from null to
> undefined on events other than ClipboardEvent.

[philipj_slow, 2014-12-12 14:54:03]

On 2014/12/12 14:35:07, Jens Widell wrote:
> On 2014/12/12 14:25:35, philipj wrote:
> > Thank you, this is great cleanup!
> > 
> > I feel like this is borderline trivial
> > <http://www.chromium.org/blink#trivial-changes>, it's only the 2% usage that
> > makes me hesitate. There may be code that does silly stuff like "if
> > (event.clipboardData !== null) { assumeItIsAnObject(); }" that would break
> when
> > event.clipboardData becomes undefined.
> 
> It was already undefined on non-clipboard events.

Ah, I didn't actually read the custom bindings, in that case I see very little
risk.

Please remove EventClipboardData from UseCounter.h and elsewhere since it has
served its purpose. No need to wait for a second review of that change, LGTM.

[vivekg, 2014-12-12 15:29:13]

On 2014/12/12 at 14:54:03, philipj wrote:
 
> Ah, I didn't actually read the custom bindings, in that case I see very little
risk.
> 
> Please remove EventClipboardData from UseCounter.h and elsewhere since it has
served its purpose. No need to wait for a second review of that change, LGTM.

Thank you! I have removed the enum value as mentioned. 

Can anyone of you please rubber stamp this change before I hit the CQ? :)

[vivekg, 2014-12-12 16:40:02]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-12-12 16:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797583002/60001

[vivekg, 2014-12-12 16:41:01]

On 2014/12/12 15:29:13, vivekg_ wrote:
> On 2014/12/12 at 14:54:03, philipj wrote:
>  
> > Ah, I didn't actually read the custom bindings, in that case I see very
little
> risk.
> > 
> > Please remove EventClipboardData from UseCounter.h and elsewhere since it
has
> served its purpose. No need to wait for a second review of that change, LGTM.
> 
> Thank you! I have removed the enum value as mentioned. 
> 
> Can anyone of you please rubber stamp this change before I hit the CQ? :)

Oh missed your previous lgtm. Thanks triggering cq.

[commit-bot: I haz the power, 2014-12-12 16:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 16:45:42]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[vivekg, 2014-12-12 23:01:37]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-12-12 23:02:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797583002/60001

[commit-bot: I haz the power, 2014-12-13 00:19:03]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187069