The pattern: ExceptionCode ec = 0; methodThatGeneratesException(ec); ASSERT(!ec); is more clearly and succinctly written as: methodThatGeneratesException(ASSERT_NO_EXCEPTION);
(In reply to comment #0) > The pattern: > > ExceptionCode ec = 0; > methodThatGeneratesException(ec); > ASSERT(!ec); > > is more clearly and succinctly written as: > > methodThatGeneratesException(ASSERT_NO_EXCEPTION); Note that this only applies to call sites that do nothing else with 'ec'. ExceptionCode ec = 0; methodThatGeneratesException(ec); ASSERT(!ec); if (ec) return; should be left alone.
Created attachment 186839 [details] Patch
Comment on attachment 186839 [details] Patch Attachment 186839 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16395204
Comment on attachment 186839 [details] Patch Attachment 186839 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16400112
Comment on attachment 186839 [details] Patch Ugh. Missed a spot. :)
Created attachment 186845 [details] Less broken.
Comment on attachment 186845 [details] Less broken. Attachment 186845 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16395232 New failing tests: dom/xhtml/level2/core/setAttributeNS10.xhtml dom/html/level2/core/setAttributeNS10.html fast/dom/Element/setAttributeNS-namespace-err.html
Comment on attachment 186845 [details] Less broken. Attachment 186845 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16394267 New failing tests: fast/dom/Element/setAttributeNS-namespace-err.html dom/html/level2/core/setAttributeNS10.html dom/xhtml/level2/core/setAttributeNS10.xhtml
Created attachment 186862 [details] Patch
Comment on attachment 186862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186862&action=review I like that this cuts down on the "ec" name, and code that knows that it’s an integer. It’s interesting how much of this is basic DOM tree and Range operations. Makes me think we should have internal functions for these instead of going through the “real DOM” functions. Likely an even better solution to this problem area for many call sites that could even possibly lead to some refactoring opportunities or a performance boost. At some point we should use grep to come up with stats about which functions are used most often with ASSERT_NO_EXCEPTION and IGNORE_EXCEPTION. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:99 > + const AtomicString& direction = directionToString(m_direction, ASSERT_NO_EXCEPTION); > return direction; No need for the local variable any more here. > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125 > + const AtomicString& mode = modeToString(m_mode, ASSERT_NO_EXCEPTION); > return mode; No need for the local variable any more here.
Comment on attachment 186862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186862&action=review >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:99 >> return direction; > > No need for the local variable any more here. And FYI the directionToString method is only ever called from here now; it was used when we supported both legacy numeric enum and new string values when called from script. Now that the numeric enum values are not supported from script we can drop the ExceptionCode argument from this method. But that can be done in a separate patch. >> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125 >> return mode; > > No need for the local variable any more here. And FYI, same as above. We can drop the ExceptionCode param from modeToStrng now.
Comment on attachment 186862 [details] Patch Attachment 186862 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16401229
(In reply to comment #10) > (From update of attachment 186862 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186862&action=review > > I like that this cuts down on the "ec" name, and code that knows that it’s an integer. Indeed, the less of that the better. > At some point we should use grep to come up with stats about which functions are used most often with ASSERT_NO_EXCEPTION and IGNORE_EXCEPTION. My plan is to post just such a list to webkit-dev once this set of patches seems more or less finished. I've broken jsbell@'s comments out into https://bugs.webkit.org/show_bug.cgi?id=109143; I'll take care of that once this lands.
Created attachment 187004 [details] Patch for landing
Comment on attachment 187004 [details] Patch for landing Clearing flags on attachment: 187004 Committed r142118: <http://trac.webkit.org/changeset/142118>
All reviewed patches have been landed. Closing bug.
Reverted r142118 for reason: Broke SVG! Oh noes! Committed r142126: <http://trac.webkit.org/changeset/142126>
Sample test run: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r142124%20(5307)/results.html>.
(In reply to comment #18) > Sample test run: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r142124%20(5307)/results.html>. Thanks ap. I can replicate these locally, though I'm not at all sure why they occur. I'll poke at it tomorrow morning, but I'll probably end up splitting the IDB and SVG portions out into separate patches.
(In reply to comment #19) > (In reply to comment #18) > > Sample test run: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r142124%20(5307)/results.html>. > > Thanks ap. I can replicate these locally, though I'm not at all sure why they occur. I'll poke at it tomorrow morning, but I'll probably end up splitting the IDB and SVG portions out into separate patches. A few possibilities I could see: - The calling object is an element in an array passed by reference. Maybe something odd happens - More likely, the mess of template code that exists in SVG animations may be interfering with the changes you made. - Or, the method to which you are passing the ASSERT_NO_EXCEPTIONS parameter is not designed to handle it.
Created attachment 187259 [details] Patch
Comment on attachment 187259 [details] Patch I've dropped IDB and SVG from this patch, and the tests pass locally. I'll take another stab at them in https://bugs.webkit.org/show_bug.cgi?id=109266 and https://bugs.webkit.org/show_bug.cgi?id=109267 respectively.
Comment on attachment 187259 [details] Patch Clearing flags on attachment: 187259 Committed r142247: <http://trac.webkit.org/changeset/142247>
*** Bug 108772 has been marked as a duplicate of this bug. ***