Unify NewTabPageItem and ItemGroup into a single tree-structured interface.

Unify NewTabPageItem and ItemGroup into a single tree-structured interface.

BUG=616090
Committed: https://crrev.com/728cf24b0a3552c2b60ed39f82760ff6d18cc21a
Cr-Commit-Position: refs/heads/master@{#425688}

[Bernhard Bauer, 2016-10-04 14:29:04]

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

[Bernhard Bauer, 2016-10-04 14:29:07]

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

[commit-bot: I haz the power, 2016-10-04 14:29:14]

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, 2016-10-04 15:40:00]

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

[commit-bot: I haz the power, 2016-10-04 15:40:00]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-13 11:20:57]

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

[commit-bot: I haz the power, 2016-10-13 11:21:03]

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, 2016-10-13 12:38:20]

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

[commit-bot: I haz the power, 2016-10-13 12:38:21]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-13 12:49:30]

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

[Bernhard Bauer, 2016-10-13 12:49:31]

Please review!

[PEConn, 2016-10-13 13:48:55]

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

[PEConn, 2016-10-13 13:48:56]

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:21:
int getChildIndexForPosition(int position) {
Is this meant to be package private?

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:49:
numItems += child.getItemCount();
Can we cache this?

[Bernhard Bauer, 2016-10-13 13:54:38]

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:21:
int getChildIndexForPosition(int position) {
On 2016/10/13 13:48:56, PEConn wrote:
> Is this meant to be package private?

Yes, I'm calling it from NewTabPageAdapter to expose getting the
SuggestionsSection for a given position for unit tests.

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:49:
numItems += child.getItemCount();
On 2016/10/13 13:48:56, PEConn wrote:
> Can we cache this?

I'm planning to do that in one of the next CLs, it just requires that we
properly notify about all changes, which I have to ensure first :)

[dgn, 2016-10-13 15:42:40]

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

[dgn, 2016-10-13 15:42:41]

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java:19:
ItemViewType.VIEW_TYPE_ABOVE_THE_FOLD,
nit: remove the VIEW_TYPE prefix on the constants? it's now mentioned by the
parent class.

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:303:

change it to as switch while we're at it?

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:384:
public int getSuggestionPosition(SnippetArticle foo) {
foo?

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:547:
SuggestionsSection section = getSuggestionsSection(swipePos);
You can swipe the signin promo and it's not a suggestion section, I think it
will crash

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:53:
if (position < 1) throw new IndexOutOfBoundsException();
why do we throw for 0 (the header) but just return null for the last item
(progress indicator)?  Also, it might be useful to have a comment making
explicit the magic numbers used here.

[Bernhard Bauer, 2016-10-13 16:13:10]

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

[Bernhard Bauer, 2016-10-13 16:13:14]

Thanks for the review!

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java:19:
ItemViewType.VIEW_TYPE_ABOVE_THE_FOLD,
On 2016/10/13 15:42:41, dgn wrote:
> nit: remove the VIEW_TYPE prefix on the constants? it's now mentioned by the
> parent class.

Done.

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:303:

On 2016/10/13 15:42:41, dgn wrote:
> change it to as switch while we're at it?

Done.

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:384:
public int getSuggestionPosition(SnippetArticle foo) {
On 2016/10/13 15:42:41, dgn wrote:
> foo?

Oops... renamed.

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:547:
SuggestionsSection section = getSuggestionsSection(swipePos);
On 2016/10/13 15:42:41, dgn wrote:
> You can swipe the signin promo and it's not a suggestion section, I think it
> will crash

Good catch! Done.

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2396523002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:53:
if (position < 1) throw new IndexOutOfBoundsException();
On 2016/10/13 15:42:41, dgn wrote:
> why do we throw for 0 (the header) but just return null for the last item
> (progress indicator)?  Also, it might be useful to have a comment making
> explicit the magic numbers used here.

Oh, good catch! Actually, I realized that the default implementation will
already just dispatch to the right child, so I just need to make SnippetArticle
return itself.

[commit-bot: I haz the power, 2016-10-13 16:13:29]

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, 2016-10-13 17:35:57]

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

[commit-bot: I haz the power, 2016-10-13 17:35:59]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-17 09:20:43]

Friendly ping? :)

[PEConn, 2016-10-17 09:23:34]

On 2016/10/17 09:20:43, Bernhard Bauer wrote:
> Friendly ping? :)

LGTM with respect to my comments.

[dgn, 2016-10-17 09:58:01]

lgtm!

https://codereview.chromium.org/2396523002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2396523002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:305:
case ItemViewType.ABOVE_THE_FOLD: {
optional nit: why the brackets?

[Bernhard Bauer, 2016-10-17 14:33:46]

https://codereview.chromium.org/2396523002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2396523002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:305:
case ItemViewType.ABOVE_THE_FOLD: {
On 2016/10/17 09:58:01, dgn wrote:
> optional nit: why the brackets?

Hm, I thought the brace-less style would only be allowed if the return fits on
the same line as the case label (per standard Java style), but I can find other
examples in the code base. OK, done.

[Bernhard Bauer, 2016-10-17 14:42:18]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2016-10-17 14:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2396523002/#ps140001
(title: "review")

[commit-bot: I haz the power, 2016-10-17 14:42:33]

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

[moscow030339, 2016-10-17 15:35:06]

moscow030339@gmail.com changed reviewers:
+ moscow030339@gmail.com

[commit-bot: I haz the power, 2016-10-17 15:48:40]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-17 15:51:12]

Description was changed from

==========
Unify NewTabPageItem and ItemGroup into a single tree-structured interface.

BUG=616090
==========

to

==========
Unify NewTabPageItem and ItemGroup into a single tree-structured interface.

BUG=616090

Committed: https://crrev.com/728cf24b0a3552c2b60ed39f82760ff6d18cc21a
Cr-Commit-Position: refs/heads/master@{#425688}
==========

[commit-bot: I haz the power, 2016-10-17 15:51:14]

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

[moscow030339, 2016-10-17 21:51:52]

понедельник, 17 октября 2016 г. пользователь commit-bot@chromium.org via
codereview.chromium.org написал:

> Patchset 8 (id:??) landed as
> https://crrev.com/728cf24b0a3552c2b60ed39f82760ff6d18cc21a
> Cr-Commit-Position: refs/heads/master@{#425688}
>
> https://codereview.chromium.org/2396523002/
>


-- 
Отправлено с мобильного устройства Алена

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.