Bug 146450

Summary: [WinCairo] Compile error, WebEditorClient::didApplyStyle() should not have any parameters.
Product: WebKit Reporter: peavo
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, darin, ossy, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description peavo 2015-06-30 02:18:28 PDT 
The abstract method EditClient::didApplyStyle() does not have any parameters, WebEditorClient::didApplyStyle() should have the same signature.
Comment 1 peavo 2015-06-30 02:20:58 PDT 
Created attachment 255815 [details]
Patch
Comment 2 Csaba Osztrogonác 2015-06-30 03:13:12 PDT 
Comment on attachment 255815 [details]
Patch

LGTM, r=me. Could you add an override keyword before 
landing to avoid possible bugs in the future here?

( Just note: This code was introduced in bug146379 - https://trac.webkit.org/changeset/186086 )
Comment 3 peavo 2015-06-30 03:28:04 PDT 
(In reply to comment #2)
> Comment on attachment 255815 [details]
> Patch
> 
> LGTM, r=me. Could you add an override keyword before 
> landing to avoid possible bugs in the future here?
> 
> ( Just note: This code was introduced in bug146379 -
> https://trac.webkit.org/changeset/186086 )

Thanks for reviewing :)

Yes, I can add the override keyword, but I seem to remember there was an agreement not to add the override keyword when overriding pure virtual methods?
Comment 4 Csaba Osztrogonác 2015-06-30 03:38:18 PDT 
Comment on attachment 255815 [details]
Patch

I missed this agreement. But I agree, adding override keyword for 
overriding pure virtual function isn't necessary. Let's land it as is.
Comment 5 peavo 2015-06-30 03:47:00 PDT 
(In reply to comment #4)
> Comment on attachment 255815 [details]
> Patch
> 
> I missed this agreement. But I agree, adding override keyword for 
> overriding pure virtual function isn't necessary. Let's land it as is.

Ok, thanks!
Comment 6 WebKit Commit Bot 2015-06-30 04:26:59 PDT 
Comment on attachment 255815 [details]
Patch

Clearing flags on attachment: 255815

Committed r186114: <http://trac.webkit.org/changeset/186114>
Comment 7 WebKit Commit Bot 2015-06-30 04:27:05 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2015-06-30 10:13:22 PDT 
(In reply to comment #3)
> I seem to remember there was an
> agreement not to add the override keyword when overriding pure virtual
> methods?

This is the first I have heard of this. I think we should definitely use override in cases like that and I’d like to know more about why we wouldn’t.
Comment 9 peavo 2015-06-30 10:39:46 PDT 
(In reply to comment #8)
> (In reply to comment #3)
> > I seem to remember there was an
> > agreement not to add the override keyword when overriding pure virtual
> > methods?
> 
> This is the first I have heard of this. I think we should definitely use
> override in cases like that and I’d like to know more about why we wouldn’t.

Ok, sorry for spreading false rumours. I thought I picked this up somewhere, but I am probably wrong.