Report initial CustomTab navigation with client package name to DataUseTabUIManager

Report initial CustomTab navigation with client package name to DataUseTabUIManager.

This will be used by the DataUseTabUIManager to know when a navigation started from
a client.

BUG=551196
Committed: https://crrev.com/e67e32fc9412388b2c20027943d830c12284857c
Cr-Commit-Position: refs/heads/master@{#358766}

[megjablon, 2015-11-04 01:17:38]

Description was changed from

==========
TODO tomorrow

BUG=
==========

to

==========
Report initial CustomTab navigation with client package name to
DataUseTabUIManager.

This will be used by the DataUseTabUIManager to know when a navigation
started from a client.

BUG=551196
==========

[megjablon, 2015-11-04 01:17:57]

Description was changed from

==========
Report initial CustomTab navigation with client package name to
DataUseTabUIManager.

This will be used by the DataUseTabUIManager to know when a navigation
started from a client.

BUG=551196
==========

to

==========
Report initial CustomTab navigation with client package name to
DataUseTabUIManager.

This will be used by the DataUseTabUIManager to know when a navigation started
from
a client.

BUG=551196
==========

[megjablon, 2015-11-04 01:18:24]

megjablon@chromium.org changed reviewers:
+ newt@chromium.org

[megjablon, 2015-11-04 01:18:24]

PTAL, thanks!

[bengr, 2015-11-05 23:50:24]

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

[bengr, 2015-11-05 23:50:24]

https://codereview.chromium.org/1407063006/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc
(right):

https://codereview.chromium.org/1407063006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc:34: void
CustomTabIsLoadingURL(JNIEnv* env,
Coordinate with tbansal on the name, but it should probably be something like
OnCustomTabInitialNavigation(). I'd probably add the url too.

[megjablon, 2015-11-06 01:30:29]

Patchset #3 (id:40001) has been deleted

[megjablon, 2015-11-06 01:30:42]

megjablon@chromium.org changed reviewers:
+ tbansal@chromium.org

[megjablon, 2015-11-06 01:30:43]

https://codereview.chromium.org/1407063006/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc
(right):

https://codereview.chromium.org/1407063006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc:34: void
CustomTabIsLoadingURL(JNIEnv* env,
On 2015/11/05 23:50:24, bengr wrote:
> Coordinate with tbansal on the name, but it should probably be something like
> OnCustomTabInitialNavigation(). I'd probably add the url too.

Renamed. @tbansal you good with this name? What do we need the url for?

[tbansal1, 2015-11-06 01:36:22]

https://codereview.chromium.org/1407063006/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc
(right):

https://codereview.chromium.org/1407063006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc:34: void
CustomTabIsLoadingURL(JNIEnv* env,
On 2015/11/06 01:30:43, megjablon wrote:
> On 2015/11/05 23:50:24, bengr wrote:
> > Coordinate with tbansal on the name, but it should probably be something
like
> > OnCustomTabInitialNavigation(). I'd probably add the url too.
> 
> Renamed. @tbansal you good with this name? What do we need the url for? 

Name sgtm. url is needed because combination of package name and 
url will determine whether the tab is covered by the matching rules
or not. That will in turn decide which UI element to show.

[newt (away), 2015-11-06 19:17:37]

newt@chromium.org changed reviewers:
+ yusufo@chromium.org

[newt (away), 2015-11-06 19:17:37]

Yusuf: could you take a look at my comment in CustomTabActivity?

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:329:
void loadUrlInCurrentTab(LoadUrlParams params, long timeStamp) {
Seems like this method could be called multiple times. I think you probably need
a way of ensuring that you only call onCustomTabInitialNavigation() once.

Adding Yusuf to comment on this.

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
(right):

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:60:
* @param packageName the client package for the custom tab loading a url.
s/client/client app/  right?

[Yusuf, 2015-11-06 19:22:53]

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:329:
void loadUrlInCurrentTab(LoadUrlParams params, long timeStamp) {
On 2015/11/06 19:17:37, newt wrote:
> Seems like this method could be called multiple times. I think you probably
need
> a way of ensuring that you only call onCustomTabInitialNavigation() once.
> 
> Adding Yusuf to comment on this.

Yes. That is right. This can be called also through another url sent to the same
tab from the client side and also for window.open() cases which currently loads
the url in the same tab.

I would recommend putting this to finishNativeInitialization close to where the
first loadUrlInCurrentTab is.

[megjablon, 2015-11-06 21:11:23]

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:329:
void loadUrlInCurrentTab(LoadUrlParams params, long timeStamp) {
On 2015/11/06 19:22:53, Yusuf wrote:
> On 2015/11/06 19:17:37, newt wrote:
> > Seems like this method could be called multiple times. I think you probably
> need
> > a way of ensuring that you only call onCustomTabInitialNavigation() once.
> > 
> > Adding Yusuf to comment on this.
> 
> Yes. That is right. This can be called also through another url sent to the
same
> tab from the client side and also for window.open() cases which currently
loads
> the url in the same tab.
> 
> I would recommend putting this to finishNativeInitialization close to where
the
> first loadUrlInCurrentTab is.

Awesome! Thanks for catching this!

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
(right):

https://codereview.chromium.org/1407063006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:60:
* @param packageName the client package for the custom tab loading a url.
On 2015/11/06 19:17:37, newt wrote:
> s/client/client app/  right?

Done.

[newt (away), 2015-11-07 00:37:09]

lgtm after method naming comment is addressed

https://codereview.chromium.org/1407063006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
(right):

https://codereview.chromium.org/1407063006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:38:
public static boolean hasDataUseTrackingStarted(Tab tab) {
So this is not a simple getter, i.e. it actually modifies internal state? If so,
let's give this a name that makes that clear, so someone doesn't accidentally
write code assuming that the value returned here won't change. Perhaps:
"checkDataUseTrackingStarted()".

Or does this return true as many times as it's called *until* the next page
load. In that case, I still think the name could be clearer. Perhaps,
"dataUseTrackingStartedThisNavigation()", though that's a mouthful.

[megjablon, 2015-11-07 02:03:06]

https://chromiumcodereview.appspot.com/1407063006/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
(right):

https://chromiumcodereview.appspot.com/1407063006/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:38:
public static boolean hasDataUseTrackingStarted(Tab tab) {
On 2015/11/07 00:37:09, newt wrote:
> So this is not a simple getter, i.e. it actually modifies internal state? If
so,
> let's give this a name that makes that clear, so someone doesn't accidentally
> write code assuming that the value returned here won't change. Perhaps:
> "checkDataUseTrackingStarted()".
> 
> Or does this return true as many times as it's called *until* the next page
> load. In that case, I still think the name could be clearer. Perhaps,
> "dataUseTrackingStartedThisNavigation()", though that's a mouthful.

Nope the former. I changed to checkDataUseTrackingStarted. I like that better.

[bengr, 2015-11-09 15:43:19]

lgtm

[megjablon, 2015-11-10 02:18:20]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-11-10 02:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
bengr@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1407063006/#ps160001 (title: "rebase")

[commit-bot: I haz the power, 2015-11-10 02:20:58]

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

[commit-bot: I haz the power, 2015-11-10 02:53:20]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-11-10 02:54:16]

Patchset 8 (id:??) landed as
https://crrev.com/e67e32fc9412388b2c20027943d830c12284857c
Cr-Commit-Position: refs/heads/master@{#358766}