Do NULL checks on TabAndroids retrieved from the model

Do NULL checks on TabAndroids retrieved from the model

There can be TabModels which know about Tabs that are not
alive and cannot yet be instantiated.  Do NULL checks before
checking the Tabs.

BUG=403592
R=dgozman@chromium.org, dtrainor@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/893a2b4298efec20e4d6f74fe524ba64cae01783

[dgozman, 2014-09-05 05:33:02]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2014-09-05 05:33:03]

https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
File chrome/browser/android/dev_tools_server.cc (right):

https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
chrome/browser/android/dev_tools_server.cc:191: if (!tab)
I wonder whether DevTools is the only place which calls GetTabAt() and expects a
Tab instead of NULL. Sounds like a strange container API.

[gone, 2014-09-05 08:02:16]

On 2014/09/05 05:33:03, dgozman wrote:
>
https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
> File chrome/browser/android/dev_tools_server.cc (right):
> 
>
https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
> chrome/browser/android/dev_tools_server.cc:191: if (!tab)
> I wonder whether DevTools is the only place which calls GetTabAt() and expects
a
> Tab instead of NULL. Sounds like a strange container API.

One of the consequences of (1) the downstream ChromeTab being so tightly coupled
to the ChromeActivity, and (2) the way Tabs are launched in new Activities on L.
 Will ask dtrainor@ to weigh in.  FWIW, we have plans to properly separate the
classes more cleanly once the dust settles, but given that this is an M38
RBB/RBS series of bugs, we've been hesitant to do anything too crazy.

[David Trainor- moved to gerrit, 2014-09-05 17:05:33]

On 2014/09/05 08:02:16, dfalcantara wrote:
> On 2014/09/05 05:33:03, dgozman wrote:
> >
>
https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
> > File chrome/browser/android/dev_tools_server.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
> > chrome/browser/android/dev_tools_server.cc:191: if (!tab)
> > I wonder whether DevTools is the only place which calls GetTabAt() and
expects
> a
> > Tab instead of NULL. Sounds like a strange container API.
> 
> One of the consequences of (1) the downstream ChromeTab being so tightly
coupled
> to the ChromeActivity, and (2) the way Tabs are launched in new Activities on
L.
>  Will ask dtrainor@ to weigh in.  FWIW, we have plans to properly separate the
> classes more cleanly once the dust settles, but given that this is an M38
> RBB/RBS series of bugs, we've been hesitant to do anything too crazy.

Right now even the base class Tab requires a Context to be built.  The Android
WebContents needs a ContentViewCore and vice versa, so without an Activity we
can't build the ContentViewCore that it talks to.  It looks like this is causing
a problem with the new way we're building the tab list.  I think we made the
assumption that it was okay that querying tabs from the model might return null,
but they still had to be there for sync purposes.

That's a good point though that I don't know if we null check everywhere we call
GetTabAt() now that we've changed the assumptions.

[David Trainor- moved to gerrit, 2014-09-05 17:10:06]

On 2014/09/05 17:05:33, David Trainor wrote:
> On 2014/09/05 08:02:16, dfalcantara wrote:
> > On 2014/09/05 05:33:03, dgozman wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
> > > File chrome/browser/android/dev_tools_server.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/542953002/diff/20001/chrome/browser/an...
> > > chrome/browser/android/dev_tools_server.cc:191: if (!tab)
> > > I wonder whether DevTools is the only place which calls GetTabAt() and
> expects
> > a
> > > Tab instead of NULL. Sounds like a strange container API.
> > 
> > One of the consequences of (1) the downstream ChromeTab being so tightly
> coupled
> > to the ChromeActivity, and (2) the way Tabs are launched in new Activities
on
> L.
> >  Will ask dtrainor@ to weigh in.  FWIW, we have plans to properly separate
the
> > classes more cleanly once the dust settles, but given that this is an M38
> > RBB/RBS series of bugs, we've been hesitant to do anything too crazy.
> 
> Right now even the base class Tab requires a Context to be built.  The Android
> WebContents needs a ContentViewCore and vice versa, so without an Activity we
> can't build the ContentViewCore that it talks to.  It looks like this is
causing
> a problem with the new way we're building the tab list.  I think we made the
> assumption that it was okay that querying tabs from the model might return
null,
> but they still had to be there for sync purposes.
> 
> That's a good point though that I don't know if we null check everywhere we
call
> GetTabAt() now that we've changed the assumptions.

FWIW, given the current time pressure I would support adding null checks for
M-38 and then investigate whether or not it makes sense to support Activity-less
Tabs actually existing and having content in 39/40.

[dgozman, 2014-09-05 18:02:11]

lgtm as a temporary solution against crashes. Note that you also need
c/b/android owner.

[David Trainor- moved to gerrit, 2014-09-05 18:12:19]

dtrainor@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2014-09-05 18:12:19]

it looks like the other cases you already took care of dan.  lgtm.

[gone, 2014-09-05 18:12:57]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2014-09-05 18:14:13]

The CQ bit was unchecked by dfalcantara@chromium.org

[gone, 2014-09-05 18:15:34]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:17:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/542953002/40001

[commit-bot: I haz the power, 2014-09-05 19:11:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 19:11:15]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[gone, 2014-09-05 20:09:02]

Findbugs have been broken for a while, and not by this.  Landing manually since
it's an Android only change.

[gone, 2014-09-05 20:34:27]

Committed patchset #3 (id:40001) manually as 893a2b4 (presubmit successful).

[commit-bot: I haz the power, 2014-09-10 03:41:22]

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