Audit use of m_client in WebFrameWidgetImpl and remove null checks.

Audit use of m_client in WebFrameWidgetImpl and remove null checks.

Now that a valid WebWidgetClient must always be passed when creating a
WebFrameWidget we can remove some of the redundant m_client == nullptr checks.

m_client can now become nullptr if close() is called on the WebFrameWidget,
but this is generally caught by checking m_layerTreeView.

To be safe if(m_client) checks have been replaced with DCHECK(m_client).

BUG=696895
Review-Url: https://codereview.chromium.org/2775113002
Cr-Commit-Position: refs/heads/master@{#460612}
Committed: https://chromium.googlesource.com/chromium/src/+/943648dbbf5a074d645d71c8108c0caedff7967a

[slangley, 2017-03-26 23:21:52]

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

[sashab, 2017-03-27 03:03:47]

Don't you hate those people who take forever to do reviews... LGTM

Btw should we make a note somewhere that we are doing this? What if this causes
crashes in the wild? Not sure what the proper process is.

[sashab, 2017-03-27 03:05:02]

Suggestion, could be wrong: maybe you want to add the DCHECKs but leave the
if-statements in, so that the failures get reported, but there's no crash
caused. Then after a few weeks or something if there's no reported failures then
you can get rid of the if-statements too.

[slangley, 2017-03-27 03:06:30]

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

[slangley, 2017-03-27 03:06:31]

dcheng@ - what's the best way to roll out a change like this? also, can you
ptal.

Thanks

[dcheng, 2017-03-27 04:30:27]

Commit it and see if it crashes =)

(If you want to be conservative, you can use CHECK() initially--that way, any
crash reports you get will be more useful. But I think DCHECK() is good
enough--it's pretty obvious when a null pointer is derefed)

LGTM

[dcheng, 2017-03-27 04:33:24]

https://codereview.chromium.org/2775113002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2775113002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:342: DCHECK(m_client);
(Also, I'm not sure where this DCHECK comes from; it looks like most of the
other ones are things that used to be checked as an if(). Is this just out of
caution?)

https://codereview.chromium.org/2775113002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:834: DCHECK(m_client);
Ditto.

[slangley, 2017-03-27 04:37:42]

On 2017/03/27 at 04:33:24, dcheng wrote:
>
https://codereview.chromium.org/2775113002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):
> 
>
https://codereview.chromium.org/2775113002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:342: DCHECK(m_client);
> (Also, I'm not sure where this DCHECK comes from; it looks like most of the
other ones are things that used to be checked as an if(). Is this just out of
caution?)
> 
>
https://codereview.chromium.org/2775113002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:834: DCHECK(m_client);
> Ditto.

Yes I added some additional DCHECK() statements where m_client is used for some
extra caution, no other reason.

[dcheng, 2017-03-27 04:37:45]

On 2017/03/27 03:05:02, sashab wrote:
> Suggestion, could be wrong: maybe you want to add the DCHECKs but leave the
> if-statements in, so that the failures get reported, but there's no crash
> caused. Then after a few weeks or something if there's no reported failures
then
> you can get rid of the if-statements too.

(Sorry I missed this message at first)

DCHECK failures aren't noticed nearly enough, unfortunately, so just DCHECKing
and then using an if () is probably going to mean that things will get missed.
It also goes against the recommendations of how to use Chrome logging:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

(specifically, DCHECK failures should not be handled in any way)

[slangley, 2017-03-29 23:10:41]

The CQ bit was checked by slangley@chromium.org

[commit-bot: I haz the power, 2017-03-29 23:11:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 01:09:18]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490829041934330, "parent_rev":
"b554454239ebb239fc7da684b70b6be8ae860c8b", "commit_rev":
"943648dbbf5a074d645d71c8108c0caedff7967a"}

[commit-bot: I haz the power, 2017-03-30 01:10:02]

Description was changed from

==========
Audit use of m_client in WebFrameWidgetImpl and remove null checks.

Now that a valid WebWidgetClient must always be passed when creating a
WebFrameWidget we can remove some of the redundant m_client == nullptr checks.

m_client can now become nullptr if close() is called on the WebFrameWidget,
but this is generally caught by checking m_layerTreeView.

To be safe if(m_client) checks have been replaced with DCHECK(m_client).

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

to

==========
Audit use of m_client in WebFrameWidgetImpl and remove null checks.

Now that a valid WebWidgetClient must always be passed when creating a
WebFrameWidget we can remove some of the redundant m_client == nullptr checks.

m_client can now become nullptr if close() is called on the WebFrameWidget,
but this is generally caught by checking m_layerTreeView.

To be safe if(m_client) checks have been replaced with DCHECK(m_client).

BUG=696895

Review-Url: https://codereview.chromium.org/2775113002
Cr-Commit-Position: refs/heads/master@{#460612}
Committed:
https://chromium.googlesource.com/chromium/src/+/943648dbbf5a074d645d71c8108c...
==========

[commit-bot: I haz the power, 2017-03-30 01:10:03]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/943648dbbf5a074d645d71c8108c...