Bug 26474

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 Flags
Patch to add title attribute to card element
none
Patch to add title attribute to card element
zimmermann: review-
Updated Patch
none
updated patch to fix const_cast
eric: review-
Adding missed manual test zimmermann: review+

Description wajahat 2009-06-16 23:48:01 PDT
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());
Comment 1 wajahat 2009-06-16 23:58:16 PDT
Created attachment 31402 [details]
Patch to add title attribute to card element

Adding title attribute to WML <card> element with necessary layout test
Comment 2 wajahat 2009-06-17 00:39:45 PDT
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 3 Nikolas Zimmermann 2009-06-18 10:21:50 PDT
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
Comment 4 wajahat 2009-06-19 03:24:52 PDT
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 5 Jan Alonzo 2009-06-20 07:32:39 PDT
Comment on attachment 31540 [details]
Updated Patch 

I think this is meant to be r? rather than r+. Setting it for review.
Comment 6 Nikolas Zimmermann 2009-06-20 08:16:18 PDT
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.
Comment 7 wajahat 2009-06-21 22:31:00 PDT
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 8 Eric Seidel (no email) 2009-06-22 00:03:04 PDT
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?
Comment 9 wajahat 2009-06-22 00:16:49 PDT
Created attachment 31635 [details]
Adding missed manual test

resubmitting patch by
Adding manual/layout test, for testing this patch
Comment 10 Nikolas Zimmermann 2009-06-22 01:26:47 PDT
Comment on attachment 31635 [details]
Adding missed manual test

Looks good! r=me.
Comment 11 wajahat 2009-06-22 07:16:48 PDT
(In reply to comment #10)
> (From update of attachment 31635 [details] [review])
> Looks good! r=me.
> 

thanks Niko can you get this in.

regards
Comment 12 Nikolas Zimmermann 2009-06-24 07:31:06 PDT
Landed in r45081. Closing bug.