Summary: | WML Card Title does not get displayed on GtkLauncher | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | wajahat <wajahatmeister> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | alp, zimmermann | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
wajahat
2009-06-16 23:48:01 PDT
Created attachment 31402 [details]
Patch to add title attribute to card element
Adding title attribute to WML <card> element with necessary layout test
Created attachment 31404 [details]
Patch to add title attribute to card element
Resubmitting with a review flag
Adding title attribute to WML <card> element with necessary layout test
Comment on attachment 31404 [details]
Patch to add title attribute to card element
Hi Wajat,
thanks for the patch. Good that you spotted it, though I'd suggest a better implementation of that.
WMLCardElement already provides a virtual-function "String title() const" through the Element parent class.
So all you'd need to do is implement WMLCardElement::title() and have it return "parseValueSub...(getAttribute(titleAttr...". See HTMLElement::title() for comparision.
This way we save memory. Glad if you could upload a new patch.
Niko
Created attachment 31540 [details]
Updated Patch
Hi Niko,
thanks for the comments. Updated as per your suggestions. Performing const_cast to remove the const property. I hope this approach is good.
let me know your comments.
Comment on attachment 31540 [details]
Updated Patch
I think this is meant to be r? rather than r+. Setting it for review.
Comment on attachment 31540 [details]
Updated Patch
Looks good except the const_cast. Just make the parseValueSubstitutingVariableReferences & parseValueForbidding... functions const. They're not modifying any member variables and they are non-const by mistake. Can you upload a new patch?
As you don't have commit access I'm going to clear the review flag until a new patch has been uploaded. Going to commit that patch then.
Created attachment 31633 [details] updated patch to fix const_cast modified as per Niko's comment#6 by making parseValueSubstitutingVariableReferences & parseValueForbidding... as const intead of performing const_cast. Pl. give a final review and get this in. thanks Wajahat Comment on attachment 31633 [details]
updated patch to fix const_cast
How do we test this? Your ChangeLog mentions a layout test, but it seems to be missing from your patch.
Otherwise this looks fine... but I don't understand how this method is ever used or tested?
Created attachment 31635 [details]
Adding missed manual test
resubmitting patch by
Adding manual/layout test, for testing this patch
Comment on attachment 31635 [details]
Adding missed manual test
Looks good! r=me.
(In reply to comment #10) > (From update of attachment 31635 [details] [review]) > Looks good! r=me. > thanks Niko can you get this in. regards Landed in r45081. Closing bug. |