Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213783002:
Pass current value of attributes to WebDOMActivityLogger Setter logs. (Closed)
|
Created:
6 years, 9 months ago by Devlin Modified:
6 years, 7 months ago CC:
blink-reviews, jamesr, arv+blink, jsbell+bindings_chromium.org, sof, kouhei+bindings_chromium.org, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, watchdog-blink-watchlist_google.com, Inactive, felt Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionPass current value of attributes to WebDOMActivityLogger Setter logs.
Pass the current value of attributes to WebDOMActivityLogger in Setter calls.
In order to keep this clean, separate out log() into logMethod(), logGetter(),
and logSetter().
This is necessary for improvements to the ActivityLogger, where we need to detect if a modification of certain attributes is a switch from a current setting, or an initialization. In the case of the former, we may need to treat this differently (as it could be modifying to perform ad injection, or other nefarious purposes).
BUG=356890
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173227
Patch Set 1 #
Total comments: 6
Patch Set 2 : Working Version #
Total comments: 24
Patch Set 3 : Adam's #Patch Set 4 : Nils's #
Total comments: 11
Patch Set 5 : Rebase to pull attribute changes into separate CL #
Total comments: 11
Patch Set 6 : Kentaro's #Patch Set 7 : Remove log method from WebDOMActivityLogger #Patch Set 8 : creation_context='info.Holder()->CreationContext()' #Patch Set 9 : #Patch Set 10 : Latest master #
Messages
Total messages: 51 (0 generated)
Petr, Adrienne mentioned you were the one to go to for WebKit side. Can you take a look? :) https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... Source/bindings/templates/attributes.cpp:303: v8::Handle<v8::Value> HACK; I'm a bit at a loss for how to best get the current value. It boils down to something like: {{cpp_class}}* impl = {{v8_class}::toNative(info.Holder()); v8::Handle<v8::Value> currentValue = impl->get{{attribute.name}} But I'm missing a few things in there (looking at the attribute_getter code at the top, I know there's a lot of subtle things here)...
https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... Source/bindings/templates/attributes.cpp:303: v8::Handle<v8::Value> HACK; On 2014/03/26 22:58:42, D Cronin wrote: > I'm a bit at a loss for how to best get the current value. ? Can't you just pass in jsValue (sans array), as that's what's currently being done?
https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... Source/bindings/templates/attributes.cpp:303: v8::Handle<v8::Value> HACK; OIC, sorry, the point of this CL is to add this value; I'd been distracted by the refactoring. Let me take a closer look.
https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... Source/bindings/templates/attributes.cpp:303: v8::Handle<v8::Value> HACK; On 2014/03/26 22:58:42, D Cronin wrote: > I'm a bit at a loss for how to best get the current value. > But I'm missing a few things in there (looking at the attribute_getter code at > the top, I know there's a lot of subtle things here)... Simplest would be if you could just call the *AttributeGetter function, no? Failing that, {{attribute.cpp_value}} will generally hold the value (as a C++ expression), but if {{attribute.cpp_value_original}} exists you'll need to use that instead (sometimes store value in a local variable). Could you see if this works?
Thanks in advance for the help! :) https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... Source/bindings/templates/attributes.cpp:303: v8::Handle<v8::Value> HACK; On 2014/03/27 02:51:25, Nils Barth wrote: > On 2014/03/26 22:58:42, D Cronin wrote: > > I'm a bit at a loss for how to best get the current value. > > But I'm missing a few things in there (looking at the attribute_getter code at > > the top, I know there's a lot of subtle things here)... > > Simplest would be if you could just call the *AttributeGetter function, no? > > Failing that, {{attribute.cpp_value}} will generally hold the value > (as a C++ expression), but if {{attribute.cpp_value_original}} exists > you'll need to use that instead (sometimes store value in a local > variable). > Could you see if this works? (I've never worked on the bindings before, so I might be missing something very obvious. If so, my apologies.) We might be able to just call *AttributeGetter, but we'd need to pass in some form of callback, right? Can we do that from here safely? For the other approach, doing something like: {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder()); {% if attribute.cpp_value_original %} {{attribute.cpp_type}} original = {{attribute.cpp_value_original}}; {% else %} {{attribute.cpp_type}} original = {{attribute.cpp_value}}; {% endif %} v8::Handle<v8::Value> originalValue = {{cpp_to_v8_conversion}}; contextData->activityLogger()->logSetter("{{interface_name}}.{{attribute.name}}, jsValue, originalValue); almost works, with the problem that {{cpp_to_v8_conversion}} doesn't exist (at least, not here). Is there an easy way to solve that (or does it already exist, under a name I couldn't find)?
On 2014/03/27 20:35:29, D Cronin wrote:
> We might be able to just call *AttributeGetter, but we'd need to pass in some
> form of callback, right? Can we do that from here safely?
haraken?
(Goal is to log current value when logging in *setter*, which requires
calling Getter or refactoring Python and template to create necessary code.)
> For the other approach, doing something like:
>
> {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder());
> {% if attribute.cpp_value_original %}
> {{attribute.cpp_type}} original = {{attribute.cpp_value_original}};
> {% else %}
> {{attribute.cpp_type}} original = {{attribute.cpp_value}};
> {% endif %}
> v8::Handle<v8::Value> originalValue = {{cpp_to_v8_conversion}};
>
contextData->activityLogger()->logSetter("{{interface_name}}.{{attribute.name}},
> jsValue, originalValue);
>
> almost works, with the problem that {{cpp_to_v8_conversion}} doesn't exist (at
> least, not here).
This should be:
v8::Handle<v8::Value> originalValue = {{cpp_value_to_v8_value}};
...as {{cpp_to_v8_conversion}} is the full line, not just the expression.
This was poor style, so I've fixed it in this CL:
https://codereview.chromium.org/215993002/
> Is there an easy way to solve that (or does it already exist,
> under a name I couldn't find)?
It's fine to add a 'cpp_value_to_v8_value' variable to attribute context
(i.e., to the dictionary "display" in v8_attributes.generate_attribute),
following the existing use in v8_callback_interface, and then you can use it
as you detail above.
Before discussing the implementation, I might want to have pmarch@ review this CL. Since changes to ActivityLogger are security-sensitive (and thus the Blink community is not really happy to add more functionality to ActivityLogger), I want to make sure that this change is necessary for ActivityLogger purpose. Cronin: Would you explain why you need this change in the CL description? In what feature is this change needed?
On 2014/03/28 01:44:46, haraken wrote: > Before discussing the implementation, I might want to have pmarch@ review this > CL. > > Since changes to ActivityLogger are security-sensitive (and thus the Blink > community is not really happy to add more functionality to ActivityLogger), I > want to make sure that this change is necessary for ActivityLogger purpose. > > Cronin: Would you explain why you need this change in the CL description? In > what feature is this change needed? @haraken: We're currently looking into using this to detect ad injection. In particular, we want to find out if an extension is modifying, e.g., an iframe src attribute from one ad network to another (though we can think of other times when this information could be useful). Petr (pmarch@) has been working with me on a few different aspects of this, and is already on the reviewer list. CL description updated to be a bit more informative. :)
On 2014/03/28 04:28:55, D Cronin wrote: > On 2014/03/28 01:44:46, haraken wrote: > > Before discussing the implementation, I might want to have pmarch@ review this > > CL. > > > > Since changes to ActivityLogger are security-sensitive (and thus the Blink > > community is not really happy to add more functionality to ActivityLogger), I > > want to make sure that this change is necessary for ActivityLogger purpose. > > > > Cronin: Would you explain why you need this change in the CL description? In > > what feature is this change needed? > > @haraken: > We're currently looking into using this to detect ad injection. In particular, > we want to find out if an extension is modifying, e.g., an iframe src attribute > from one ad network to another (though we can think of other times when this > information could be useful). Petr (pmarch@) has been working with me on a few > different aspects of this, and is already on the reviewer list. > > CL description updated to be a bit more informative. :) Friendly ping on this?
On 2014/04/02 13:53:58, D Cronin wrote: > Friendly ping on this? pmarch@, haraken@ ?
On 2014/04/03 00:00:21, Nils Barth wrote: > On 2014/04/02 13:53:58, D Cronin wrote: > > Friendly ping on this? > > pmarch@, haraken@ ? Ping again?
On 2014/04/11 22:42:17, D Cronin wrote: > On 2014/04/03 00:00:21, Nils Barth wrote: > > On 2014/04/02 13:53:58, D Cronin wrote: > > > Friendly ping on this? > > > > pmarch@, haraken@ ? > > Ping again? Kentaro: Yes, this feature is necessary for ActivityLog and detection of ad injection. Currently, detection of ad injections is a high priority task among several security teams. To reduce the performance overhead, I have the following suggestions: 1) Since, we should only care about specific HTML elements (e.g., HTMLIFrameElement) and specific attributes (e.g., src), we should introduce a new IDL directive, e.g., ActivityLoggingOldValueForSetter, and mark required properties in IDL files of required HTML elements. Then, we add the oldValue only to those setters that attributed with the new directive in their IDL files. 2) Alternatively, if we only care about URLs, in the binding generating code we can check if the setter deals with a URL, by checking URL directive, and then generate oldValue-code only for these setters. I am in favor of #1 Devlin: Please, run ./Tools/Scripts/run-bindings-tests --reset-results to regenerate all test results not just V8TestObject.cpp and V8TestObjectPython.cpp
https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/1/Source/bindings/templates/at... Source/bindings/templates/attributes.cpp:303: v8::Handle<v8::Value> HACK; I am not an expert on this, but it looks to me that {{cpp_to_v8_conversion}} which can convert arbitrary cpp values to JS values does not exist. You need to check the type of oldValue and generate conversion code specific to this type. The conversion code needs to use V8 API to produce JS values. From my previous comment, if you chose to track only HTMLIFrameElement.src then the oldValue type is sting and you need to create an external string. Note that V8 binding code use cache for fast string conversions which would be nice to use as well. See https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... On 2014/03/27 20:35:30, D Cronin wrote: > On 2014/03/27 02:51:25, Nils Barth wrote: > > On 2014/03/26 22:58:42, D Cronin wrote: > > > I'm a bit at a loss for how to best get the current value. > > > But I'm missing a few things in there (looking at the attribute_getter code > at > > > the top, I know there's a lot of subtle things here)... > > > > Simplest would be if you could just call the *AttributeGetter function, no? > > > > Failing that, {{attribute.cpp_value}} will generally hold the value > > (as a C++ expression), but if {{attribute.cpp_value_original}} exists > > you'll need to use that instead (sometimes store value in a local > > variable). > > Could you see if this works? > > (I've never worked on the bindings before, so I might be missing something very > obvious. If so, my apologies.) > > We might be able to just call *AttributeGetter, but we'd need to pass in some > form of callback, right? Can we do that from here safely? > > For the other approach, doing something like: > > {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder()); > {% if attribute.cpp_value_original %} > {{attribute.cpp_type}} original = {{attribute.cpp_value_original}}; > {% else %} > {{attribute.cpp_type}} original = {{attribute.cpp_value}}; > {% endif %} > v8::Handle<v8::Value> originalValue = {{cpp_to_v8_conversion}}; > contextData->activityLogger()->logSetter("{{interface_name}}.{{attribute.name}}, > jsValue, originalValue); > > almost works, with the problem that {{cpp_to_v8_conversion}} doesn't exist (at > least, not here). Is there an easy way to solve that (or does it already exist, > under a name I couldn't find)?
not lgtm Before adding more functionality to the activity logger, I think we should have a design review of what people would like to add, why they would like to add it, and what the costs are for adding those features. I'm not sure who all the relevant folks are to involve, but I would like to participate in the review.
To summarize, AFAICT: * This needs design review and discussion, but should be done in some form. * This is high-priority item (useful for detecting ad injection) (Petr). * ...but has performance and complexity costs (Adam). * ...so we don't want to just patch on one feature. The fact that it's requires significant new code (setter now calling getter) reflects that there is real complexity here. Also, activity logging is generally sensitive, so we should air concerns early. Devlin, Petr: could you circulate a design doc? (Adam, haraken: what's suitable? blink-dev@?) To reiterate: I understand there are good reasons to do this; just want to make sure we understand the consequences and any concerns first.
(Oh, see we've got a design review scheduled, which should handle this.)
On 2014/04/18 01:07:19, Nils Barth wrote: > (Oh, see we've got a design review scheduled, which should handle this.) Based on the discussion in the design review, I filed two bugs: https://code.google.com/p/chromium/issues/detail?id=365500 https://code.google.com/p/chromium/issues/detail?id=365504 I don't think those bugs need to block this CL. Presumably the next step for this review is to prepare a version that doesn't have a local variable named HACK. Once you do that, I can look in more detail.
All - working patch uploaded. Per Petr's advice, this introduces a new extended attribute tag property for including the old value in setter logs, and only does the work in that case. Also reuses code for cpp -> v8 conversion in what appears to be a reasonable way -- that said, my knowledge of what is "reasonable" here is fairly limited, since this is my first foray into bindings stuff. :) Please take a look. Cheers. https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter This is getting a bit crazy - do we want to split these up?
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/22 17:47:09, D Cronin wrote: > This is getting a bit crazy - do we want to split these up? Yeah, there seem to be several bits conflated here. Can you split these up into separate attributes? https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_utilities.py:156: .find('IncludeOldValueForSetter') != -1) This line of code is a sign that IncludeOldValueForSetter should be a separate IDL attribute. https://codereview.chromium.org/213783002/diff/40001/Source/bindings/v8/V8DOM... File Source/bindings/v8/V8DOMActivityLogger.h (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/v8/V8DOM... Source/bindings/v8/V8DOMActivityLogger.h:45: virtual void log(const String& apiName, int argc, const v8::Handle<v8::Value>* argv, const String& extraInfo) { } Why is this needed to chrome compilation? Code outside of Blink should not depend directly on this header. https://codereview.chromium.org/213783002/diff/40001/Source/core/html/HTMLEmb... File Source/core/html/HTMLEmbedElement.idl (right): https://codereview.chromium.org/213783002/diff/40001/Source/core/html/HTMLEmb... Source/core/html/HTMLEmbedElement.idl:27: [Reflect, URL, PerWorldBindings, ActivityLogging=SetterForIsolatedWorldsIncludeOldValueForSetter] attribute DOMString src; Can't someone circumvent this by just using setAttribute ? https://codereview.chromium.org/213783002/diff/40001/public/web/WebDOMActivit... File public/web/WebDOMActivityLogger.h (right): https://codereview.chromium.org/213783002/diff/40001/public/web/WebDOMActivit... public/web/WebDOMActivityLogger.h:46: virtual void log(const WebString& apiName, int argc, const v8::Handle<v8::Value>* argv, const WebString& extraInfo, const WebURL& url, const WebString& title) { } It makes sense that this function is needed during the transition, but that's because it's in the |public| folder.
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/22 23:35:51, abarth wrote: > On 2014/04/22 17:47:09, D Cronin wrote: > > This is getting a bit crazy - do we want to split these up? > > Yeah, there seem to be several bits conflated here. Can you split these up into > separate attributes? Done, per offline conversation, decided on one attribute each for the type of activity, the worlds we log, and whether or not to include the old value. As expected, this resulted in a bit of noise in the CL. Apologies. https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_utilities.py:156: .find('IncludeOldValueForSetter') != -1) On 2014/04/22 23:35:51, abarth wrote: > This line of code is a sign that IncludeOldValueForSetter should be a separate > IDL attribute. Done. https://codereview.chromium.org/213783002/diff/40001/Source/bindings/v8/V8DOM... File Source/bindings/v8/V8DOMActivityLogger.h (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/v8/V8DOM... Source/bindings/v8/V8DOMActivityLogger.h:45: virtual void log(const String& apiName, int argc, const v8::Handle<v8::Value>* argv, const String& extraInfo) { } On 2014/04/22 23:35:51, abarth wrote: > Why is this needed to chrome compilation? Code outside of Blink should not > depend directly on this header. Whoops! Meant to only leave in the one in public/web/WebDOMActivityLogger.h. Removed. https://codereview.chromium.org/213783002/diff/40001/Source/core/html/HTMLEmb... File Source/core/html/HTMLEmbedElement.idl (right): https://codereview.chromium.org/213783002/diff/40001/Source/core/html/HTMLEmb... Source/core/html/HTMLEmbedElement.idl:27: [Reflect, URL, PerWorldBindings, ActivityLogging=SetterForIsolatedWorldsIncludeOldValueForSetter] attribute DOMString src; On 2014/04/22 23:35:51, abarth wrote: > Can't someone circumvent this by just using setAttribute ? Sadly, yes (neither caused nor helped by this CL). We'll need to address that. https://codereview.chromium.org/213783002/diff/40001/public/web/WebDOMActivit... File public/web/WebDOMActivityLogger.h (right): https://codereview.chromium.org/213783002/diff/40001/public/web/WebDOMActivit... public/web/WebDOMActivityLogger.h:46: virtual void log(const WebString& apiName, int argc, const v8::Handle<v8::Value>* argv, const WebString& extraInfo, const WebURL& url, const WebString& title) { } On 2014/04/22 23:35:51, abarth wrote: > It makes sense that this function is needed during the transition, but that's > because it's in the |public| folder. Right. I'll take it out as soon as we're done with the transition.
Thanks for revisions! A few suggestions. https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/22 23:35:51, abarth wrote: > On 2014/04/22 17:47:09, D Cronin wrote: > > This is getting a bit crazy - do we want to split these up? > > Yeah, there seem to be several bits conflated here. Can you split these up into > separate attributes? Combinatorial explosion (>.<) We can keep one attribute but have a list instead: ActivityLogging=Getter|Setter|ForAllWorlds|ForIsolatedWorlds|OldValueForSetter (I'm happy to do splitting in separate CL if that's desired.) haraken@? https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_utilities.py:156: .find('IncludeOldValueForSetter') != -1) On 2014/04/22 23:35:51, abarth wrote: > This line of code is a sign that IncludeOldValueForSetter should be a separate > IDL attribute. Python note: You can use |in| for substring searches: 'IncludeOldValueForSetter' in member.extended_attributes['ActivityLogging'] Anyway, as Adam notes, we should split up ActivityLogging (I'd suggest into a list of values), and then we can use the existing utility function v8_utilites.has_extended_attribute_value ...to check the list, as: has_extended_attribute_value(member, 'ActivityLogging', 'IncludeOldValueForSetter') (inline, no need for auxiliary function) https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... Source/bindings/templates/attributes.cpp:321: v8::Handle<v8::Value> originalValue = {{attribute.cpp_value_to_v8_value(cpp_value='original', isolate='info.GetIsolate()', creation_context='v8::Handle<v8::Object>()')}}; Could you put the function call in the Python code, not the template? https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... Source/bindings/templates/attributes.cpp:323: {# If we don't log the old value, we send an empty value in its place. #} Would it be clearer to overload logSetter so we can just omit sending a dummy value?
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/23 01:12:02, Nils Barth wrote: > On 2014/04/22 23:35:51, abarth wrote: > > On 2014/04/22 17:47:09, D Cronin wrote: > > > This is getting a bit crazy - do we want to split these up? > > > > Yeah, there seem to be several bits conflated here. Can you split these up > into > > separate attributes? > > Combinatorial explosion (>.<) > We can keep one attribute but have a list instead: > ActivityLogging=Getter|Setter|ForAllWorlds|ForIsolatedWorlds|OldValueForSetter > > (I'm happy to do splitting in separate CL if that's desired.) > haraken@? Already fixed per Adam's comment. Does the new format look okay? https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_utilities.py:156: .find('IncludeOldValueForSetter') != -1) On 2014/04/23 01:12:02, Nils Barth wrote: > On 2014/04/22 23:35:51, abarth wrote: > > This line of code is a sign that IncludeOldValueForSetter should be a separate > > IDL attribute. > > Python note: > You can use |in| for substring searches: > 'IncludeOldValueForSetter' in member.extended_attributes['ActivityLogging'] > > Anyway, as Adam notes, we should split up ActivityLogging > (I'd suggest into a list of values), and then we can use the > existing utility function > v8_utilites.has_extended_attribute_value > ...to check the list, as: > has_extended_attribute_value(member, 'ActivityLogging', > 'IncludeOldValueForSetter') > (inline, no need for auxiliary function) Already fixed, but thanks for the tips! :) https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... Source/bindings/templates/attributes.cpp:321: v8::Handle<v8::Value> originalValue = {{attribute.cpp_value_to_v8_value(cpp_value='original', isolate='info.GetIsolate()', creation_context='v8::Handle<v8::Object>()')}}; On 2014/04/23 01:12:02, Nils Barth wrote: > Could you put the function call in the Python code, not the template? Yes. But I didn't because there's so much context that's lost when that happens (it's unclear then what 'original' and 'info.GetIsolate()' refer to). But if it's preferred, I have no problem doing so. Preference? (Made the change, but happy to change back.) https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... Source/bindings/templates/attributes.cpp:323: {# If we don't log the old value, we send an empty value in its place. #} On 2014/04/23 01:12:02, Nils Barth wrote: > Would it be clearer to overload logSetter so we can just omit sending a dummy > value? Sure. (Originally didn't because chrome doesn't like overloading, but this isn't chrome.) :)
(adding haraken@ back)
Some followups; main request is to split off the extended attribute renaming into a separate CL, otherwise fine. https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/23 01:31:43, D Cronin wrote: > Already fixed per Adam's comment. Does the new format look okay? New format is fine, thanks! Could you please post this in a separate CL, to simplify this CL? (i.e., the extended attribute renaming, without adding new feature) https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... Source/bindings/templates/attributes.cpp:321: v8::Handle<v8::Value> originalValue = {{attribute.cpp_value_to_v8_value(cpp_value='original', isolate='info.GetIsolate()', creation_context='v8::Handle<v8::Object>()')}}; On 2014/04/23 01:31:43, D Cronin wrote: > On 2014/04/23 01:12:02, Nils Barth wrote: > > Could you put the function call in the Python code, not the template? > > Yes. But I didn't because there's so much context that's lost when that happens > (it's unclear then what 'original' and 'info.GetIsolate()' refer to). That's a good point, but as a rule we're putting all these calls in the Python logic, to avoid Python ⇄ Jinja cycles: this way you can read the Jinja and just refer to Python to understand variable values, rather than pingponging; also simplifies the template, which is otherwise a bit hard to read. (Your suggestion is sane, but not how we've been doing it: we've been keeping a strict pipeline.) I've documented this at: http://www.chromium.org/developers/jinja#TOC-Python-Jinja-split In this case the only context is the variable name 'original' (isolate is default), and we've considered that acceptable. Thanks for your explanation! https://codereview.chromium.org/213783002/diff/40001/Source/bindings/template... Source/bindings/templates/attributes.cpp:323: {# If we don't log the old value, we send an empty value in its place. #} On 2014/04/23 01:31:43, D Cronin wrote: > (Originally didn't because chrome doesn't like overloading, but this > isn't chrome.) :) ...at least for the next day or two ;) https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:85: 'activity_logging_include_old_value_for_setter': ('LogPreviousValue' in extended_attributes), # [ActivityLogging] No parentheses. (Only need if doing line continuation, which you don't need here.) https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', isolate='info.GetIsolate()', creation_context='v8::Handle<v8::Object>()'), Don't need isolate='info.GetIsolate()', as that's the default value. https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:136: if 'LogActivity' not in member.extended_attributes: BTW, could you add extended_attributes = member.extended_attributes here? (As a rule we're using a local var if used 2+ times.) https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:139: if (log_activity == 'GetterOnly' and access_type != 'Getter') or (log_activity == 'SetterOnly' and access_type != 'Setter'): This could be: if log_activity and not log_activity.startswith(access_type): (changing default to access_type='') If we remove the 'Only', this could simply be: if log_activity and log_activity != access_type: https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:147: return world_list return set(world_list) I'd tend to prefer avoiding mutation: if 'LogAllWorlds' in extended_attributes: return set(['', 'ForMainWorld']) return set(['']) # At minimum, include isolated worlds.
https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/23 03:04:42, Nils Barth wrote: > On 2014/04/23 01:31:43, D Cronin wrote: > > Already fixed per Adam's comment. Does the new format look okay? > > New format is fine, thanks! > Could you please post this in a separate CL, to simplify this CL? > (i.e., the extended attribute renaming, without adding new feature) Done, https://codereview.chromium.org/249833002/ https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:85: 'activity_logging_include_old_value_for_setter': ('LogPreviousValue' in extended_attributes), # [ActivityLogging] On 2014/04/23 03:04:42, Nils Barth wrote: > No parentheses. > (Only need if doing line continuation, which you don't need here.) Done. https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', isolate='info.GetIsolate()', creation_context='v8::Handle<v8::Object>()'), On 2014/04/23 03:04:42, Nils Barth wrote: > Don't need isolate='info.GetIsolate()', as that's the default value. Ah, so it is. Removed. https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:136: if 'LogActivity' not in member.extended_attributes: On 2014/04/23 03:04:42, Nils Barth wrote: > BTW, could you add > extended_attributes = member.extended_attributes > here? > (As a rule we're using a local var if used 2+ times.) Done (in other cl). https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:139: if (log_activity == 'GetterOnly' and access_type != 'Getter') or (log_activity == 'SetterOnly' and access_type != 'Setter'): On 2014/04/23 03:04:42, Nils Barth wrote: > This could be: > if log_activity and not log_activity.startswith(access_type): > > (changing default to access_type='') > If we remove the 'Only', this could simply be: > if log_activity and log_activity != access_type: Done (in other cl). My inclination is to keep GetterOnly/SetterOnly, because it makes it more clear that they are values which limit (rather than extend) the default behavior. But I don't feel strongly, so if 'Getter'/'Setter' is preferred, happy to do so. :) https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:147: return world_list On 2014/04/23 03:04:42, Nils Barth wrote: > return set(world_list) > I'd tend to prefer avoiding mutation: > if 'LogAllWorlds' in extended_attributes: > return set(['', 'ForMainWorld']) > return set(['']) # At minimum, include isolated worlds. Done (in other cl).
Thanks, LGTM CG + templates-wise! haraken, abarth: how are semantics? https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/40001/Source/bindings/IDLExten... Source/bindings/IDLExtendedAttributes.txt:37: ActivityLogging=ForAllWorlds|GetterForAllWorlds|SetterForAllWorlds|ForIsolatedWorlds|GetterForIsolatedWorlds|SetterForIsolatedWorlds|ForAllWorlsIncludeOldValueForSetter|SetterForAllWorldsIncludeOldValueForSetter|ForIsolatedWorldsIncludeOldValueForSetter|SetterForIsolatedWorldsIncludeOldValueForSetter On 2014/04/23 18:28:02, D Cronin wrote: > Done, https://codereview.chromium.org/249833002/ Thanks! https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... File Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/213783002/diff/120001/Source/bindings/scripts... Source/bindings/scripts/v8_utilities.py:139: if (log_activity == 'GetterOnly' and access_type != 'Getter') or (log_activity == 'SetterOnly' and access_type != 'Setter'): On 2014/04/23 18:28:02, D Cronin wrote: > My inclination is to keep GetterOnly/SetterOnly, because it > makes it more clear that they are values which limit (rather than extend) the > default behavior. That's fine, and agreed that explicit is better here. Clearer IDL files at v. minor Python complexity expense is the right trade-off.
We did a design review, and this CL looks OK as a short-term fix. I've posted an email to related folks about how the ActivityLogger mechanism can be improved. We're planning to work on the improvement in a reasonable timeline, so this CL LGTM. Please wait for an approval from abarth@. https://codereview.chromium.org/213783002/diff/140001/Source/bindings/IDLExte... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/IDLExte... Source/bindings/IDLExtendedAttributes.txt:66: LogPreviousValue Add a test about [LogPreviousValue] to run-bindings-tests (e.g., TestObject.idl). https://codereview.chromium.org/213783002/diff/140001/Source/bindings/templat... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/templat... Source/bindings/templates/attributes.cpp:313: if (contextData && contextData->activityLogger()) { Not related to this CL, it's mis-designed that you cache activityLogger() onto V8PerContextData. Conceptually it should be put on DOMWrapperWorld. When ataly@ implemented the ActivityLogger, we had to cache activityLogger() onto V8PerContextData because DOMWrapperWorld::current() was too slow. Now that I've fixed the performance, I think you can simply use: if (DOMWrapperWorld::current(isolate)->activityLogger()) { ...; } You can fix it in a follow-up CL.
https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', creation_context='v8::Handle<v8::Object>()'), v8::Handle<v8::Object>() isn't the right ceration_context. That's an empty handle, which means we'll use the wrong creation context... https://codereview.chromium.org/213783002/diff/140001/public/web/WebDOMActivi... File public/web/WebDOMActivityLogger.h (right): https://codereview.chromium.org/213783002/diff/140001/public/web/WebDOMActivi... public/web/WebDOMActivityLogger.h:46: virtual void log(const WebString& apiName, int argc, const v8::Handle<v8::Value>* argv, const WebString& extraInfo, const WebURL& url, const WebString& title) { } You should be able to remove this function. Chromium shouldn't be using the OVERRIDE keyword, which means it won't notice if this function disappears from this base class.
https://codereview.chromium.org/213783002/diff/140001/Source/bindings/IDLExte... File Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/IDLExte... Source/bindings/IDLExtendedAttributes.txt:66: LogPreviousValue On 2014/04/24 01:10:57, haraken wrote: > > Add a test about [LogPreviousValue] to run-bindings-tests (e.g., > TestObject.idl). Done. https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', creation_context='v8::Handle<v8::Object>()'), On 2014/04/24 14:48:22, abarth wrote: > v8::Handle<v8::Object>() isn't the right ceration_context. That's an empty > handle, which means we'll use the wrong creation context... Is the correct context '' then, as it's the default (tentatively done)? Or something else? (Sorry, I'm still a bit ignorant about a lot of these parts.) https://codereview.chromium.org/213783002/diff/140001/Source/bindings/templat... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/templat... Source/bindings/templates/attributes.cpp:313: if (contextData && contextData->activityLogger()) { On 2014/04/24 01:10:57, haraken wrote: > > Not related to this CL, it's mis-designed that you cache activityLogger() onto > V8PerContextData. Conceptually it should be put on DOMWrapperWorld. > > When ataly@ implemented the ActivityLogger, we had to cache activityLogger() > onto V8PerContextData because DOMWrapperWorld::current() was too slow. Now that > I've fixed the performance, I think you can simply use: > > if (DOMWrapperWorld::current(isolate)->activityLogger()) { > ...; > } > > You can fix it in a follow-up CL. Okay, I'll put up a CL for that soon. :) https://codereview.chromium.org/213783002/diff/140001/public/web/WebDOMActivi... File public/web/WebDOMActivityLogger.h (right): https://codereview.chromium.org/213783002/diff/140001/public/web/WebDOMActivi... public/web/WebDOMActivityLogger.h:46: virtual void log(const WebString& apiName, int argc, const v8::Handle<v8::Value>* argv, const WebString& extraInfo, const WebURL& url, const WebString& title) { } On 2014/04/24 14:48:22, abarth wrote: > You should be able to remove this function. Chromium shouldn't be using the > OVERRIDE keyword, which means it won't notice if this function disappears from > this base class. Chromium does use the OVERRIDE keyword. Relevant instance here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
On 2014/04/24 16:40:34, D Cronin wrote: > Chromium does use the OVERRIDE keyword. Relevant instance here: Please remove the OVERRIDE keyword from Chromium. We generally don't use OVERRIDE for virtual functions on Blink API objects because it makes the API even harder to change.
On 2014/04/24 16:40:34, D Cronin wrote: > https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... > File Source/bindings/scripts/v8_attributes.py (right): > > https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... > Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': > idl_type.cpp_value_to_v8_value(cpp_value='original', > creation_context='v8::Handle<v8::Object>()'), > On 2014/04/24 14:48:22, abarth wrote: > > v8::Handle<v8::Object>() isn't the right ceration_context. That's an empty > > handle, which means we'll use the wrong creation context... > > Is the correct context '' then, as it's the default (tentatively done)? Or > something else? (Sorry, I'm still a bit ignorant about a lot of these parts.) I'm going to defer to haraken and nbarth on this question. In the past, every instance of passing an empty handle as the creation context was a bug. I'm not sure if that's still true.
On 2014/04/24 16:58:43, abarth wrote: > On 2014/04/24 16:40:34, D Cronin wrote: > > Chromium does use the OVERRIDE keyword. Relevant instance here: > > Please remove the OVERRIDE keyword from Chromium. We generally don't use > OVERRIDE for virtual functions on Blink API objects because it makes the API > even harder to change. Removed log method here; I'll push through the chrome change asap (since this obviously can't land until that's in). Anything else (apart from the standing creation_context question)?
On 2014/04/24 17:23:45, D Cronin wrote: > Anything else (apart from the standing creation_context question)? Nope, after than I'm happy. :)
https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', creation_context='v8::Handle<v8::Object>()'), On 2014/04/24 16:40:35, D Cronin wrote: > On 2014/04/24 14:48:22, abarth wrote: > > v8::Handle<v8::Object>() isn't the right ceration_context. That's an empty > > handle, which means we'll use the wrong creation context... > > Is the correct context '' then, as it's the default (tentatively done)? Or > something else? (Sorry, I'm still a bit ignorant about a lot of these parts.) +haraken? (What's the correct creation context here?)
https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', creation_context='v8::Handle<v8::Object>()'), On 2014/04/25 01:45:41, Nils Barth wrote: > On 2014/04/24 16:40:35, D Cronin wrote: > > On 2014/04/24 14:48:22, abarth wrote: > > > v8::Handle<v8::Object>() isn't the right ceration_context. That's an empty > > > handle, which means we'll use the wrong creation context... > > > > Is the correct context '' then, as it's the default (tentatively done)? Or > > something else? (Sorry, I'm still a bit ignorant about a lot of these parts.) > > +haraken? > (What's the correct creation context here?) I got confused. I cannot find any "v8::Handle<v8::Object>()" in tests/results/V8TestObject.cpp. Is cpp_value_to_v8_value used?
https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... File Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(cpp_value='original', creation_context='v8::Handle<v8::Object>()'), On 2014/04/25 01:50:49, haraken wrote: > I got confused. > > I cannot find any "v8::Handle<v8::Object>()" in tests/results/V8TestObject.cpp. That's a good point. > Is cpp_value_to_v8_value used? Yes, but creation_context is only used for interface types. Devlin, could you also add a test case that uses an interface type to test this??
On 2014/04/25 01:50:48, haraken wrote: > https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... > File Source/bindings/scripts/v8_attributes.py (right): > > https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... > Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': > idl_type.cpp_value_to_v8_value(cpp_value='original', > creation_context='v8::Handle<v8::Object>()'), > On 2014/04/25 01:45:41, Nils Barth wrote: > > On 2014/04/24 16:40:35, D Cronin wrote: > > > On 2014/04/24 14:48:22, abarth wrote: > > > > v8::Handle<v8::Object>() isn't the right ceration_context. That's an > empty > > > > handle, which means we'll use the wrong creation context... > > > > > > Is the correct context '' then, as it's the default (tentatively done)? Or > > > something else? (Sorry, I'm still a bit ignorant about a lot of these > parts.) > > > > +haraken? > > (What's the correct creation context here?) > > I got confused. > > I cannot find any "v8::Handle<v8::Object>()" in tests/results/V8TestObject.cpp. > Is cpp_value_to_v8_value used? In the latest patch, I removed the "creation_context='v8::Handle<v8::Object>()'", and left the default of '' (since Adam mentioned that v8::Handle<v8::Object>() was incorrect, this was my next best guess).
On 2014/04/25 01:55:38, D Cronin wrote: > On 2014/04/25 01:50:48, haraken wrote: > > > https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... > > File Source/bindings/scripts/v8_attributes.py (right): > > > > > https://codereview.chromium.org/213783002/diff/140001/Source/bindings/scripts... > > Source/bindings/scripts/v8_attributes.py:92: 'cpp_value_to_v8_value': > > idl_type.cpp_value_to_v8_value(cpp_value='original', > > creation_context='v8::Handle<v8::Object>()'), > > On 2014/04/25 01:45:41, Nils Barth wrote: > > > On 2014/04/24 16:40:35, D Cronin wrote: > > > > On 2014/04/24 14:48:22, abarth wrote: > > > > > v8::Handle<v8::Object>() isn't the right ceration_context. That's an > > empty > > > > > handle, which means we'll use the wrong creation context... > > > > > > > > Is the correct context '' then, as it's the default (tentatively done)? > Or > > > > something else? (Sorry, I'm still a bit ignorant about a lot of these > > parts.) > > > > > > +haraken? > > > (What's the correct creation context here?) > > > > I got confused. > > > > I cannot find any "v8::Handle<v8::Object>()" in > tests/results/V8TestObject.cpp. > > Is cpp_value_to_v8_value used? > > In the latest patch, I removed the > "creation_context='v8::Handle<v8::Object>()'", and left the default of '' (since > Adam mentioned that v8::Handle<v8::Object>() was incorrect, this was my next > best guess). Please don't write code based on guess. If you need a creation context, we need to set a correct creation context. If you don't need cpp_value_to_v8_value, we can just drop cpp_value_to_v8_value. As Nils mentioned, would you add a test that uses the code path? It's hard to give you advice without seeing actually generated code.
On 2014/04/25 01:58:45, haraken wrote: > If you need a creation context, we need to set a correct creation context. > If you don't need cpp_value_to_v8_value, we can just drop cpp_value_to_v8_value. cpp_value_to_v8_value is definitely used; however, the issue is how to get this for interface types. If we're only logging non-interface types (like DOMStrings), we should explicitly disallow non-interface types; if we are logging interface types, we'll need to figure out how it's being used.
On 2014/04/25 03:14:17, Nils Barth wrote: > On 2014/04/25 01:58:45, haraken wrote: > > If you need a creation context, we need to set a correct creation context. > > If you don't need cpp_value_to_v8_value, we can just drop > cpp_value_to_v8_value. > > cpp_value_to_v8_value is definitely used; > however, the issue is how to get this for interface types. > If we're only logging non-interface types (like DOMStrings), we should > explicitly disallow non-interface types; > if we are logging interface types, we'll need to figure out how it's being used. Sorry; still learning as I go (didn't realize that creation_context) was only used with interface types). A new test has been added for an interface type. CreationContext of info.Holder()->CreationContext() seems correct, but please check.
On 2014/04/25 19:46:26, D Cronin wrote: > On 2014/04/25 03:14:17, Nils Barth wrote: > > On 2014/04/25 01:58:45, haraken wrote: > > > If you need a creation context, we need to set a correct creation context. > > > If you don't need cpp_value_to_v8_value, we can just drop > > cpp_value_to_v8_value. > > > > cpp_value_to_v8_value is definitely used; > > however, the issue is how to get this for interface types. > > If we're only logging non-interface types (like DOMStrings), we should > > explicitly disallow non-interface types; > > if we are logging interface types, we'll need to figure out how it's being > used. > > Sorry; still learning as I go (didn't realize that creation_context) was only > used with interface types). A new test has been added for an interface type. > CreationContext of info.Holder()->CreationContext() seems correct, but please > check. Thanks, looks good. You can use info.Holder() instead of info.Holder()->CreationContext(). (Sorry, the name "creationContext" is confusing. For creationContext, You're expected to specify "an object created from the context". In that sense, info.Holder()->CreationContext() is of course fine, but info.Holder() is enough:-)
On 2014/04/25 23:46:38, haraken wrote: > On 2014/04/25 19:46:26, D Cronin wrote: > > On 2014/04/25 03:14:17, Nils Barth wrote: > > > On 2014/04/25 01:58:45, haraken wrote: > > > > If you need a creation context, we need to set a correct creation context. > > > > If you don't need cpp_value_to_v8_value, we can just drop > > > cpp_value_to_v8_value. > > > > > > cpp_value_to_v8_value is definitely used; > > > however, the issue is how to get this for interface types. > > > If we're only logging non-interface types (like DOMStrings), we should > > > explicitly disallow non-interface types; > > > if we are logging interface types, we'll need to figure out how it's being > > used. > > > > Sorry; still learning as I go (didn't realize that creation_context) was only > > used with interface types). A new test has been added for an interface type. > > CreationContext of info.Holder()->CreationContext() seems correct, but please > > check. > > Thanks, looks good. You can use info.Holder() instead of > info.Holder()->CreationContext(). > > (Sorry, the name "creationContext" is confusing. For creationContext, You're > expected to specify "an object created from the context". In that sense, > info.Holder()->CreationContext() is of course fine, but info.Holder() is > enough:-) Okay, s/info.Holder()->CreationContext()/info.Holder(). Thanks for the help. :) Adam, wanna take another look?
lgtm
Chrome support has gone through, so going to push this in now.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/213783002/...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/bindings/templates/attributes.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/bindings/templates/attributes.cpp
Hunk #1 FAILED at 171.
Hunk #2 FAILED at 311.
2 out of 2 hunks FAILED -- saving rejects to file
Source/bindings/templates/attributes.cpp.rej
Patch: Source/bindings/templates/attributes.cpp
Index: Source/bindings/templates/attributes.cpp
diff --git a/Source/bindings/templates/attributes.cpp
b/Source/bindings/templates/attributes.cpp
index
9badf4e1637be08d03ffa0ef528f2fa2c24f9d73..729fdfb5b43c274c90d2d25b650ca2d3abc112f5
100644
--- a/Source/bindings/templates/attributes.cpp
+++ b/Source/bindings/templates/attributes.cpp
@@ -171,7 +171,7 @@ v8::Local<v8::String>, const
v8::PropertyCallbackInfo<v8::Value>& info
{% if world_suffix in attribute.activity_logging_world_list_for_getter %}
V8PerContextData* contextData =
V8PerContextData::from(info.GetIsolate()->GetCurrentContext());
if (contextData && contextData->activityLogger())
-
contextData->activityLogger()->log("{{interface_name}}.{{attribute.name}}", 0,
0, "Getter");
+
contextData->activityLogger()->logGetter("{{interface_name}}.{{attribute.name}}");
{% endif %}
{% if attribute.has_custom_getter %}
{{v8_class}}::{{attribute.name}}AttributeGetterCustom(info);
@@ -311,8 +311,18 @@ v8::Local<v8::String>, v8::Local<v8::Value> v8Value, const
v8::PropertyCallbackI
{% if world_suffix in attribute.activity_logging_world_list_for_setter %}
V8PerContextData* contextData =
V8PerContextData::from(info.GetIsolate()->GetCurrentContext());
if (contextData && contextData->activityLogger()) {
- v8::Handle<v8::Value> loggerArg[] = { v8Value };
-
contextData->activityLogger()->log("{{interface_name}}.{{attribute.name}}", 1,
&loggerArg[0], "Setter");
+ {% if attribute.activity_logging_include_old_value_for_setter %}
+ {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder());
+ {% if attribute.cpp_value_original %}
+ {{attribute.cpp_type}} original = {{attribute.cpp_value_original}};
+ {% else %}
+ {{attribute.cpp_type}} original = {{attribute.cpp_value}};
+ {% endif %}
+ v8::Handle<v8::Value> originalValue =
{{attribute.cpp_value_to_v8_value}};
+
contextData->activityLogger()->logSetter("{{interface_name}}.{{attribute.name}}",
v8Value, originalValue);
+ {% else %}
+
contextData->activityLogger()->logSetter("{{interface_name}}.{{attribute.name}}",
v8Value);
+ {% endif %}
}
{% endif %}
{% if attribute.is_custom_element_callbacks or attribute.is_reflect %}
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/213783002/...
Message was sent while issue was closed.
Change committed as 173227 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
