ForeignSessionHelper Changes Needed for NTP Other Devices.

* Fixed out-of-bounds crash bug in foreign_session_helper.cc.
* Now skips windows if all their tabs are empty.
* Now skips sessions if all their windows are empty.
* Method to query if foreign session are collapsed or expanded.
* Returns Java timestamps (previously returned Windows timestamps)

BUG=247382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224787

[newt (away), 2013-09-09 17:24:09]

how do we want to handle the case where a synced tab has an invalid nav index?
 1. Show the tab but adjust the current nav index to be valid, or
 2. Don't show the tab at all?

If we want 1), then this lgtm, though perhaps 2) might be better. What do you
think?

[apiccion, 2013-09-12 00:18:37]

@newt
ATM I think we want it to skip the tab. Not sure tho.

[newt (away), 2013-09-20 17:15:02]

lgtm, thanks! and you'll need OWNERs approval too.

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
File chrome/browser/android/foreign_session_helper.cc (right):

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:47: return
(url.SchemeIs(chrome::kChromeNativeScheme) ||
do we want to show other chrome:// pages? chrome://bookmarks or
chrome://history, for example? I'd expect these to be hidden as well.

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:102: int selected_index =
tab.current_navigation_index;
you just verified in shouldSkipTab(), that the current_nav_index is valid. no
need to check again.

[apiccion, 2013-09-20 20:47:57]

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
File chrome/browser/android/foreign_session_helper.cc (right):

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:47: return
(url.SchemeIs(chrome::kChromeNativeScheme) ||
On 2013/09/20 17:15:03, newt wrote:
> do we want to show other chrome:// pages? chrome://bookmarks or
> chrome://history, for example? I'd expect these to be hidden as well.

I can confirm these aren't being shown.

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:102: int selected_index =
tab.current_navigation_index;
On 2013/09/20 17:15:03, newt wrote:
> you just verified in shouldSkipTab(), that the current_nav_index is valid. no
> need to check again.

IS it okay if I put a DCHECK? I'd hate if we changed shouldSkipTab and somehow
forgot to re-add the check here.

[newt (away), 2013-09-20 21:14:35]

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
File chrome/browser/android/foreign_session_helper.cc (right):

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:47: return
(url.SchemeIs(chrome::kChromeNativeScheme) ||
On 2013/09/20 20:47:57, apiccion wrote:
> On 2013/09/20 17:15:03, newt wrote:
> > do we want to show other chrome:// pages? chrome://bookmarks or
> > chrome://history, for example? I'd expect these to be hidden as well.
> 
> I can confirm these aren't being shown.
> 

why not? are they not being synced in the first place? if so, we should also
prevent chrome-native URLs from being synced. then they won't show up on any
platform. currently chrome-native:// URLs show up under other devices on Linux,
for example.

https://codereview.chromium.org/23514039/diff/16001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:102: int selected_index =
tab.current_navigation_index;
On 2013/09/20 20:47:57, apiccion wrote:
> On 2013/09/20 17:15:03, newt wrote:
> > you just verified in shouldSkipTab(), that the current_nav_index is valid.
no
> > need to check again.
> 
> IS it okay if I put a DCHECK? I'd hate if we changed shouldSkipTab and somehow
> forgot to re-add the check here.

sure.

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java
(right):

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:201:
* @return          {@code True} if the session is collapsed, false if expanded.
nit: spacing

[apiccion, 2013-09-20 22:15:21]

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java
(right):

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:201:
* @return          {@code True} if the session is collapsed, false if expanded.
On 2013/09/20 21:14:35, newt wrote:
> nit: spacing

Done.

[Yaron, 2013-09-20 22:18:08]

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java
(right):

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:40:
// Please keep in sync with sync_session.h
We have a way of ensuring this. See usage of build/android/java_cpp_template. If
you don't do that, you at least need to add the comment there as it's more
likely to change.

Do you need this enum though? Could you instead return up a resource that's
computed the same way it already was for native? I'm not sure how this is used..

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
File chrome/browser/android/foreign_session_helper.cc (right):

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:46: bool shouldSkipUrl(const
GURL& url) {
All of these should start with Caps.

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:71: bool shouldSkipWindow(const
SessionWindow* window) {
use a const&

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:81: bool
shouldSkipSession(const browser_sync::SyncedSession* session) {
use a const&

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:103: DCHECK(selected_index >= 0
Split to 2 DCHECKs (otherwise you can't tell which failed)

[apiccion, 2013-09-20 23:02:56]

@rlarocque: Please have a look at synced_session.h

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java
(right):

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:40:
// Please keep in sync with sync_session.h
On 2013/09/20 22:18:08, Yaron wrote:
> We have a way of ensuring this. See usage of build/android/java_cpp_template.
If
> you don't do that, you at least need to add the comment there as it's more
> likely to change.
> 
> Do you need this enum though? Could you instead return up a resource that's
> computed the same way it already was for native? I'm not sure how this is
used..

Using this to determine what icon to display (laptop or phone) for next to the
foreign session device name on the other devices page. The template says it
works for (preprocessor (e.g. #define)) but the constants I want are enums. I
added the comment to synced_session.h

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
File chrome/browser/android/foreign_session_helper.cc (right):

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:46: bool shouldSkipUrl(const
GURL& url) {
On 2013/09/20 22:18:08, Yaron wrote:
> All of these should start with Caps.

Done.

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:71: bool shouldSkipWindow(const
SessionWindow* window) {
On 2013/09/20 22:18:08, Yaron wrote:
> use a const&

Done.

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:81: bool
shouldSkipSession(const browser_sync::SyncedSession* session) {
On 2013/09/20 22:18:08, Yaron wrote:
> use a const&

Done.

https://codereview.chromium.org/23514039/diff/21001/chrome/browser/android/fo...
chrome/browser/android/foreign_session_helper.cc:103: DCHECK(selected_index >= 0
On 2013/09/20 22:18:08, Yaron wrote:
> Split to 2 DCHECKs (otherwise you can't tell which failed)

Done.

[Yaron, 2013-09-20 23:07:13]

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java
(right):

https://codereview.chromium.org/23514039/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:40:
// Please keep in sync with sync_session.h
On 2013/09/20 23:02:57, apiccion wrote:
> On 2013/09/20 22:18:08, Yaron wrote:
> > We have a way of ensuring this. See usage of
build/android/java_cpp_template.
> If
> > you don't do that, you at least need to add the comment there as it's more
> > likely to change.
> > 
> > Do you need this enum though? Could you instead return up a resource that's
> > computed the same way it already was for native? I'm not sure how this is
> used..
> 
> Using this to determine what icon to display (laptop or phone) for next to the
> foreign session device name on the other devices page. The template says it
> works for (preprocessor (e.g. #define)) but the constants I want are enums. I
> added the comment to synced_session.h

Right, so we already have those assets in chromium for the open tabs webui page.
Could you return the resource instead of having to map the ids? At a minimum, we
should reuse the resources if possible unless there are scaling issues with
using the existing resources on android

[rlarocque, 2013-09-20 23:07:56]

sync_session.h LGTM, since the change is trivial.

If you ever need a more thorough review of sessions code, you should talk to
zea@.

[Yaron, 2013-09-20 23:17:38]

lgtm

[commit-bot: I haz the power, 2013-09-20 23:21:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/23514039/33001

[commit-bot: I haz the power, 2013-09-21 00:03:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-23 18:05:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/23514039/33001

[commit-bot: I haz the power, 2013-09-23 20:59:12]

Change committed as 224787