🏠 Close the bottom sheet when opening link in incognito

🏠 Close the bottom sheet when opening link in incognito

Opening links in the regular profile closes the bottom sheet,
so this makes the behaviour for incognito consistent with that
and fixes the issue of the sheet being open while the tab model
switches from regular to incognito.

BUG=690059
Review-Url: https://codereview.chromium.org/2710323003
Cr-Commit-Position: refs/heads/master@{#453221}
Committed: https://chromium.googlesource.com/chromium/src/+/7ac231eae0280b84411b27628bad2f6ec4ea17ba

[dgn, 2017-02-24 11:50:36]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 11:50:48]

Dry run: 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-02-24 12:26:39]

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

[commit-bot: I haz the power, 2017-02-24 12:26:39]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-02-24 13:04:04]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgn, 2017-02-24 13:04:05]

PTAL

[Bernhard Bauer, 2017-02-24 13:22:30]

bauerb@chromium.org changed reviewers:
+ mdjones@chromium.org

[Bernhard Bauer, 2017-02-24 13:22:31]

LGTM if Matt is happy.

[mdjones, 2017-02-24 18:03:38]

https://codereview.chromium.org/2710323003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/NativePageHost.java
(right):

https://codereview.chromium.org/2710323003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/NativePageHost.java:23: int
loadUrl(LoadUrlParams urlParams);
Can we instead add a boolean "loadAsIncognito" or something similar to this api?
It looks like you're pretty close to this already in BottomSheet.

I'm also a little concerned about supporting this api directly in Tab.java since
the caller of this function on Tab should never be able to control the incognito
state. For now we could overload this function to ignore the extra param, but I
would like to see something, maybe TabShim, that implements the NativePageHost
and is backed by a Tab. That way we can obscure these little details from Tab.
Thoughts?

[dgn, 2017-02-24 18:17:08]

https://codereview.chromium.org/2710323003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/NativePageHost.java
(right):

https://codereview.chromium.org/2710323003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/NativePageHost.java:23: int
loadUrl(LoadUrlParams urlParams);
On 2017/02/24 18:03:38, mdjones wrote:
> Can we instead add a boolean "loadAsIncognito" or something similar to this
api?
> It looks like you're pretty close to this already in BottomSheet.
> 

I thought about doing that but I'm not too excited about changing the 42 calls
to Tab#loadUrl.

> I'm also a little concerned about supporting this api directly in Tab.java
since
> the caller of this function on Tab should never be able to control the
incognito
> state. For now we could overload this function to ignore the extra param, but
I
> would like to see something, maybe TabShim, that implements the NativePageHost
> and is backed by a Tab. That way we can obscure these little details from Tab.
> Thoughts?

The TabShim idea sounds good yes, I'll send a patch for that.

[dgn, 2017-02-24 19:18:17]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[dgn, 2017-02-24 19:18:32]

PTAL

[commit-bot: I haz the power, 2017-02-24 19:18:41]

Dry run: 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-02-24 19:22:08]

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

[commit-bot: I haz the power, 2017-02-24 19:22:09]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgn, 2017-02-24 19:29:46]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 19:30:25]

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

[mdjones, 2017-02-24 20:13:14]

Awesome, thanks! lgtm!

https://codereview.chromium.org/2710323003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2710323003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:50:
//                activity, activity.getActivityTab(),
activity.getTabModelSelector());
nit: remove?

[commit-bot: I haz the power, 2017-02-24 20:28:20]

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

[commit-bot: I haz the power, 2017-02-24 20:28:20]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-02-27 15:14:04]

On 2017/02/24 20:13:14, mdjones wrote:
> Awesome, thanks! lgtm!
> 
>
https://codereview.chromium.org/2710323003/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
> (right):
> 
>
https://codereview.chromium.org/2710323003/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:50:
> //                activity, activity.getActivityTab(),
> activity.getTabModelSelector());
> nit: remove?

I copied the tab shim class here to make it the code work properly, but this
class is going away and is not being used currently. Patch to remove it:
https://codereview.chromium.org/2720693004/

[dgn, 2017-02-27 15:14:19]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-02-27 15:14:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2710323003/#ps60001
(title: "Properly make ContentSuggestionsActivity compile")

[commit-bot: I haz the power, 2017-02-27 15:14:35]

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-02-27 15:48:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488208459782880,
"parent_rev": "d6336797781587abf6c3fa648d034315714d1134", "commit_rev":
"7ac231eae0280b84411b27628bad2f6ec4ea17ba"}

[commit-bot: I haz the power, 2017-02-27 15:50:06]

Description was changed from

==========
🏠 Close the bottom sheet when opening link in incognito

Opening links in the regular profile closes the bottom sheet,
so this makes the behaviour for incognito consistent with that
and fixes the issue of the sheet being open while the tab model
switches from regular to incognito.

BUG=690059
==========

to

==========
🏠 Close the bottom sheet when opening link in incognito

Opening links in the regular profile closes the bottom sheet,
so this makes the behaviour for incognito consistent with that
and fixes the issue of the sheet being open while the tab model
switches from regular to incognito.

BUG=690059

Review-Url: https://codereview.chromium.org/2710323003
Cr-Commit-Position: refs/heads/master@{#453221}
Committed:
https://chromium.googlesource.com/chromium/src/+/7ac231eae0280b84411b27628bad...
==========

[commit-bot: I haz the power, 2017-02-27 15:50:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7ac231eae0280b84411b27628bad...