Bug 146450 - [WinCairo] Compile error, WebEditorClient::didApplyStyle() should not have any parameters.
Summary: [WinCairo] Compile error, WebEditorClient::didApplyStyle() should not have an...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-30 02:18 PDT by peavo
Modified: 2015-06-30 10:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.99 KB, patch)
2015-06-30 02:20 PDT, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.