These two methods can be safely replaced by item() to reduce the interface surface.
Created attachment 149401 [details] Cleanup; also improve some insanely inefficient code
Comment on attachment 149401 [details] Cleanup; also improve some insanely inefficient code View in context: https://bugs.webkit.org/attachment.cgi?id=149401&action=review > Source/WebCore/html/HTMLCollection.cpp:-202 > - invalidateCacheIfNeeded(); Note I'm fixing 5-space indentation here.
Comment on attachment 149401 [details] Cleanup; also improve some insanely inefficient code View in context: https://bugs.webkit.org/attachment.cgi?id=149401&action=review > Source/WebCore/html/HTMLCollection.cpp:-223 > - return item(0); Rather than deleting this entirely, why not just make it inline? firstItem() reads better to me than item(0) (compare to Vector::at(0) vs Vector::first())
Comment on attachment 149401 [details] Cleanup; also improve some insanely inefficient code Attachment 149401 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13101240
(In reply to comment #3) > (From update of attachment 149401 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149401&action=review > > > Source/WebCore/html/HTMLCollection.cpp:-223 > > - return item(0); > > Rather than deleting this entirely, why not just make it inline? firstItem() reads better to me than item(0) (compare to Vector::at(0) vs Vector::first()) I think it's better to have fewer methods.
Created attachment 149446 [details] Fixed Chromium build
Comment on attachment 149446 [details] Fixed Chromium build Attachment 149446 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13099278
Created attachment 149449 [details] Fixed more Chromium build failures
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.
Comment on attachment 149449 [details] Fixed more Chromium build failures Attachment 149449 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13090379
Created attachment 149450 [details] Another Chromium build fix
Comment on attachment 149450 [details] Another Chromium build fix Attachment 149450 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13101292
Created attachment 149455 [details] It must compile this time
Comment on attachment 149455 [details] It must compile this time View in context: https://bugs.webkit.org/attachment.cgi?id=149455&action=review r=me assuming it builds etc. > Source/WebCore/html/HTMLCollection.h:56 > + bool hasAnyItem() const I'd call this hasItems().
Comment on attachment 149455 [details] It must compile this time Attachment 149455 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13101300
Committed r121232: <http://trac.webkit.org/changeset/121232>
Comment on attachment 149455 [details] It must compile this time View in context: https://bugs.webkit.org/attachment.cgi?id=149455&action=review > Source/WebCore/html/HTMLCollection.h:65 > + bool hasExactlyOneItem() const > + { > + invalidateCacheIfNeeded(); > + return (m_cache.hasLength && m_cache.length == 1) || (m_cache.current && !itemAfter(m_cache.current)) || (item(0) && !item(1)); > + } This looks like it could return true for a list with two items if hasLength is false, m_cache.current is 2, and itemAfter(2) is 0. Is there some reason that’s not a problem?
Comment on attachment 149455 [details] It must compile this time View in context: https://bugs.webkit.org/attachment.cgi?id=149455&action=review >> Source/WebCore/html/HTMLCollection.h:65 >> + } > > This looks like it could return true for a list with two items if hasLength is false, m_cache.current is 2, and itemAfter(2) is 0. Is there some reason that’s not a problem? Sorry, let me say that again: This looks like it could return true for a collection with two items if m_cache.hasLength is false and m_cache.current is 1. If so, there is a bug here. If not, why can’t this happen?
Comment on attachment 149455 [details] It must compile this time View in context: https://bugs.webkit.org/attachment.cgi?id=149455&action=review >>> Source/WebCore/html/HTMLCollection.h:65 >>> + } >> >> This looks like it could return true for a list with two items if hasLength is false, m_cache.current is 2, and itemAfter(2) is 0. Is there some reason that’s not a problem? > > Sorry, let me say that again: > > This looks like it could return true for a collection with two items if m_cache.hasLength is false and m_cache.current is 1. If so, there is a bug here. If not, why can’t this happen? Oops, you're right. I should have done !item(m_cache. position + 1) instead of !itemAfter(m_cache.current)