Last Comment Bug 441770 - Add 'sizes' getter/setter to link elements
: Add 'sizes' getter/setter to link elements
Status: ASSIGNED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: mozilla1.9.1
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on:
Blocks: 596650
  Show dependency treegraph
 
Reported: 2008-06-25 02:56 PDT by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2011-08-21 16:53 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.03 KB, patch)
2008-06-25 03:15 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
jst: review-
jst: superreview-
Details | Diff | Review
Patch v2 (8.00 KB, patch)
2008-10-16 20:12 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Review
Patch v2.1 (10.19 KB, patch)
2008-10-16 20:18 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
bugs: review-
Details | Diff | Review
new patch (1.46 KB, patch)
2010-10-02 00:38 PDT, [:fabrice] Fabrice Desré
bugs: review-
Details | Diff | Review
addression comments (2.08 KB, patch)
2010-10-04 05:54 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
corrected patch (3.09 KB, patch)
2010-10-04 06:10 PDT, [:fabrice] Fabrice Desré
bugs: review-
Details | Diff | Review

Description Ryan Flint [:rflint] (ping via IRC for reviews) 2008-06-25 02:56:30 PDT
 
Comment 1 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-06-25 03:15:16 PDT
Created attachment 326656 [details] [diff] [review]
Patch

Noticed this was a frozen interface only after writing the patch. Guess this is a Moz2 kind of thing then?
Comment 2 Olli Pettay [:smaug] 2008-06-25 03:47:53 PDT
New things should go to nsIDOMNSHTMLLinkElement interface.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-25 13:11:52 PDT
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.
Comment 4 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-10-16 20:12:06 PDT
Created attachment 343495 [details] [diff] [review]
Patch v2
Comment 5 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-10-16 20:18:26 PDT
Created attachment 343496 [details] [diff] [review]
Patch v2.1

Let's try that again with the new interface actually included in the diff this time...
Comment 6 Olli Pettay [:smaug] 2008-10-17 02:23:17 PDT
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 7 Olli Pettay [:smaug] 2009-01-19 10:12:24 PST
Comment on attachment 343496 [details] [diff] [review]
Patch v2.1

New patch coming?
Comment 8 [:fabrice] Fabrice Desré 2010-10-02 00:38:31 PDT
Created attachment 480372 [details] [diff] [review]
new patch

New patch, taking advantage of the now non-frozen interfaces to simply add the sizes attribute to nsIDOMHTMLLinkElement
Comment 9 Olli Pettay [:smaug] 2010-10-04 02:15:37 PDT
Comment on attachment 480372 [details] [diff] [review]
new patch

You should update the iid of the interface.
And where is the test?
Comment 10 [:fabrice] Fabrice Desré 2010-10-04 05:54:01 PDT
Created attachment 480602 [details] [diff] [review]
addression comments

Addressing issues from comment 9
Comment 11 [:fabrice] Fabrice Desré 2010-10-04 06:10:49 PDT
Created attachment 480604 [details] [diff] [review]
corrected patch

Sorry, I had not hg added the test case.
Comment 12 Mounir Lamouri (:mounir) 2010-10-06 06:41:17 PDT
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 13 Olli Pettay [:smaug] 2010-10-11 13:37:38 PDT
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)?

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