Bug 102726

Created attachment 176016 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Attachment 176016 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Makes sense to me.

Comment on attachment 176016 [details] Patch Attachment 176016 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14986770 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html fast/text/atsui-small-caps-punctuation-size.html

I'm a bit worried about the change to the Chromium WebKit API. Are changes like that allowed?

(In reply to comment #6) > I'm a bit worried about the change to the Chromium WebKit API. Are changes like that allowed? Yes, generally those are used when code on the Chromium side needs to pass an EC through to WebKit, and it's not worth add a mapping function. They're listed in Source/WebKit/chromium/src/AssertMatchingEnums.cpp already which is how we ensure everything stays in sync.

(In reply to comment #7) > (In reply to comment #6) > > I'm a bit worried about the change to the Chromium WebKit API. Are changes like that allowed? > > Yes, generally those are used when code on the Chromium side needs to pass an EC through to WebKit, and it's not worth add a mapping function. > > They're listed in Source/WebKit/chromium/src/AssertMatchingEnums.cpp already which is how we ensure everything stays in sync. ... although it turns out Chromium has a dependency on those values remaining the same, which will require fixing. (FWIW, the Chromium IDB code uses the WebXXXX enums directly, as it should.)

Created attachment 177748 [details] Patch

The chromium side changes are done so this is now ready for review.

Comment on attachment 177748 [details] Patch IDB changes lgtm.

Comment on attachment 177748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177748&action=review > Source/WebCore/dom/DOMCoreException.cpp:39 > + { "IndexSizeError", "Index or size was negative, or greater than the allowed value.", 1 }, It will now be easier for this table to get out of sync with the enum. Do we have any tests that verify the codes/names where possible?

Attachment 177748 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/dom/ExceptionCode.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:40: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:41: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:42: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:43: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:44: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:47: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:48: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:49: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:50: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:51: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:54: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:57: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:60: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:61: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:62: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:63: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:64: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:65: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/ExceptionCode.h:66: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 21 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #12) > It will now be easier for this table to get out of sync with the enum. Do we have any tests that verify the codes/names where possible? We have a lot of tests covering this already. I agree that it would be nice if we could ensure this without actually having tests that hits the code path that sets the ExceptionCode. Do you have any ideas?

(In reply to comment #14) > (In reply to comment #12) > > It will now be easier for this table to get out of sync with the enum. Do we have any tests that verify the codes/names where possible? > > We have a lot of tests covering this already. > > I agree that it would be nice if we could ensure this without actually having tests that hits the code path that sets the ExceptionCode. Do you have any ideas? I only know offhand how to trigger 11/25 of them from script. We could do a webkit_unit_test. Chromium only, but would catch glitches via ews.

Comment on attachment 177748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177748&action=review > Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:53 > + { "NotFoundError", "An operation failed because the requested database object could not be found.", DOMCoreException::getLegacyErrorCode(NOT_FOUND_ERR) }, I don’t think this compiles on the Mac platform, because it looks like the call to DOMCoreException::getLegacyErrorCode is going to be a library initialization-time constructor, which we don’t allow in the WebCore framework on Mac. We’re probably OK only because the feature is currently off for Mac. Please change the idiom so that feature, library load-time constructors, is not required.

Comment on attachment 177748 [details] Patch I'll have to come back to this in a few weeks since I have more important work to do.

This has happened at some point.