Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
42143002:
Make createServiceWorkerProvider stub smarter (Closed)
|
Created:
7 years, 1 month ago by alecflett Modified:
6 years, 10 months ago Reviewers:
abarth-chromium CC:
blink-reviews, jamesr, dglazkov+blink Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionMake createServiceWorkerProvider stub smarter
I'm not sure if this is necessary or not. It is certainly "more correct" but
its code that should never be reached.
BUG=285976
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix #include #
Total comments: 1
Patch Set 3 : Fix #include lines and delete #
Total comments: 1
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/42143002/diff/1/public/web/WebFrameClient.h File public/web/WebFrameClient.h (right): https://codereview.chromium.org/42143002/diff/1/public/web/WebFrameClient.h#n... public/web/WebFrameClient.h:101: return 0; might read better as { delete client; return 0; } all on the same line, there's no harm in calling delete on a null ptr
As per a suggestion in https://codereview.chromium.org/25008006/
On 2013/10/24 22:54:21, alecflett wrote: > As per a suggestion in https://codereview.chromium.org/25008006/ Note that I had to bypass checkdeps to get this uploaded - no rule lets web/* include ../platform/* - there are others here in this file though, which is interesting.
https://codereview.chromium.org/42143002/diff/40001/public/web/WebFrameClient.h File public/web/WebFrameClient.h (right): https://codereview.chromium.org/42143002/diff/40001/public/web/WebFrameClient... public/web/WebFrameClient.h:37: #include "../platform/WebServiceWorkerProviderClient.h" i think the deps get confused because of the relative paths, you can probably switch these all to... #include "public/platform/Web[Somthing].h"
learn something new every day. https://codereview.chromium.org/42143002/diff/1/public/web/WebFrameClient.h File public/web/WebFrameClient.h (right): https://codereview.chromium.org/42143002/diff/1/public/web/WebFrameClient.h#n... public/web/WebFrameClient.h:101: return 0; On 2013/10/24 22:53:37, michaeln wrote: > might read better as { delete client; return 0; } all on the same line, there's > no harm in calling delete on a null ptr oh wow, sure enough. apparently the last time I manually 'delete'ed a pointer in C++ was 2003, when this wasn't true :)
https://codereview.chromium.org/42143002/diff/20002/public/web/WebFrameClient.h File public/web/WebFrameClient.h (right): https://codereview.chromium.org/42143002/diff/20002/public/web/WebFrameClient... public/web/WebFrameClient.h:97: virtual WebServiceWorkerProvider* createServiceWorkerProvider(WebFrame*, WebServiceWorkerProviderClient* client ) { delete client; return 0; } I don't think we want to call |delete| in an API header. It's not clear which DLL will call |delete|... Maybe we should make this pure virtual to force the embedder to implement it? Alternatively, we could move the implementation out of line so that it's clear that |delete| is called inside the Blink DLL.
On 2013/10/25 15:29:50, abarth wrote: > https://codereview.chromium.org/42143002/diff/20002/public/web/WebFrameClient.h > File public/web/WebFrameClient.h (right): > > https://codereview.chromium.org/42143002/diff/20002/public/web/WebFrameClient... > public/web/WebFrameClient.h:97: virtual WebServiceWorkerProvider* > createServiceWorkerProvider(WebFrame*, WebServiceWorkerProviderClient* client ) > { delete client; return 0; } > I don't think we want to call |delete| in an API header. It's not clear which > DLL will call |delete|... Maybe we should make this pure virtual to force the > embedder to implement it? Alternatively, we could move the implementation out > of line so that it's clear that |delete| is called inside the Blink DLL. FWIW, The destructor of WebServiceWorkerProviderClient is virtual.. in my mind that mitigates this. I'm happy to put it out-of-line, but I don't see any precident for that (WebFrameClient is 100% header-defined) I'll try making this pure-virtual though I don't see precident for that in WebFrameClient.h either..
The motivation for the suggestion was mostly for self-documenting purposes,
out-out-line would defeat that.
> > { delete client; return 0; }
> > I don't think we want to call |delete| in an API header. It's not clear
which
> > DLL will call |delete|... Maybe we should make this pure virtual to force
the
> > embedder to implement it? Alternatively, we could move the implementation
out
> > of line so that it's clear that |delete| is called inside the Blink DLL.
Interesting point, but does it matter which will call delete? The way the api is
constructed and used, in some cases delete is invoked by the webkit/blink DLL
and in other cases by the embedding DLL. I hope that's not an issue (it isn't to
my knowledge), we have other apis in the webkit layer that result in the created
here / deleted there control flow (one shot callback interface sort of objects
come to mind).
> I'll try making this pure-virtual though I don't see precident for that in
> WebFrameClient.h either..
We generally avoid pure virtuals here to ease two sided patch work (which i
think is a good reason).
Another option is to just leave it as is with the comment and the leaky impl
(and maybe an additional comment about how a real impl shouldn't leak).
On 2013/10/25 17:39:37, alecflett wrote:
> FWIW, The destructor of WebServiceWorkerProviderClient is virtual.. in my mind
> that mitigates this.
That doesn't help. You don't know which DLL the virtual function will be linked
into.
> I'm happy to put it out-of-line, but I don't see any precident for that
> (WebFrameClient is 100% header-defined)
>
> I'll try making this pure-virtual though I don't see precident for that in
> WebFrameClient.h either..
The ideal thing would be to use a smart pointer type like PassOwnPtr to document
that we're passing ownership through the API. Unfortunately, we don't have an
API type for that.
On 2013/10/25 21:12:13, michaeln wrote:
> The motivation for the suggestion was mostly for self-documenting purposes,
> out-out-line would defeat that.
There's already a comment, but documentation in code is better than
documentation in comments.
> > > { delete client; return 0; }
> > > I don't think we want to call |delete| in an API header. It's not clear
> which
> > > DLL will call |delete|... Maybe we should make this pure virtual to force
> the
> > > embedder to implement it? Alternatively, we could move the implementation
> out
> > > of line so that it's clear that |delete| is called inside the Blink DLL.
>
> Interesting point, but does it matter which will call delete? The way the api
is
> constructed and used, in some cases delete is invoked by the webkit/blink DLL
> and in other cases by the embedding DLL. I hope that's not an issue (it isn't
to
> my knowledge), we have other apis in the webkit layer that result in the
created
> here / deleted there control flow (one shot callback interface sort of objects
> come to mind).
Currently |delete| does the same thing in both DLLs. However, that might not
continue to be the case in the future. For example, it's not the case in the
apple-mac port of WebKit.
> > I'll try making this pure-virtual though I don't see precident for that in
> > WebFrameClient.h either..
>
> We generally avoid pure virtuals here to ease two sided patch work (which i
> think is a good reason).
>
> Another option is to just leave it as is with the comment and the leaky impl
> (and maybe an additional comment about how a real impl shouldn't leak).
You can add an ASSERT_NOT_REACHED() to emphasize that the default implementation
shouldn't be used because it leaks.
|
