Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG=613359
Committed: https://crrev.com/261c816b0900eec6c3b41277288ccd4020c23f5d
Cr-Commit-Position: refs/heads/master@{#395422}

[dewittj, 2016-05-20 16:34:37]

Description was changed from

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

BUG=613359
==========

to

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

BUG=613359
==========

[dewittj, 2016-05-20 16:34:37]

dewittj@chromium.org changed reviewers:
+ fgorski@chromium.org, tedchoc@chromium.org

[dewittj, 2016-05-20 16:35:34]

Description was changed from

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

BUG=613359
==========

to

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG=613359
==========

[dewittj, 2016-05-20 16:47:36]

ptal, thanks.

[Ted C, 2016-05-20 16:55:59]

https://codereview.chromium.org/2004473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java
(right):

https://codereview.chromium.org/2004473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174:
if (bridge == null) {
this seems too late to be checking this.  A user should never be able to get to
here if the bridge is null.  Shouldn't this be checked somewhere earlier in the
UI code to prevent it from showing?

Or is this a race?

[fgorski, 2016-05-20 17:03:56]

https://codereview.chromium.org/2004473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java
(right):

https://codereview.chromium.org/2004473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174:
if (bridge == null) {
On 2016/05/20 16:55:59, Ted C wrote:
> this seems too late to be checking this.  A user should never be able to get
to
> here if the bridge is null.  Shouldn't this be checked somewhere earlier in
the
> UI code to prevent it from showing?
> 
> Or is this a race?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

We should ensure the menu item is not visible, I think.

[dewittj, 2016-05-20 18:33:21]

https://codereview.chromium.org/2004473002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java
(right):

https://codereview.chromium.org/2004473002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174:
if (bridge == null) {
On 2016/05/20 17:03:56, fgorski wrote:
> On 2016/05/20 16:55:59, Ted C wrote:
> > this seems too late to be checking this.  A user should never be able to get
> to
> > here if the bridge is null.  Shouldn't this be checked somewhere earlier in
> the
> > UI code to prevent it from showing?
> > 
> > Or is this a race?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> We should ensure the menu item is not visible, I think.

I'm planning to merge this patch, so updates to the context menu delegate seem
out of scope (especially since it's behind a flag right now).  Can fix this
immediately after landing this patch.

Also there might indeed be a race, since there could be a lot of time between
showing the context menu and actually executing the click action.  I'd leave the
null check here even after fixing the ChromeContextMenuItemPopulator.

WDYT?

[fgorski, 2016-05-23 20:15:38]

lgtm with nit based on the discussion we had earlier today.

https://codereview.chromium.org/2004473002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java
(right):

https://codereview.chromium.org/2004473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:173:
// If the tab is incognito we may get a null offline page bridge.
nit: Add a TODO for moving to a proper check for offline pages being enabled.

[dewittj, 2016-05-23 20:23:23]

dewittj@chromium.org changed reviewers:
- tedchoc@chromium.org

[dewittj, 2016-05-23 20:24:39]

-tedchoc to cc

Filip, I removed the tab changes since they are behind a flag anyway.  I added
crbug.com/614138 to track proper usage of enabled checks on that UI.

WDYT?

[Ted C, 2016-05-23 20:25:14]

On 2016/05/23 20:15:38, fgorski wrote:
> lgtm with nit based on the discussion we had earlier today.
> 
>
https://codereview.chromium.org/2004473002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java
> (right):
> 
>
https://codereview.chromium.org/2004473002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:173:
> // If the tab is incognito we may get a null offline page bridge.
> nit: Add a TODO for moving to a proper check for offline pages being enabled.

also seems reasonable to me, lgtm

[fgorski, 2016-05-23 20:32:49]

still lgtm

[dewittj, 2016-05-23 20:35:22]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2016-05-23 20:36:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2004473002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2004473002/40001

[commit-bot: I haz the power, 2016-05-23 21:44:35]

Description was changed from

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG=613359
==========

to

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG=613359
==========

[commit-bot: I haz the power, 2016-05-23 21:44:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-23 21:47:15]

Description was changed from

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG=613359
==========

to

==========
Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG=613359

Committed: https://crrev.com/261c816b0900eec6c3b41277288ccd4020c23f5d
Cr-Commit-Position: refs/heads/master@{#395422}
==========

[commit-bot: I haz the power, 2016-05-23 21:47:16]

Patchset 3 (id:??) landed as
https://crrev.com/261c816b0900eec6c3b41277288ccd4020c23f5d
Cr-Commit-Position: refs/heads/master@{#395422}