Bug 26474 – WML Card Title does not get displayed on GtkLauncher

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[wajahat]

WML <card> title attribute is not bieng implemented in WMLCardElement as a result it fails to show card title on the title bar of GtkLauncher. 
This implementation is returning empty string
    // Update the document title
    doc->setTitle(activeCard->title());

[wajahat]

Created attachment 31402 [details]
Patch to add title attribute to card element

Adding title attribute to WML <card> element with necessary layout test

[wajahat]

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

[Nikolas Zimmermann]

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

[wajahat]

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.

[Jan Alonzo]

Comment on attachment 31540 [details]
Updated Patch 

I think this is meant to be r? rather than r+. Setting it for review.

[Nikolas Zimmermann]

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.

[wajahat]

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

[Eric Seidel (no email)]

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?

[wajahat]

Created attachment 31635 [details]
Adding missed manual test

resubmitting patch by
Adding manual/layout test, for testing this patch

[Nikolas Zimmermann]

Comment on attachment 31635 [details]
Adding missed manual test

Looks good! r=me.

[wajahat]

(In reply to comment #10)
> (From update of attachment 31635 [details] [review])
> Looks good! r=me.
> 

thanks Niko can you get this in.

regards

[Nikolas Zimmermann]

Landed in r45081. Closing bug.