Open Bug 441770 Opened 14 years ago Updated 3 years ago

Add 'sizes' getter/setter to link elements

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

ASSIGNED
mozilla1.9.1

People

(Reporter: rflint, Assigned: fabrice)

References

()

Details

Attachments

(2 files, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Noticed this was a frozen interface only after writing the patch. Guess this is a Moz2 kind of thing then?
Attachment #326656 - Flags: superreview?(jst)
Attachment #326656 - Flags: review?(jst)
New things should go to nsIDOMNSHTMLLinkElement interface.
Comment on attachment 326656 [details] [diff] [review]
Patch

Yeah, either wait for Moz2 (and re-request reviews then), or create a new nsIDOMNSHTMLLinkElement interface that extens nsIDOMHTMLLinkElement and adds this property.
Attachment #326656 - Flags: superreview?(jst)
Attachment #326656 - Flags: superreview-
Attachment #326656 - Flags: review?(jst)
Attachment #326656 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #326656 - Attachment is obsolete: true
Attachment #343495 - Flags: review?(Olli.Pettay)
Attached patch Patch v2.1Splinter Review
Let's try that again with the new interface actually included in the diff this time...
Attachment #343495 - Attachment is obsolete: true
Attachment #343496 - Flags: review?(Olli.Pettay)
Attachment #343495 - Flags: review?(Olli.Pettay)
Comment on attachment 343496 [details] [diff] [review]
Patch v2.1


>+[scriptable, uuid(ea4e4325-9cc4-42ac-8126-0a8c035c165a)]
>+interface nsIDOMNSHTMLLinkElement : nsISupports
>+{
>+  attribute DOMString    sizes;
>+};
Make nsIDOMNSHTMLLinkElement inherit nsIDOMHTMLLinkElement and then
nsHTMLLinkElement should inherit only nsIDOMNSHTMLLinkElement.
That way you don't increase the size of the nsHTMLLinkElement
(doesn't increase vtable).
Comment on attachment 343496 [details] [diff] [review]
Patch v2.1

New patch coming?
Attachment #343496 - Flags: review?(Olli.Pettay) → review-
Target Milestone: mozilla1.9.1 → ---
Blocks: 596650
Target Milestone: --- → mozilla1.9.1
Attached patch new patch (obsolete) — Splinter Review
New patch, taking advantage of the now non-frozen interfaces to simply add the sizes attribute to nsIDOMHTMLLinkElement
Attachment #480372 - Flags: review?
Attachment #480372 - Flags: review? → review?(Olli.Pettay)
Comment on attachment 480372 [details] [diff] [review]
new patch

You should update the iid of the interface.
And where is the test?
Attachment #480372 - Flags: review?(Olli.Pettay) → review-
Attached patch addression comments (obsolete) — Splinter Review
Addressing issues from comment 9
Assignee: rflint → fabrice
Attachment #480372 - Attachment is obsolete: true
Attachment #480602 - Flags: review?(Olli.Pettay)
Attached patch corrected patchSplinter Review
Sorry, I had not hg added the test case.
Attachment #480602 - Attachment is obsolete: true
Attachment #480604 - Flags: review?(Olli.Pettay)
Attachment #480602 - Flags: review?(Olli.Pettay)
The IDL attribute shouldn't be a DOMString but a DOMSettableTokenList.

By the way, if you just need .sizes done this way (a simple reflection of the content attribute), you can use .getAttribute('sizes') for the moment and change that when .sizes will be implemented.
Comment on attachment 480604 [details] [diff] [review]
corrected patch

Mounir is very true. 
.sizes isn't a DOMString, but DOMSettableTokenList.
Unfortunately the current HTML5 draft has  [PutForwards=value] - 
I think we should get rid of that.
Want to sent question to whatwg why Hixie has used
PutForwards (which is generally an indicator of an ugly API, IMHO)?
Attachment #480604 - Flags: review?(Olli.Pettay) → review-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.