Bug 118561

Summary: NamedFlowCollection getters should follow the same pattern as HTMLCollection
Product: WebKit Reporter: Radu Stavila <stavila>
Component: CSSAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, arv, cdumez, commit-queue, esprehn+autocc, kangil.han, rniwa, WebkitBugTracker
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
achicu: review-, achicu: commit-queue-
Patch which also removes generator code none

Description Radu Stavila 2013-07-11 05:39:48 PDT
When there's no named flow in the collection:

collection[42] === undefined
collection['fooBar'] === undefined
collection.item(42) === null
collection.namedItem('fooBar') === null


https://www.w3.org/Bugs/Public/show_bug.cgi?id=22617
Comment 1 Radu Stavila 2013-07-11 06:22:15 PDT
Created attachment 206456 [details]
Patch
Comment 2 Chris Dumez 2013-07-11 06:24:55 PDT
Maybe we should wait until we get resolution on the W3C bug, just in case?
Comment 3 Radu Stavila 2013-07-11 06:27:12 PDT
The discussion was on the www-style list, Alan already decided to roll back the modification. I already emailed him to post a comment on the w3 bug so it's clear to everybody, he will probably do it when he wakes up.
Comment 4 Chris Dumez 2013-07-11 06:34:10 PDT
Comment on attachment 206456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206456&action=review

> LayoutTests/fast/regions/webkit-named-flow-collection-empty-getters.html:4
> +

nit: extra line

> LayoutTests/fast/regions/webkit-named-flow-collection-empty-getters.html:7
> +            if (window.testRunner)

nit: We probably don't need these 2 lines, I believe js-test-pre.js takes care of it.

> LayoutTests/fast/regions/webkit-named-flow-collection-empty-getters.html:18
> +    <script src="../../fast/js/resources/js-test-post.js"></script> 

nit: weird identation

> Source/WebCore/dom/DOMNamedFlowCollection.idl:-39
> -    [TreatReturnedNullObjectAs=Undefined] getter WebKitNamedFlow namedItem(DOMString name);

We should drop support for this extended attribute as it is no longer used and was introduced just for this special case.
Comment 5 Alexandru Chiculita 2013-07-11 06:37:34 PDT
Comment on attachment 206456 [details]
Patch

r=me
Comment 6 Alexandru Chiculita 2013-07-11 06:40:20 PDT
(In reply to comment #4)
> (From update of attachment 206456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206456&action=review
> 
Didn't see your review. I've set the cq-.

Do you want to drop that in this patch? I would put it in a different one, so that we can revert it when needed in the future.
Comment 7 Chris Dumez 2013-07-11 06:43:29 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 206456 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206456&action=review
> > 
> Didn't see your review. I've set the cq-.
> 
> Do you want to drop that in this patch? I would put it in a different one, so that we can revert it when needed in the future.

well, this is basically a revert of <http://trac.webkit.org/changeset/152466> so I think it makes sense. Alternatively, we can simply revert the previous patch and merely add the test.
Comment 8 Alexandru Chiculita 2013-07-11 06:46:54 PDT
Comment on attachment 206456 [details]
Patch

Let's remove the code in the generator too.
Comment 9 Radu Stavila 2013-07-11 07:09:51 PDT
Created attachment 206458 [details]
Patch which also removes generator code
Comment 10 Chris Dumez 2013-07-11 07:13:07 PDT
Comment on attachment 206458 [details]
Patch which also removes generator code

Thanks. Looks good. r=me.
Comment 11 WebKit Commit Bot 2013-07-11 07:55:33 PDT
Comment on attachment 206458 [details]
Patch which also removes generator code

Clearing flags on attachment: 206458

Committed r152569: <http://trac.webkit.org/changeset/152569>
Comment 12 WebKit Commit Bot 2013-07-11 07:55:37 PDT
All reviewed patches have been landed.  Closing bug.