Bug 112108

Created attachment 193043 [details] WIP

Comment on attachment 193043 [details] WIP Attachment 193043 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17149572

Comment on attachment 193043 [details] WIP Attachment 193043 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17160244

Comment on attachment 193043 [details] WIP Attachment 193043 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17210100

Comment on attachment 193043 [details] WIP Attachment 193043 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17172091

Created attachment 193081 [details] Testing GTK build

Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

Comment on attachment 193081 [details] Testing GTK build Attachment 193081 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17055538

Comment on attachment 193081 [details] Testing GTK build Attachment 193081 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17182076

Comment on attachment 193081 [details] Testing GTK build Attachment 193081 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17161555

Created attachment 193108 [details] Target only JavaScript interfaces

Comment on attachment 193108 [details] Target only JavaScript interfaces Attachment 193108 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17041555

Comment on attachment 193108 [details] Target only JavaScript interfaces Attachment 193108 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17080513

Created attachment 193113 [details] Fixed incorrect XCode path

Comment on attachment 193113 [details] Fixed incorrect XCode path Attachment 193113 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17187176

Created attachment 193217 [details] EFL GTK patch

Created attachment 193223 [details] Rebased onto ToT

(In reply to comment #17) > Created an attachment (id=193223) [details] > Rebased onto ToT This patch appears to be building properly on the bots (pending Mac and Linux though they were passing before already). I submit this patch up for preliminary code review.

Comment on attachment 193223 [details] Rebased onto ToT View in context: https://bugs.webkit.org/attachment.cgi?id=193223&action=review Large patch! :) It looks like a lot of it is just tedious project adds and boilerplate, but there are important bits that we probably don't want to lose sight of. The bindings code will need to be reviewed by bindings experts (sam, ggaren for JSC, haraken, abarth for V8), the CSSOM by the style experts (anttik, kling) > Source/WebCore/ChangeLog:8 > + WIP. Fixing build issues with EFL and GTK. Rebased patch onto ToT master. Still has unresolved matter of pointer to PropertySetCSSStyleDeclaration object shared across JS thread not ref counted. If it's a WIP, probably don't need to mark it for review yet :) > Source/WebCore/bindings/js/JSCSSVariablesDeclarationCustom.h:31 > +#include "JSCSSVariablesDeclaration.h" This is weird. Why do we need this file? > Source/WebCore/css/StylePropertySet.cpp:752 > + if (testProperty.id() == CSSPropertyVariable && static_cast<const CSSVariableValue*>(testProperty.value())->name() == name) { This looks like a toCSSVariableValue helper. > Source/WebCore/css/StylePropertySet.cpp:768 > +void StylePropertySet::getVariableNames(Vector<AtomicString>& names) const here, get is not superfluous, you are actually not returning a value. > Source/WebCore/css/StylePropertySet.cpp:777 > +const String& StylePropertySet::getVariableValue(const AtomicString& name) const here get is superfluous -> variableValue

(In reply to comment #19) > (From update of attachment 193223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193223&action=review > > Large patch! :) > > It looks like a lot of it is just tedious project adds and boilerplate, but there are important bits that we probably don't want to lose sight of. > > The bindings code will need to be reviewed by bindings experts (sam, ggaren for JSC, haraken, abarth for V8), the CSSOM by the style experts (anttik, kling) Thanks for the advice, I will go to these people for further review. > > Source/WebCore/bindings/js/JSCSSVariablesDeclarationCustom.h:31 > > +#include "JSCSSVariablesDeclaration.h" > > This is weird. Why do we need this file? It does seem unnecessary but the EFL port wouldn't compile without it. Its binding code generator most likely includes it without checking for its existence first: [ 45%] /home/eflews/git/webkit/WebKitBuild/Release/DerivedSources/WebCore/JSCSSVariablesDeclaration.cpp:28:45: fatal error: JSCSSVariablesDeclarationCustom.h: No such file or directory compilation terminated. I'll add a comment stating this. > > > Source/WebCore/css/StylePropertySet.cpp:752 > > + if (testProperty.id() == CSSPropertyVariable && static_cast<const CSSVariableValue*>(testProperty.value())->name() == name) { > > This looks like a toCSSVariableValue helper. I see what you mean but I'm not sure where this helper function should go. This doesn't appear to be an idiom that CSSValue uses. > > Source/WebCore/css/StylePropertySet.cpp:768 > > +void StylePropertySet::getVariableNames(Vector<AtomicString>& names) const > > here, get is not superfluous, you are actually not returning a value. > > > Source/WebCore/css/StylePropertySet.cpp:777 > > +const String& StylePropertySet::getVariableValue(const AtomicString& name) const > > here get is superfluous -> variableValue Thanks for the style tip, updated function name.

Created attachment 194178 [details] Patch

(In reply to comment #21) > Created an attachment (id=194178) [details] > Patch - Made changes based on Dimitri's review. - Split CSSOM test into smaller CRUD tests. - Fixed bug where non-inherited styles were not updating when a variable value cascade should have updated them.

Comment on attachment 194178 [details] Patch Attachment 194178 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17235329 New failing tests: platform/chromium/fast/forms/calendar-picker/calendar-picker-appearance-month-popup.html

Created attachment 194226 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

(In reply to comment #24) > Created an attachment (id=194226) [details] > Archive of layout-test-results from gce-cr-linux-02 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04 These appear to be flakey/failing tests: http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/46263 Reuploading patch while the tree is green.

Created attachment 194369 [details] Patch

Created attachment 195498 [details] Patch

(In reply to comment #27) > Created an attachment (id=195498) [details] > Patch - Rebaselined patch. - Implemented CSSOM API for computed styles.

Comment on attachment 195498 [details] Patch Attachment 195498 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17339232

Comment on attachment 195498 [details] Patch Attachment 195498 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17320281

Comment on attachment 195498 [details] Patch Attachment 195498 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17292357

Comment on attachment 195498 [details] Patch We should do this without introducing custom bindings.

(In reply to comment #32) > (From update of attachment 195498 [details]) > We should do this without introducing custom bindings. Thanks for the review. AFAIK avoiding custom bindings here would involve implementing getter, setter, deleter and enumerator interface attributes for the IDL binding code generators. Possibly even a special getter that returns undefined for empty strings. These would call the getPropertyValue, setProperty, removeProperty and getPropertyNames methods respectively. Please correct me if I'm incorrect in this approach.

Those are good things to add to the code generators. We'd eventually like to implement all of WebIDL.

Created attachment 196074 [details] Repatch for JSC

Comment on attachment 196074 [details] Repatch for JSC Attachment 196074 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17360153 New failing tests: media/video-controls-fullscreen-volume.html

Created attachment 196214 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2

Migrated to Blink: https://code.google.com/p/chromium/issues/detail?id=231791