Remove unnecessary 'virtual' annotation from mojo

Remove unnecessary 'virtual' annotation from mojo

Removes virtual keyword from overridden methods in
mojo/services/html_viewer.

BUG=417463, 469785
Committed: https://crrev.com/675b9a2fae1b3fc36121c45dd144d572e499d36b
Cr-Commit-Position: refs/heads/master@{#323102}

[cwgreene, 2015-03-27 19:31:53]

cwgreene@amazon.com changed reviewers:
+ sky@chromium.org

[cwgreene, 2015-03-27 19:31:54]

PTAL! :)

[sky, 2015-03-27 21:24:26]

I don't see you're name in the list of folks that have signed the CLA:

https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
File mojo/services/html_viewer/websockethandle_impl.h (right):

https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/websockethandle_impl.h:35: const
blink::WebVector<blink::WebString>& protocols,
Run git cl format as the spacing needs to be updated now.

https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
File mojo/services/html_viewer/weburlloader_impl.h (right):

https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/weburlloader_impl.h:35: virtual ~WebURLLoaderImpl();
If you're going fix overrides you should fix destructors too.

[cwgreene, 2015-03-27 21:42:53]

On 2015/03/27 21:24:26, sky wrote:
> I don't see you're name in the list of folks that have signed the CLA:
> 
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> File mojo/services/html_viewer/websockethandle_impl.h (right):
CLA approval is underway.
> 
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> mojo/services/html_viewer/websockethandle_impl.h:35: const
> blink::WebVector<blink::WebString>& protocols,
> Run git cl format as the spacing needs to be updated now.
Thanks, I think my workspace is misconfigured, as this should have been run
before the submit. Sorry about that!
> 
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> File mojo/services/html_viewer/weburlloader_impl.h (right):
> 
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> mojo/services/html_viewer/weburlloader_impl.h:35: virtual ~WebURLLoaderImpl();
> If you're going fix overrides you should fix destructors too.
Ah good catch! I had forgotten about the destructors. Does the clang plugin
detect when destructors have been overridden but are still marked as virtual?

[cwgreene, 2015-03-27 21:51:56]

On 2015/03/27 21:42:53, cwgreene wrote:
> On 2015/03/27 21:24:26, sky wrote:
> > I don't see you're name in the list of folks that have signed the CLA:
> > 
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> > File mojo/services/html_viewer/websockethandle_impl.h (right):
> CLA approval is underway.
> > 
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> > mojo/services/html_viewer/websockethandle_impl.h:35: const
> > blink::WebVector<blink::WebString>& protocols,
> > Run git cl format as the spacing needs to be updated now.
> Thanks, I think my workspace is misconfigured, as this should have been run
> before the submit. Sorry about that!
> > 
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> > File mojo/services/html_viewer/weburlloader_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> > mojo/services/html_viewer/weburlloader_impl.h:35: virtual
~WebURLLoaderImpl();
> > If you're going fix overrides you should fix destructors too.
> Ah good catch! I had forgotten about the destructors. Does the clang plugin
> detect when destructors have been overridden but are still marked as virtual?

I just noticed that there are a number of methods marked virtual that are in
fact overriding a parent method; should I fix them up too?

[sky, 2015-03-27 22:38:21]

Yes please

On Fri, Mar 27, 2015 at 2:51 PM,
<reply@chromiumcodereview-hr.appspotmail.com> wrote:
> On 2015/03/27 21:42:53, cwgreene wrote:
>>
>> On 2015/03/27 21:24:26, sky wrote:
>> > I don't see you're name in the list of folks that have signed the CLA:
>> >
>> >
>
>
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
>>
>> > File mojo/services/html_viewer/websockethandle_impl.h (right):
>> CLA approval is underway.
>> >
>> >
>
>
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
>>
>> > mojo/services/html_viewer/websockethandle_impl.h:35: const
>> > blink::WebVector<blink::WebString>& protocols,
>> > Run git cl format as the spacing needs to be updated now.
>> Thanks, I think my workspace is misconfigured, as this should have been
>> run
>> before the submit. Sorry about that!
>> >
>> >
>
>
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
>>
>> > File mojo/services/html_viewer/weburlloader_impl.h (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
>>
>> > mojo/services/html_viewer/weburlloader_impl.h:35: virtual
>
> ~WebURLLoaderImpl();
>>
>> > If you're going fix overrides you should fix destructors too.
>> Ah good catch! I had forgotten about the destructors. Does the clang
>> plugin
>> detect when destructors have been overridden but are still marked as
>> virtual?
>
>
> I just noticed that there are a number of methods marked virtual that are in
> fact overriding a parent method; should I fix them up too?
>
> https://codereview.chromium.org/1037363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cwgreene, 2015-03-31 17:05:42]

Patch set updated, and CLA approval gained.

On 2015/03/27 22:38:21, sky wrote:
> Yes please
> 
> On Fri, Mar 27, 2015 at 2:51 PM,
> <mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> > On 2015/03/27 21:42:53, cwgreene wrote:
> >>
> >> On 2015/03/27 21:24:26, sky wrote:
> >> > I don't see you're name in the list of folks that have signed the CLA:
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> >>
> >> > File mojo/services/html_viewer/websockethandle_impl.h (right):
> >> CLA approval is underway.
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> >>
> >> > mojo/services/html_viewer/websockethandle_impl.h:35: const
> >> > blink::WebVector<blink::WebString>& protocols,
> >> > Run git cl format as the spacing needs to be updated now.
> >> Thanks, I think my workspace is misconfigured, as this should have been
> >> run
> >> before the submit. Sorry about that!
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> >>
> >> > File mojo/services/html_viewer/weburlloader_impl.h (right):
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/1037363002/diff/20001/mojo/services/html_view...
> >>
> >> > mojo/services/html_viewer/weburlloader_impl.h:35: virtual
> >
> > ~WebURLLoaderImpl();
> >>
> >> > If you're going fix overrides you should fix destructors too.
> >> Ah good catch! I had forgotten about the destructors. Does the clang
> >> plugin
> >> detect when destructors have been overridden but are still marked as
> >> virtual?
> >
> >
> > I just noticed that there are a number of methods marked virtual that are in
> > fact overriding a parent method; should I fix them up too?
> >
> > https://codereview.chromium.org/1037363002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-03-31 17:15:57]

https://codereview.chromium.org/1037363002/diff/40001/mojo/services/html_view...
File mojo/services/html_viewer/webthread_impl.h (right):

https://codereview.chromium.org/1037363002/diff/40001/mojo/services/html_view...
mojo/services/html_viewer/webthread_impl.h:24: blink::PlatformThreadId
threadId() const override = 0;
I don't think this is necessary, can you remove it?

[cwgreene, 2015-03-31 18:02:05]

On 2015/03/31 17:15:57, sky wrote:
>
https://codereview.chromium.org/1037363002/diff/40001/mojo/services/html_view...
> File mojo/services/html_viewer/webthread_impl.h (right):
> 
>
https://codereview.chromium.org/1037363002/diff/40001/mojo/services/html_view...
> mojo/services/html_viewer/webthread_impl.h:24: blink::PlatformThreadId
> threadId() const override = 0;
> I don't think this is necessary, can you remove it?

I have deleted the line and recompiled without issues. Is there a test suite for
this component I should run?

[sky, 2015-03-31 18:06:16]

Nope, LGTM

[cwgreene, 2015-03-31 18:11:42]

The CQ bit was checked by cwgreene@amazon.com

[commit-bot: I haz the power, 2015-03-31 18:12:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1037363002/60001

[commit-bot: I haz the power, 2015-03-31 18:18:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-31 18:18:15]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cwgreene, 2015-03-31 18:25:32]

On 2015/03/31 18:18:15, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Forgot to sync; Resolved merge conflict, rebuilding.

[cwgreene, 2015-03-31 20:09:16]

On 2015/03/31 18:25:32, cwgreene wrote:
> On 2015/03/31 18:18:15, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Forgot to sync; Resolved merge conflict, rebuilding.

Resolved merge conflicts and rebuilt.

[cwgreene, 2015-03-31 20:14:03]

The CQ bit was checked by cwgreene@amazon.com

[cwgreene, 2015-03-31 20:14:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1037363002/#ps80001
(title: "Resolved merge conflicts")

[commit-bot: I haz the power, 2015-03-31 20:14:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1037363002/80001

[commit-bot: I haz the power, 2015-03-31 20:47:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-31 20:48:05]

Patchset 5 (id:??) landed as
https://crrev.com/675b9a2fae1b3fc36121c45dd144d572e499d36b
Cr-Commit-Position: refs/heads/master@{#323102}