Provide an overload to WebView::create that does not require a WebViewClient.

Provide an overload to WebView::create that does not require a WebViewClient.

Provide an overload to WebView::create and WebViewImpl::create where a
that does not require WebViewClient or nullptr to be passed.

This is part of the pre-work to make it invalid to pass a nullptr to
WebView::create - it makes more sense for clients to be able to call a specific
method when they don't want to supply a client rathen than passing nullptr.

Also added DCHECK() to WebView*::create(WebViewClient*) to prevent nullptr being
passed, and went and cleaned up all of the call sites.

Followup work will create a internal specialization of WebViewClient that will
behave the way that a null WebViewClient works today.

BUG=696895
COMMIT=false

[slangley, 2017-03-09 04:31:00]

slangley@chromium.org changed reviewers:
+ sashab@chromium.org

[sashab, 2017-03-09 04:38:28]

Description was changed from

==========
Provide an overload to WebView::create and WebViewImpl::create where a
that does not require WebViewClient or nullptr to be passed.

This is part of the pre-work to make it invalid to pass a nullptr to
WebView::create - it makes more sense for clients to be able to call a specific
method when they don't want to supply a client rathen than passing nullptr.

Also added DCHECK() to WebView*::create(WebViewClient*) to prevent nullptr being
passed, and went and cleaned up all of the call sites.

Followup work will create a internal specialization of WebViewClient that will
behave the way that a null WebViewClient works today.

BUG=696895
==========

to

==========
Provide an overload to WebView::create that does not require a WebViewClient.

Provide an overload to WebView::create and WebViewImpl::create where a
that does not require WebViewClient or nullptr to be passed.

This is part of the pre-work to make it invalid to pass a nullptr to
WebView::create - it makes more sense for clients to be able to call a specific
method when they don't want to supply a client rathen than passing nullptr.

Also added DCHECK() to WebView*::create(WebViewClient*) to prevent nullptr being
passed, and went and cleaned up all of the call sites.

Followup work will create a internal specialization of WebViewClient that will
behave the way that a null WebViewClient works today.

BUG=696895
==========

[sashab, 2017-03-09 04:43:10]

LGTM but please get another review :)

Also great CL description, you're starting to get the hang of this ;)

[slangley, 2017-03-09 04:54:38]

slangley@chromium.org changed reviewers:
+ dcheng@chromium.org

[slangley, 2017-03-09 04:54:39]

dcheng - ptal

[dcheng, 2017-03-09 05:13:06]

On 2017/03/09 04:54:39, slangley wrote:
> dcheng - ptal

Hmm, so one of the motivating reasons for this is to remove a lot of the if
(m_client) checks in WebViewImpl. If we're still passing null in one path, then
we'll still need to keep the DCHECKs. My original thinking was just to make the
callers that pass nullptr hold a WebViewClient.

[slangley, 2017-03-09 05:24:46]

On 2017/03/09 05:13:06, dcheng wrote:
> On 2017/03/09 04:54:39, slangley wrote:
> > dcheng - ptal
> 
> Hmm, so one of the motivating reasons for this is to remove a lot of the if
> (m_client) checks in WebViewImpl. If we're still passing null in one path,
then
> we'll still need to keep the DCHECKs. My original thinking was just to make
the
> callers that pass nullptr hold a WebViewClient.

Callers cannot pass in a WebViewClient due to:
(a) protected dtor() and 
(b) the logic would be slightly different in some places.

My plan is to derive a new type of WebViewClient that will ensure the logic
matches
with what happens now if m_client is null, and then I can strip all of the if
(m_client)
type logic.

This CL removes the validity of passing nullptr from the public API - subsequent
CL's
would work inside web/.

SGTY?

[dcheng, 2017-03-09 05:30:42]

On 2017/03/09 05:24:46, slangley wrote:
> On 2017/03/09 05:13:06, dcheng wrote:
> > On 2017/03/09 04:54:39, slangley wrote:
> > > dcheng - ptal
> > 
> > Hmm, so one of the motivating reasons for this is to remove a lot of the if
> > (m_client) checks in WebViewImpl. If we're still passing null in one path,
> then
> > we'll still need to keep the DCHECKs. My original thinking was just to make
> the
> > callers that pass nullptr hold a WebViewClient.
> 
> Callers cannot pass in a WebViewClient due to:
> (a) protected dtor() and 
> (b) the logic would be slightly different in some places.
> 
> My plan is to derive a new type of WebViewClient that will ensure the logic
> matches
> with what happens now if m_client is null, and then I can strip all of the if
> (m_client)
> type logic.

I regard the difference in logic as a bug. I would like to not bother preserving
the existing behavior when m_client is null.

> 
> This CL removes the validity of passing nullptr from the public API -
subsequent
> CL's
> would work inside web/.
> 
> SGTY?

Would we eventually remove the WebViewClient::create() overload then?

[slangley, 2017-03-09 05:36:05]

On 2017/03/09 05:30:42, dcheng wrote:
> On 2017/03/09 05:24:46, slangley wrote:
> > On 2017/03/09 05:13:06, dcheng wrote:
> > > On 2017/03/09 04:54:39, slangley wrote:
> > > > dcheng - ptal
> > > 
> > > Hmm, so one of the motivating reasons for this is to remove a lot of the
if
> > > (m_client) checks in WebViewImpl. If we're still passing null in one path,
> > then
> > > we'll still need to keep the DCHECKs. My original thinking was just to
make
> > the
> > > callers that pass nullptr hold a WebViewClient.
> > 
> > Callers cannot pass in a WebViewClient due to:
> > (a) protected dtor() and 
> > (b) the logic would be slightly different in some places.
> > 
> > My plan is to derive a new type of WebViewClient that will ensure the logic
> > matches
> > with what happens now if m_client is null, and then I can strip all of the
if
> > (m_client)
> > type logic.
> 
> I regard the difference in logic as a bug. I would like to not bother
preserving
> the existing behavior when m_client is null.
> 
> > 
> > This CL removes the validity of passing nullptr from the public API -
> subsequent
> > CL's
> > would work inside web/.
> > 
> > SGTY?
> 
> Would we eventually remove the WebViewClient::create() overload then?

Well depends - to remove it would involve adding some class in public/web that
allowed callers to say "I don't care about callbacks" and then pass that class
to create. To me this pattern is more naturally achieved using the overload.

The same could potentially be achieved by making the WebViewClient dtor()
public, but that changes the ownership semantics of WebViewClient when supplied
to WebView::create().

[dcheng, 2017-03-09 19:35:35]

On 2017/03/09 05:36:05, slangley wrote:
> On 2017/03/09 05:30:42, dcheng wrote:
> > On 2017/03/09 05:24:46, slangley wrote:
> > > On 2017/03/09 05:13:06, dcheng wrote:
> > > > On 2017/03/09 04:54:39, slangley wrote:
> > > > > dcheng - ptal
> > > > 
> > > > Hmm, so one of the motivating reasons for this is to remove a lot of the
> if
> > > > (m_client) checks in WebViewImpl. If we're still passing null in one
path,
> > > then
> > > > we'll still need to keep the DCHECKs. My original thinking was just to
> make
> > > the
> > > > callers that pass nullptr hold a WebViewClient.
> > > 
> > > Callers cannot pass in a WebViewClient due to:
> > > (a) protected dtor() and 
> > > (b) the logic would be slightly different in some places.
> > > 
> > > My plan is to derive a new type of WebViewClient that will ensure the
logic
> > > matches
> > > with what happens now if m_client is null, and then I can strip all of the
> if
> > > (m_client)
> > > type logic.
> > 
> > I regard the difference in logic as a bug. I would like to not bother
> preserving
> > the existing behavior when m_client is null.
> > 
> > > 
> > > This CL removes the validity of passing nullptr from the public API -
> > subsequent
> > > CL's
> > > would work inside web/.
> > > 
> > > SGTY?
> > 
> > Would we eventually remove the WebViewClient::create() overload then?
> 
> Well depends - to remove it would involve adding some class in public/web that
> allowed callers to say "I don't care about callbacks" and then pass that class
> to create. To me this pattern is more naturally achieved using the overload.

Doesn't that mean we'd need to remove the one-argument overload eventually
anyway? Is this just for convenience in finding those callsites as we do this
transition?

> 
> The same could potentially be achieved by making the WebViewClient dtor()
> public, but that changes the ownership semantics of WebViewClient when
supplied
> to WebView::create().

AFAIk, it's only protected so that an embedder is forced to declare a subclass
rather than just reusing the default interface.

[slangley, 2017-03-09 22:35:53]

On 2017/03/09 19:35:35, dcheng wrote:
> On 2017/03/09 05:36:05, slangley wrote:
> > On 2017/03/09 05:30:42, dcheng wrote:
> > > On 2017/03/09 05:24:46, slangley wrote:
> > > > On 2017/03/09 05:13:06, dcheng wrote:
> > > > > On 2017/03/09 04:54:39, slangley wrote:
> > > > > > dcheng - ptal
> > > > > 
> > > > > Hmm, so one of the motivating reasons for this is to remove a lot of
the
> > if
> > > > > (m_client) checks in WebViewImpl. If we're still passing null in one
> path,
> > > > then
> > > > > we'll still need to keep the DCHECKs. My original thinking was just to
> > make
> > > > the
> > > > > callers that pass nullptr hold a WebViewClient.
> > > > 
> > > > Callers cannot pass in a WebViewClient due to:
> > > > (a) protected dtor() and 
> > > > (b) the logic would be slightly different in some places.
> > > > 
> > > > My plan is to derive a new type of WebViewClient that will ensure the
> logic
> > > > matches
> > > > with what happens now if m_client is null, and then I can strip all of
the
> > if
> > > > (m_client)
> > > > type logic.
> > > 
> > > I regard the difference in logic as a bug. I would like to not bother
> > preserving
> > > the existing behavior when m_client is null.
> > > 
> > > > 
> > > > This CL removes the validity of passing nullptr from the public API -
> > > subsequent
> > > > CL's
> > > > would work inside web/.
> > > > 
> > > > SGTY?
> > > 
> > > Would we eventually remove the WebViewClient::create() overload then?
> > 
> > Well depends - to remove it would involve adding some class in public/web
that
> > allowed callers to say "I don't care about callbacks" and then pass that
class
> > to create. To me this pattern is more naturally achieved using the overload.
> 
> Doesn't that mean we'd need to remove the one-argument overload eventually
> anyway? Is this just for convenience in finding those callsites as we do this
> transition?

So my idea is to leave the overload and make it part of the public API. I'm not
sure if this goes against some prior art or convention, but seems the most
obvious choice to me.

> 
> > 
> > The same could potentially be achieved by making the WebViewClient dtor()
> > public, but that changes the ownership semantics of WebViewClient when
> supplied
> > to WebView::create().
> 
> AFAIk, it's only protected so that an embedder is forced to declare a subclass
> rather than just reusing the default interface.

True, but generally you'd do that with a protected ctor() I guess. The protected
dtor() means that you can never call delete on the base class type so you can't
transfer ownership. I'm not sure if that was the original implementors intention
tho.

Generally, if this CL gets in I do not plan on making future modifications to
the public API.

[slangley, 2017-03-09 22:42:31]

On 2017/03/09 22:35:53, slangley wrote:
> On 2017/03/09 19:35:35, dcheng wrote:
> > On 2017/03/09 05:36:05, slangley wrote:
> > > On 2017/03/09 05:30:42, dcheng wrote:
> > > > On 2017/03/09 05:24:46, slangley wrote:
> > > > > On 2017/03/09 05:13:06, dcheng wrote:
> > > > > > On 2017/03/09 04:54:39, slangley wrote:
> > > > > > > dcheng - ptal
> > > > > > 
> > > > > > Hmm, so one of the motivating reasons for this is to remove a lot of
> the
> > > if
> > > > > > (m_client) checks in WebViewImpl. If we're still passing null in one
> > path,
> > > > > then
> > > > > > we'll still need to keep the DCHECKs. My original thinking was just
to
> > > make
> > > > > the
> > > > > > callers that pass nullptr hold a WebViewClient.
> > > > > 
> > > > > Callers cannot pass in a WebViewClient due to:
> > > > > (a) protected dtor() and 
> > > > > (b) the logic would be slightly different in some places.
> > > > > 
> > > > > My plan is to derive a new type of WebViewClient that will ensure the
> > logic
> > > > > matches
> > > > > with what happens now if m_client is null, and then I can strip all of
> the
> > > if
> > > > > (m_client)
> > > > > type logic.
> > > > 
> > > > I regard the difference in logic as a bug. I would like to not bother
> > > preserving
> > > > the existing behavior when m_client is null.
> > > > 
> > > > > 
> > > > > This CL removes the validity of passing nullptr from the public API -
> > > > subsequent
> > > > > CL's
> > > > > would work inside web/.
> > > > > 
> > > > > SGTY?
> > > > 
> > > > Would we eventually remove the WebViewClient::create() overload then?
> > > 
> > > Well depends - to remove it would involve adding some class in public/web
> that
> > > allowed callers to say "I don't care about callbacks" and then pass that
> class
> > > to create. To me this pattern is more naturally achieved using the
overload.
> > 
> > Doesn't that mean we'd need to remove the one-argument overload eventually
> > anyway? Is this just for convenience in finding those callsites as we do
this
> > transition?
> 
> So my idea is to leave the overload and make it part of the public API. I'm
not
> sure if this goes against some prior art or convention, but seems the most
> obvious choice to me.
> 
> > 
> > > 
> > > The same could potentially be achieved by making the WebViewClient dtor()
> > > public, but that changes the ownership semantics of WebViewClient when
> > supplied
> > > to WebView::create().
> > 
> > AFAIk, it's only protected so that an embedder is forced to declare a
subclass
> > rather than just reusing the default interface.
> 
> True, but generally you'd do that with a protected ctor() I guess. The
protected
> dtor() means that you can never call delete on the base class type so you
can't
> transfer ownership. I'm not sure if that was the original implementors
intention
> tho.
> 
> Generally, if this CL gets in I do not plan on making future modifications to
> the public API.

Also I'd like take the same approach with WebWidgetClient, although there are
fewer cases of nullptr being passed in.

[dcheng, 2017-03-09 22:49:22]

On 2017/03/09 22:42:31, slangley wrote:
> On 2017/03/09 22:35:53, slangley wrote:
> > On 2017/03/09 19:35:35, dcheng wrote:
> > > On 2017/03/09 05:36:05, slangley wrote:
> > > > On 2017/03/09 05:30:42, dcheng wrote:
> > > > > On 2017/03/09 05:24:46, slangley wrote:
> > > > > > On 2017/03/09 05:13:06, dcheng wrote:
> > > > > > > On 2017/03/09 04:54:39, slangley wrote:
> > > > > > > > dcheng - ptal
> > > > > > > 
> > > > > > > Hmm, so one of the motivating reasons for this is to remove a lot
of
> > the
> > > > if
> > > > > > > (m_client) checks in WebViewImpl. If we're still passing null in
one
> > > path,
> > > > > > then
> > > > > > > we'll still need to keep the DCHECKs. My original thinking was
just
> to
> > > > make
> > > > > > the
> > > > > > > callers that pass nullptr hold a WebViewClient.
> > > > > > 
> > > > > > Callers cannot pass in a WebViewClient due to:
> > > > > > (a) protected dtor() and 
> > > > > > (b) the logic would be slightly different in some places.
> > > > > > 
> > > > > > My plan is to derive a new type of WebViewClient that will ensure
the
> > > logic
> > > > > > matches
> > > > > > with what happens now if m_client is null, and then I can strip all
of
> > the
> > > > if
> > > > > > (m_client)
> > > > > > type logic.
> > > > > 
> > > > > I regard the difference in logic as a bug. I would like to not bother
> > > > preserving
> > > > > the existing behavior when m_client is null.
> > > > > 
> > > > > > 
> > > > > > This CL removes the validity of passing nullptr from the public API
-
> > > > > subsequent
> > > > > > CL's
> > > > > > would work inside web/.
> > > > > > 
> > > > > > SGTY?
> > > > > 
> > > > > Would we eventually remove the WebViewClient::create() overload then?
> > > > 
> > > > Well depends - to remove it would involve adding some class in
public/web
> > that
> > > > allowed callers to say "I don't care about callbacks" and then pass that
> > class
> > > > to create. To me this pattern is more naturally achieved using the
> overload.
> > > 
> > > Doesn't that mean we'd need to remove the one-argument overload eventually
> > > anyway? Is this just for convenience in finding those callsites as we do
> this
> > > transition?
> > 
> > So my idea is to leave the overload and make it part of the public API. I'm
> not
> > sure if this goes against some prior art or convention, but seems the most
> > obvious choice to me.
> > 
> > > 
> > > > 
> > > > The same could potentially be achieved by making the WebViewClient
dtor()
> > > > public, but that changes the ownership semantics of WebViewClient when
> > > supplied
> > > > to WebView::create().
> > > 
> > > AFAIk, it's only protected so that an embedder is forced to declare a
> subclass
> > > rather than just reusing the default interface.
> > 
> > True, but generally you'd do that with a protected ctor() I guess. The
> protected
> > dtor() means that you can never call delete on the base class type so you
> can't
> > transfer ownership. I'm not sure if that was the original implementors
> intention
> > tho.
> > 
> > Generally, if this CL gets in I do not plan on making future modifications
to
> > the public API.
> 
> Also I'd like take the same approach with WebWidgetClient, although there are
> fewer cases of nullptr being passed in.

I think my preference is still towards making callers have an actual
WebViewClient instantiated. Presumably, the version that doesn't require a
client would have to use a static WebViewClient somewhere.

[slangley, 2017-03-09 22:55:30]

On 2017/03/09 22:49:22, dcheng wrote:
> On 2017/03/09 22:42:31, slangley wrote:
> > On 2017/03/09 22:35:53, slangley wrote:
> > > On 2017/03/09 19:35:35, dcheng wrote:
> > > > On 2017/03/09 05:36:05, slangley wrote:
> > > > > On 2017/03/09 05:30:42, dcheng wrote:
> > > > > > On 2017/03/09 05:24:46, slangley wrote:
> > > > > > > On 2017/03/09 05:13:06, dcheng wrote:
> > > > > > > > On 2017/03/09 04:54:39, slangley wrote:
> > > > > > > > > dcheng - ptal
> > > > > > > > 
> > > > > > > > Hmm, so one of the motivating reasons for this is to remove a
lot
> of
> > > the
> > > > > if
> > > > > > > > (m_client) checks in WebViewImpl. If we're still passing null in
> one
> > > > path,
> > > > > > > then
> > > > > > > > we'll still need to keep the DCHECKs. My original thinking was
> just
> > to
> > > > > make
> > > > > > > the
> > > > > > > > callers that pass nullptr hold a WebViewClient.
> > > > > > > 
> > > > > > > Callers cannot pass in a WebViewClient due to:
> > > > > > > (a) protected dtor() and 
> > > > > > > (b) the logic would be slightly different in some places.
> > > > > > > 
> > > > > > > My plan is to derive a new type of WebViewClient that will ensure
> the
> > > > logic
> > > > > > > matches
> > > > > > > with what happens now if m_client is null, and then I can strip
all
> of
> > > the
> > > > > if
> > > > > > > (m_client)
> > > > > > > type logic.
> > > > > > 
> > > > > > I regard the difference in logic as a bug. I would like to not
bother
> > > > > preserving
> > > > > > the existing behavior when m_client is null.
> > > > > > 
> > > > > > > 
> > > > > > > This CL removes the validity of passing nullptr from the public
API
> -
> > > > > > subsequent
> > > > > > > CL's
> > > > > > > would work inside web/.
> > > > > > > 
> > > > > > > SGTY?
> > > > > > 
> > > > > > Would we eventually remove the WebViewClient::create() overload
then?
> > > > > 
> > > > > Well depends - to remove it would involve adding some class in
> public/web
> > > that
> > > > > allowed callers to say "I don't care about callbacks" and then pass
that
> > > class
> > > > > to create. To me this pattern is more naturally achieved using the
> > overload.
> > > > 
> > > > Doesn't that mean we'd need to remove the one-argument overload
eventually
> > > > anyway? Is this just for convenience in finding those callsites as we do
> > this
> > > > transition?
> > > 
> > > So my idea is to leave the overload and make it part of the public API.
I'm
> > not
> > > sure if this goes against some prior art or convention, but seems the most
> > > obvious choice to me.
> > > 
> > > > 
> > > > > 
> > > > > The same could potentially be achieved by making the WebViewClient
> dtor()
> > > > > public, but that changes the ownership semantics of WebViewClient when
> > > > supplied
> > > > > to WebView::create().
> > > > 
> > > > AFAIk, it's only protected so that an embedder is forced to declare a
> > subclass
> > > > rather than just reusing the default interface.
> > > 
> > > True, but generally you'd do that with a protected ctor() I guess. The
> > protected
> > > dtor() means that you can never call delete on the base class type so you
> > can't
> > > transfer ownership. I'm not sure if that was the original implementors
> > intention
> > > tho.
> > > 
> > > Generally, if this CL gets in I do not plan on making future modifications
> to
> > > the public API.
> > 
> > Also I'd like take the same approach with WebWidgetClient, although there
are
> > fewer cases of nullptr being passed in.
> 
> I think my preference is still towards making callers have an actual
> WebViewClient instantiated. Presumably, the version that doesn't require a
> client would have to use a static WebViewClient somewhere.

So do you want to see how things shake out and add a TODO to consider this
later, or do you want me to try and add it now?

FWIW I'm not overly confident that it's possible to remove the null checks
without providing a specialization of WebViewClient() or altering the current
specializations of WebViewClient but time will tell. e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...

[dcheng, 2017-03-09 23:06:17]

On 2017/03/09 22:55:30, slangley wrote:
> On 2017/03/09 22:49:22, dcheng wrote:
> > On 2017/03/09 22:42:31, slangley wrote:
> > > On 2017/03/09 22:35:53, slangley wrote:
> > > > On 2017/03/09 19:35:35, dcheng wrote:
> > > > > On 2017/03/09 05:36:05, slangley wrote:
> > > > > > On 2017/03/09 05:30:42, dcheng wrote:
> > > > > > > On 2017/03/09 05:24:46, slangley wrote:
> > > > > > > > On 2017/03/09 05:13:06, dcheng wrote:
> > > > > > > > > On 2017/03/09 04:54:39, slangley wrote:
> > > > > > > > > > dcheng - ptal
> > > > > > > > > 
> > > > > > > > > Hmm, so one of the motivating reasons for this is to remove a
> lot
> > of
> > > > the
> > > > > > if
> > > > > > > > > (m_client) checks in WebViewImpl. If we're still passing null
in
> > one
> > > > > path,
> > > > > > > > then
> > > > > > > > > we'll still need to keep the DCHECKs. My original thinking was
> > just
> > > to
> > > > > > make
> > > > > > > > the
> > > > > > > > > callers that pass nullptr hold a WebViewClient.
> > > > > > > > 
> > > > > > > > Callers cannot pass in a WebViewClient due to:
> > > > > > > > (a) protected dtor() and 
> > > > > > > > (b) the logic would be slightly different in some places.
> > > > > > > > 
> > > > > > > > My plan is to derive a new type of WebViewClient that will
ensure
> > the
> > > > > logic
> > > > > > > > matches
> > > > > > > > with what happens now if m_client is null, and then I can strip
> all
> > of
> > > > the
> > > > > > if
> > > > > > > > (m_client)
> > > > > > > > type logic.
> > > > > > > 
> > > > > > > I regard the difference in logic as a bug. I would like to not
> bother
> > > > > > preserving
> > > > > > > the existing behavior when m_client is null.
> > > > > > > 
> > > > > > > > 
> > > > > > > > This CL removes the validity of passing nullptr from the public
> API
> > -
> > > > > > > subsequent
> > > > > > > > CL's
> > > > > > > > would work inside web/.
> > > > > > > > 
> > > > > > > > SGTY?
> > > > > > > 
> > > > > > > Would we eventually remove the WebViewClient::create() overload
> then?
> > > > > > 
> > > > > > Well depends - to remove it would involve adding some class in
> > public/web
> > > > that
> > > > > > allowed callers to say "I don't care about callbacks" and then pass
> that
> > > > class
> > > > > > to create. To me this pattern is more naturally achieved using the
> > > overload.
> > > > > 
> > > > > Doesn't that mean we'd need to remove the one-argument overload
> eventually
> > > > > anyway? Is this just for convenience in finding those callsites as we
do
> > > this
> > > > > transition?
> > > > 
> > > > So my idea is to leave the overload and make it part of the public API.
> I'm
> > > not
> > > > sure if this goes against some prior art or convention, but seems the
most
> > > > obvious choice to me.
> > > > 
> > > > > 
> > > > > > 
> > > > > > The same could potentially be achieved by making the WebViewClient
> > dtor()
> > > > > > public, but that changes the ownership semantics of WebViewClient
when
> > > > > supplied
> > > > > > to WebView::create().
> > > > > 
> > > > > AFAIk, it's only protected so that an embedder is forced to declare a
> > > subclass
> > > > > rather than just reusing the default interface.
> > > > 
> > > > True, but generally you'd do that with a protected ctor() I guess. The
> > > protected
> > > > dtor() means that you can never call delete on the base class type so
you
> > > can't
> > > > transfer ownership. I'm not sure if that was the original implementors
> > > intention
> > > > tho.
> > > > 
> > > > Generally, if this CL gets in I do not plan on making future
modifications
> > to
> > > > the public API.
> > > 
> > > Also I'd like take the same approach with WebWidgetClient, although there
> are
> > > fewer cases of nullptr being passed in.
> > 
> > I think my preference is still towards making callers have an actual
> > WebViewClient instantiated. Presumably, the version that doesn't require a
> > client would have to use a static WebViewClient somewhere.
> 
> So do you want to see how things shake out and add a TODO to consider this
> later, or do you want me to try and add it now?

I'd prefer to see what it looks like now. That's what I did previously for
WebFrameClient:
https://chromium.googlesource.com/chromium/src/+/9f8e2f611e44753f7d8de5a2a671...

> 
> FWIW I'm not overly confident that it's possible to remove the null checks
> without providing a specialization of WebViewClient() or altering the current
> specializations of WebViewClient but time will tell. e.g.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...

[slangley, 2017-03-10 02:39:46]

Description was changed from

==========
Provide an overload to WebView::create that does not require a WebViewClient.

Provide an overload to WebView::create and WebViewImpl::create where a
that does not require WebViewClient or nullptr to be passed.

This is part of the pre-work to make it invalid to pass a nullptr to
WebView::create - it makes more sense for clients to be able to call a specific
method when they don't want to supply a client rathen than passing nullptr.

Also added DCHECK() to WebView*::create(WebViewClient*) to prevent nullptr being
passed, and went and cleaned up all of the call sites.

Followup work will create a internal specialization of WebViewClient that will
behave the way that a null WebViewClient works today.

BUG=696895
==========

to

==========
Provide an overload to WebView::create that does not require a WebViewClient.

Provide an overload to WebView::create and WebViewImpl::create where a
that does not require WebViewClient or nullptr to be passed.

This is part of the pre-work to make it invalid to pass a nullptr to
WebView::create - it makes more sense for clients to be able to call a specific
method when they don't want to supply a client rathen than passing nullptr.

Also added DCHECK() to WebView*::create(WebViewClient*) to prevent nullptr being
passed, and went and cleaned up all of the call sites.

Followup work will create a internal specialization of WebViewClient that will
behave the way that a null WebViewClient works today.

BUG=696895
COMMIT=false
==========

[slangley, 2017-03-10 02:40:44]

dcheng@ - so i added some code to WebViewImpl.cc to illustrate my idea. It
cannot be submitted in this state as it cases tests to fail in places, but you
get the idea.

WDYT of this approach?

[dcheng, 2017-03-10 02:53:28]

On 2017/03/10 02:40:44, slangley wrote:
> dcheng@ - so i added some code to WebViewImpl.cc to illustrate my idea. It
> cannot be submitted in this state as it cases tests to fail in places, but you
> get the idea.
> 
> WDYT of this approach?

Hmm... I think I'd rather just explicitly pass a client to use. It doesn't seem
like it's a big burden to do so here, especially given the small number of
callsites.

[slangley, 2017-03-10 03:01:22]

On 2017/03/10 at 02:53:28, dcheng wrote:
> On 2017/03/10 02:40:44, slangley wrote:
> > dcheng@ - so i added some code to WebViewImpl.cc to illustrate my idea. It
> > cannot be submitted in this state as it cases tests to fail in places, but
you
> > get the idea.
> > 
> > WDYT of this approach?
> 
> Hmm... I think I'd rather just explicitly pass a client to use. It doesn't
seem like it's a big burden to do so here, especially given the small number of
callsites.

OK - in that case do you expect local callers to derive their own WebViewClient,
or should we provide one in the public API, or should we make WebViewClient
instantiable and expect each callsite to manage the life cycle(note this would
require additional changes to existing derived WebViewClients)?

[dcheng, 2017-03-10 03:27:40]

On 2017/03/10 03:01:22, slangley wrote:
> On 2017/03/10 at 02:53:28, dcheng wrote:
> > On 2017/03/10 02:40:44, slangley wrote:
> > > dcheng@ - so i added some code to WebViewImpl.cc to illustrate my idea. It
> > > cannot be submitted in this state as it cases tests to fail in places, but
> you
> > > get the idea.
> > > 
> > > WDYT of this approach?
> > 
> > Hmm... I think I'd rather just explicitly pass a client to use. It doesn't
> seem like it's a big burden to do so here, especially given the small number
of
> callsites.
> 
> OK - in that case do you expect local callers to derive their own
WebViewClient,
> or should we provide one in the public API, or should we make WebViewClient
> instantiable and expect each callsite to manage the life cycle(note this would
> require additional changes to existing derived WebViewClients)?

They can just derive one themselves. It'll likely be a direct subclass with no
overrides most of the time, but I think that's OK.

[slangley, 2017-03-10 05:46:31]

On 2017/03/10 at 03:27:40, dcheng wrote:
> On 2017/03/10 03:01:22, slangley wrote:
> > On 2017/03/10 at 02:53:28, dcheng wrote:
> > > On 2017/03/10 02:40:44, slangley wrote:
> > > > dcheng@ - so i added some code to WebViewImpl.cc to illustrate my idea.
It
> > > > cannot be submitted in this state as it cases tests to fail in places,
but
> > you
> > > > get the idea.
> > > > 
> > > > WDYT of this approach?
> > > 
> > > Hmm... I think I'd rather just explicitly pass a client to use. It doesn't
> > seem like it's a big burden to do so here, especially given the small number
of
> > callsites.
> > 
> > OK - in that case do you expect local callers to derive their own
WebViewClient,
> > or should we provide one in the public API, or should we make WebViewClient
> > instantiable and expect each callsite to manage the life cycle(note this
would
> > require additional changes to existing derived WebViewClients)?
> 
> They can just derive one themselves. It'll likely be a direct subclass with no
overrides most of the time, but I think that's OK.

Ok I'll spin up a new CL for doing it that way and keep this one as is for now.