Adding CrHome-specific implementation for home page tile.

Adding CrHome-specific implementation for home page tile.

This CL adds an HomePageClient for Android that is only set if the
kChromeHome feature is enabled because the normal NTP has a working
home button.

If either kChromeHome or kPinHomePageAsTileFeature is not enabled,
the MostVisited tiles will not contain the home tile.

Special case: The home tile can be removed as every tile and will be
blacklisted then.
By explicitly enabling (or reenabling) the Homepage, the blacklisting
will be undone and the tile will reappear as first tile.

Removing the home tile does not disable the Homepage setting (because
this setting also controls the start-up page and the home tile is not
visually linked to the home page .... besides having the same URL).

BUG=703994
Review-Url: https://codereview.chromium.org/2897293002
Cr-Commit-Position: refs/heads/master@{#476258}
Committed: https://chromium.googlesource.com/chromium/src/+/3fc1f818eaab417801a21886ce9c293701b10486

[fhorschig, 2017-05-24 15:47:57]

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

[fhorschig, 2017-05-24 15:47:58]

Hi Nicholas,
would you please take a look at the Android and browser
part of this CL pinning the home page as first tile in
CrHome?
(Feel free to check the rest as well. I intend to ask
mastiz@ for that separately.)

[fhorschig, 2017-05-24 15:51:50]

fhorschig@chromium.org changed reviewers:
+ mastiz@chromium.org

[fhorschig, 2017-05-24 15:51:51]

Hi Mikel,
this is the CL with the dependency I was talking about.

I used to try to inject the Client implementation into
the constructor using the factory but this turned out
to be very hard to do as the implementation needs to
call the Java part of Android.

Therefore, I decided to go for this minimal solution
that sets the (optional) dependency with a setter 
function. 

Do you think this is appropriate or do you know a more
elegant solution?

[dgn, 2017-05-25 10:33:33]

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java
(right):

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java:46:
interface Client {
nit: why not HomePageClient like in c++ ?

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java:48:
* @return Returns whether a home page is active and set.
Don't add "Returns" at the beginning of a @return text. The javadoc parser
already adds that. If you're using Android Studio or Eclipse for Java you can
preview the javadoc right there.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
(right):

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:7:
import static org.chromium.base.ContextUtils.getApplicationContext;
just use a regular import here, and call ContextUtils.getApplicationContext.

For tests static imports are fine to make things less verbose, but it just looks
confusing in production code.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:33:
HomepageManager.getInstance(getApplicationContext()).addListener(this);
when should the listener be removed? destroy() ?

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:92:
if (mNativeMostVisitedSitesBridge == 0) {
can it happen in the normal functioning of the class? just assert otherwise?
That's what we do in the other functions.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:95:
// Ensure the home page is set as first tile if (re-)enabled - even if
blacklisted before.
how does that set the tile as first? to me it only looks like it just removes it
from the backlist. Isn't the order implementation detail thus not useful as a
comment here?

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:110:
return HomepageManager.getHomepageUri(getApplicationContext())
compare strings with TextUtils.equals(s1, s2), the home page URI being null here
would cause a crash. Also, why not just use getHomePageUrl here?

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:115:
public String getHomePageUrl() {
this can return null, mark @Nullable? also, does the backend properly handle
that?

[mastiz, 2017-05-26 08:00:54]

On 2017/05/24 15:51:51, fhorschig wrote:
> Hi Mikel,
> this is the CL with the dependency I was talking about.
> 
> I used to try to inject the Client implementation into
> the constructor using the factory but this turned out
> to be very hard to do as the implementation needs to
> call the Java part of Android.
> 
> Therefore, I decided to go for this minimal solution
> that sets the (optional) dependency with a setter 
> function. 
> 
> Do you think this is appropriate or do you know a more
> elegant solution?

I don't think I have a more elegant solution, and I like simple :-)

[fhorschig, 2017-05-30 10:24:16]

Patchset #2 (id:20001) has been deleted

[fhorschig, 2017-05-30 10:25:22]

I added a test to verify that null home pages are handled.
(And hidden)

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java
(right):

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java:46:
interface Client {
On 2017/05/25 10:33:33, dgn wrote:
> nit: why not HomePageClient like in c++ ?

Done.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java:48:
* @return Returns whether a home page is active and set.
On 2017/05/25 10:33:32, dgn wrote:
> Don't add "Returns" at the beginning of a @return text. The javadoc parser
> already adds that. If you're using Android Studio or Eclipse for Java you can
> preview the javadoc right there.

Thanks! (3x Done.)

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
(right):

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:7:
import static org.chromium.base.ContextUtils.getApplicationContext;
On 2017/05/25 10:33:33, dgn wrote:
> just use a regular import here, and call ContextUtils.getApplicationContext.
> 
> For tests static imports are fine to make things less verbose, but it just
looks
> confusing in production code.

Done.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:33:
HomepageManager.getInstance(getApplicationContext()).addListener(this);
On 2017/05/25 10:33:33, dgn wrote:
> when should the listener be removed? destroy() ?

Yes. Done.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:92:
if (mNativeMostVisitedSitesBridge == 0) {
On 2017/05/25 10:33:33, dgn wrote:
> can it happen in the normal functioning of the class? just assert otherwise?
> That's what we do in the other functions.

Changed to assert.
This can not happen anymore (because the listener is always removed before
destroy()-ing the mNativeBridge; and initialized after native initialization).

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:95:
// Ensure the home page is set as first tile if (re-)enabled - even if
blacklisted before.
On 2017/05/25 10:33:33, dgn wrote:
> how does that set the tile as first? to me it only looks like it just removes
it
> from the backlist. Isn't the order implementation detail thus not useful as a
> comment here?

Absolutely. Gone.

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:110:
return HomepageManager.getHomepageUri(getApplicationContext())
On 2017/05/25 10:33:33, dgn wrote:
> compare strings with TextUtils.equals(s1, s2), the home page URI being null
here
> would cause a crash. Also, why not just use getHomePageUrl here?

Thanks, done and used.
(This code used to work because isHomePageEnabled() is checked before anything
else but it's of course better to avoid the pitfalls for future changes.)

https://codereview.chromium.org/2897293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:115:
public String getHomePageUrl() {
On 2017/05/25 10:33:33, dgn wrote:
> this can return null, mark @Nullable? also, does the backend properly handle
> that?

@Nullable added.
The backend handles null values now. (Previously, it relied on checking whether
the feature is enabled. But setting an empty page would have crashed.)

[fhorschig, 2017-05-30 12:53:18]

@mastiz: Thanks for the confirmation! Would you mind having a look at the
component/ and chrome/browser/ parts of this CL?
(Esp. since handling an empty home page changed)

[mastiz, 2017-06-01 08:18:45]

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

https://codereview.chromium.org/2897293002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:33:
initializeClient();
Not that I'm against your proposal, but just to clarify: according to this
snippet, looks like initializeClient() can be merged into nativeInit()? What is
the rationale against, that you need to verify the feature toggle? Or is it
mostly about the client adapter that you instantiate? Please add a comment.

https://codereview.chromium.org/2897293002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites_bridge.h (right):

https://codereview.chromium.org/2897293002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites_bridge.h:33: void
SetClient(JNIEnv* env,
Nit: SetHomePageClient for consistency?

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:62: home_page_client_(nullptr),
Nit: no need for this line, unique_ptr constructs empty by default, as opposed
to raw points like observer_ below.

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:487:
!home_page_client_->GetHomepageUrl().is_empty() &&
For safety, check for home_page_client_ being nonnull?

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites_unittest.cc:384:
most_visited_sites_->SetHomePageClient(std::move(home_page_client));
Can you please add at lest one test where there is no homepage client? Optional:
this is in fact the default, so one can argue that most tests shouldn't use a
homepage client, and a few that require verifying the corresponding requirements
would instantiate it (possibly by having another fixture,
MostVisitedSitesWithHomePageClientTest).

[fhorschig, 2017-06-01 10:12:36]

Patchset #3 (id:60001) has been deleted

[fhorschig, 2017-06-01 10:12:48]

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

[fhorschig, 2017-06-01 10:12:56]

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

https://codereview.chromium.org/2897293002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java:33:
initializeClient();
On 2017/06/01 08:18:44, mastiz wrote:
> Not that I'm against your proposal, but just to clarify: according to this
> snippet, looks like initializeClient() can be merged into nativeInit()? What
is
> the rationale against, that you need to verify the feature toggle? Or is it
> mostly about the client adapter that you instantiate? Please add a comment.

Inlining seems to be the cleanest way (now).

https://codereview.chromium.org/2897293002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites_bridge.h (right):

https://codereview.chromium.org/2897293002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites_bridge.h:33: void
SetClient(JNIEnv* env,
On 2017/06/01 08:18:44, mastiz wrote:
> Nit: SetHomePageClient for consistency?

Thanks, I missed one :D

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:62: home_page_client_(nullptr),
On 2017/06/01 08:18:44, mastiz wrote:
> Nit: no need for this line, unique_ptr constructs empty by default, as opposed
> to raw points like observer_ below.

Gone.

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:487:
!home_page_client_->GetHomepageUrl().is_empty() &&
On 2017/06/01 08:18:44, mastiz wrote:
> For safety, check for home_page_client_ being nonnull?

Added a comment to make this existing check more visible.
(Otherwise, git cl format will hide it again.)

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites_unittest.cc:384:
most_visited_sites_->SetHomePageClient(std::move(home_page_client));
On 2017/06/01 08:18:44, mastiz wrote:
> Can you please add at lest one test where there is no homepage client?
Optional:

Added test with feature but without client.

> this is in fact the default, so one can argue that most tests shouldn't use a
> homepage client, and a few that require verifying the corresponding
requirements
> would instantiate it (possibly by having another fixture,
> MostVisitedSitesWithHomePageClientTest).

I have every test create the client explicitly which highlights the
requirement. By default, no client is necessary.
Additionally, this gets rid of the class member access.

[commit-bot: I haz the power, 2017-06-01 10:13:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2017-06-01 10:59:24]

LGTM.

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:487:
!home_page_client_->GetHomepageUrl().is_empty() &&
On 2017/06/01 10:12:56, fhorschig wrote:
> On 2017/06/01 08:18:44, mastiz wrote:
> > For safety, check for home_page_client_ being nonnull?
> 
> Added a comment to make this existing check more visible.
> (Otherwise, git cl format will hide it again.)

Oop, sorry, I totally missed the fact that it predates this CL.

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/2897293002/diff/40001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites_unittest.cc:384:
most_visited_sites_->SetHomePageClient(std::move(home_page_client));
On 2017/06/01 10:12:56, fhorschig wrote:
> On 2017/06/01 08:18:44, mastiz wrote:
> > Can you please add at lest one test where there is no homepage client?
> Optional:
> 
> Added test with feature but without client.
> 
> > this is in fact the default, so one can argue that most tests shouldn't use
a
> > homepage client, and a few that require verifying the corresponding
> requirements
> > would instantiate it (possibly by having another fixture,
> > MostVisitedSitesWithHomePageClientTest).
> 
> I have every test create the client explicitly which highlights the
> requirement. By default, no client is necessary.
> Additionally, this gets rid of the class member access.

I like it much better now, thanks!

[commit-bot: I haz the power, 2017-06-01 11:17:08]

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

[commit-bot: I haz the power, 2017-06-01 11:17:09]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-06-01 12:22:28]

lgtm

[fhorschig, 2017-06-01 12:24:04]

The CQ bit was checked by fhorschig@chromium.org

[commit-bot: I haz the power, 2017-06-01 12:24:19]

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-06-01 12:28:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496319844616560,
"parent_rev": "9e1250518374b610d8a23e8b87c44a717a8c919a", "commit_rev":
"3fc1f818eaab417801a21886ce9c293701b10486"}

[commit-bot: I haz the power, 2017-06-01 12:28:37]

Description was changed from

==========
Adding CrHome-specific implementation for home page tile.

This CL adds an HomePageClient for Android that is only set if the
kChromeHome feature is enabled because the normal NTP has a working
home button.

If either kChromeHome or kPinHomePageAsTileFeature is not enabled,
the MostVisited tiles will not contain the home tile.

Special case: The home tile can be removed as every tile and will be
blacklisted then.
By explicitly enabling (or reenabling) the Homepage, the blacklisting
will be undone and the tile will reappear as first tile.

Removing the home tile does not disable the Homepage setting (because
this setting also controls the start-up page and the home tile is not
visually linked to the home page .... besides having the same URL).

BUG=703994
==========

to

==========
Adding CrHome-specific implementation for home page tile.

This CL adds an HomePageClient for Android that is only set if the
kChromeHome feature is enabled because the normal NTP has a working
home button.

If either kChromeHome or kPinHomePageAsTileFeature is not enabled,
the MostVisited tiles will not contain the home tile.

Special case: The home tile can be removed as every tile and will be
blacklisted then.
By explicitly enabling (or reenabling) the Homepage, the blacklisting
will be undone and the tile will reappear as first tile.

Removing the home tile does not disable the Homepage setting (because
this setting also controls the start-up page and the home tile is not
visually linked to the home page .... besides having the same URL).

BUG=703994

Review-Url: https://codereview.chromium.org/2897293002
Cr-Commit-Position: refs/heads/master@{#476258}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc1f818eaab417801a21886ce9c...
==========

[commit-bot: I haz the power, 2017-06-01 12:28:38]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3fc1f818eaab417801a21886ce9c...