Bug 89923

Summary:

Get rid of firstItem and nextItem from HTMLCollection

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, adamk, arv, darin, dglazkov, eric, fishd, haraken, jamesr, japhet, jochen, kling, koivisto, sam, tkent+wkapi, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

89919, 90133

Attachments:

Description	Flags
Cleanup; also improve some insanely inefficient code	none
Fixed Chromium build	none
Fixed more Chromium build failures	none
Another Chromium build fix	none
It must compile this time	kling: review+, webkit.review.bot: commit-queue-

Ryosuke Niwa

Reported 2012-06-25 17:04:22 PDT

These two methods can be safely replaced by item() to reduce the interface surface.

Attachments
Cleanup; also improve some insanely inefficient code (11.78 KB, patch) 2012-06-25 17:16 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixed Chromium build (11.87 KB, patch) 2012-06-25 21:28 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixed more Chromium build failures (14.45 KB, patch) 2012-06-25 22:00 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Another Chromium build fix (14.45 KB, patch) 2012-06-25 22:14 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
It must compile this time (14.46 KB, patch) 2012-06-25 22:29 PDT, Ryosuke Niwa	kling: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2012-06-25 17:16:10 PDT

Created attachment 149401 [details] Cleanup; also improve some insanely inefficient code

Ryosuke Niwa

Comment 2 2012-06-25 17:17:25 PDT

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.

Adam Klein

Comment 3 2012-06-25 17:25:12 PDT

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())

WebKit Review Bot

Comment 4 2012-06-25 18:19:48 PDT

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

Ryosuke Niwa

Comment 5 2012-06-25 20:02:28 PDT

(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.

Ryosuke Niwa

Comment 6 2012-06-25 21:28:12 PDT

Created attachment 149446 [details] Fixed Chromium build

WebKit Review Bot

Comment 7 2012-06-25 21:36:57 PDT

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

Ryosuke Niwa

Comment 8 2012-06-25 22:00:58 PDT

Created attachment 149449 [details] Fixed more Chromium build failures

WebKit Review Bot

Comment 9 2012-06-25 22:06:27 PDT

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.

WebKit Review Bot

Comment 10 2012-06-25 22:13:09 PDT

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

Ryosuke Niwa

Comment 11 2012-06-25 22:14:08 PDT

Created attachment 149450 [details] Another Chromium build fix

WebKit Review Bot

Comment 12 2012-06-25 22:24:13 PDT

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

Ryosuke Niwa

Comment 13 2012-06-25 22:29:51 PDT

Created attachment 149455 [details] It must compile this time

Andreas Kling

Comment 14 2012-06-25 22:32:58 PDT

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().

WebKit Review Bot

Comment 15 2012-06-25 22:55:30 PDT

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

Ryosuke Niwa

Comment 16 2012-06-25 23:30:03 PDT

Committed r121232: <http://trac.webkit.org/changeset/121232>

Darin Adler

Comment 17 2012-06-26 11:55:47 PDT

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?

Darin Adler

Comment 18 2012-06-26 11:57:21 PDT

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?

Ryosuke Niwa

Comment 19 2012-06-26 12:06:06 PDT

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)

Note You need to log in before you can comment on or make changes to this bug.