Add more checks to ShowPageActionPopup

Add more checks to ShowPageActionPopup

Add some debug checks to assist in tracking why page_action_view
can be NULL.

BUG=442521
Committed: https://crrev.com/0b446d878107a56dbb9ba738e9ab414261c66ac0
Cr-Commit-Position: refs/heads/master@{#309294}

[danduong, 2014-12-19 17:30:13]

danduong@chromium.org changed reviewers:
+ pkasting@chromium.org, wittman@chromium.org

[Mike Wittman, 2014-12-19 19:13:36]

https://codereview.chromium.org/819623003/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/819623003/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1319:
CHECK(GetWebContents());
It would be better to check here that WebContents was not null during the last
call to RefreshPageActionViews. It's entirely possible it could have been null
then but non-null here.

[danduong, 2014-12-19 19:15:08]

https://codereview.chromium.org/819623003/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/819623003/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1319:
CHECK(GetWebContents());
On 2014/12/19 19:13:36, Mike Wittman wrote:
> It would be better to check here that WebContents was not null during the last
> call to RefreshPageActionViews. It's entirely possible it could have been null
> then but non-null here.

I'll probably add both checks.

[danduong, 2014-12-19 19:23:51]

I changed my mind. I'll just check the debug state variable.

[Mike Wittman, 2014-12-19 19:33:14]

https://codereview.chromium.org/819623003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/819623003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:188:
web_contents_null_while_refresh_(false) {
How about web_contents_not_null_at_last_refresh?

As is, we would not assert on calls to GetPageActionView with no prior call to
RefreshPageActionViews.

[danduong, 2014-12-19 19:38:11]

https://codereview.chromium.org/819623003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/819623003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:188:
web_contents_null_while_refresh_(false) {
On 2014/12/19 19:33:14, Mike Wittman wrote:
> How about web_contents_not_null_at_last_refresh?
> 
> As is, we would not assert on calls to GetPageActionView with no prior call to
> RefreshPageActionViews.

Done.

[Peter Kasting, 2014-12-19 22:14:12]

LGTM

https://codereview.chromium.org/819623003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/819623003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1082:
web_contents_not_null_at_last_refresh_ = web_contents != NULL;
Super-tiny nit, but I'd change "not_null" to "null" here and then add a !
operator at the lone use below -- that seems fractionally easier to mentally
parse, to me, somehow.

[danduong, 2014-12-19 22:27:46]

The CQ bit was checked by danduong@chromium.org

[commit-bot: I haz the power, 2014-12-19 22:29:09]

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

[commit-bot: I haz the power, 2014-12-19 23:38:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-19 23:40:18]

Patchset 4 (id:??) landed as
https://crrev.com/0b446d878107a56dbb9ba738e9ab414261c66ac0
Cr-Commit-Position: refs/heads/master@{#309294}