Bug 75313 - [V8][Chromium] 'randomString' in document.body.style always returns true
Summary: [V8][Chromium] 'randomString' in document.body.style always returns true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Major
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-28 06:57 PST by Alexander Pavlov (apavlov)
Modified: 2011-12-28 12:22 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.64 KB, patch)
2011-12-28 07:10 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Allow both 'webkit...' and 'Webkit...' prefixes for compatibility (6.33 KB, patch)
2011-12-28 08:23 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Fixed CSS property detection (4.23 KB, patch)
2011-12-28 10:00 PST, Alexander Pavlov (apavlov)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-12-28 06:57:29 PST
V8CSSStyleDeclaration::namedPropertyQuery() always returns v8::Integer::New(v8::None)
Regression from http://trac.webkit.org/changeset/102578
Comment 1 Alexander Pavlov (apavlov) 2011-12-28 06:58:00 PST
Patch to follow.
Comment 2 Alexander Pavlov (apavlov) 2011-12-28 07:10:47 PST
Created attachment 120667 [details]
Patch
Comment 3 Alexander Pavlov (apavlov) 2011-12-28 07:27:36 PST
Upstreaming http://code.google.com/p/chromium/issues/detail?id=107941
Comment 4 Alexander Pavlov (apavlov) 2011-12-28 08:23:12 PST
Created attachment 120672 [details]
[PATCH] Allow both 'webkit...' and 'Webkit...' prefixes for compatibility
Comment 5 Alexander Pavlov (apavlov) 2011-12-28 08:33:27 PST
Anton, can you have a look at the change please?
Comment 6 WebKit Review Bot 2011-12-28 09:14:29 PST
Comment on attachment 120672 [details]
[PATCH] Allow both 'webkit...' and 'Webkit...' prefixes for compatibility

Attachment 120672 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11036597

New failing tests:
fast/dynamic/005.html
Comment 7 Alexander Pavlov (apavlov) 2011-12-28 10:00:37 PST
Created attachment 120684 [details]
[PATCH] Fixed CSS property detection
Comment 8 Adam Barth 2011-12-28 10:04:55 PST
Comment on attachment 120684 [details]
[PATCH] Fixed CSS property detection

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

> Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:185
> +    CSSPropertyInfo* propertyInfo = cssPropertyInfo(v8Name);
> +    if (propertyInfo)

We could combine these lines.
Comment 9 Alexander Pavlov (apavlov) 2011-12-28 10:05:56 PST
(In reply to comment #8)
> (From update of attachment 120684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120684&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:185
> > +    CSSPropertyInfo* propertyInfo = cssPropertyInfo(v8Name);
> > +    if (propertyInfo)
> 
> We could combine these lines.

Thanks Adam, will fix before landing!
Comment 10 Alexander Pavlov (apavlov) 2011-12-28 10:08:58 PST
Committed r103769: <http://trac.webkit.org/changeset/103769>
Comment 11 anton muhin 2011-12-28 10:57:19 PST
Comment on attachment 120684 [details]
[PATCH] Fixed CSS property detection

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

> Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:186
> +        return v8::Integer::New(v8::None);

just a question: is v8::None the best option? That's most probably is, but just double checking if those properties are enumerable and deletable?
Comment 12 Alexander Pavlov (apavlov) 2011-12-28 12:22:57 PST
Comment on attachment 120684 [details]
[PATCH] Fixed CSS property detection

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

>> Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:186
>> +        return v8::Integer::New(v8::None);
> 
> just a question: is v8::None the best option? That's most probably is, but just double checking if those properties are enumerable and deletable?

That seems to be what we used to return earlier, before the fix.