Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904713002:
[Home] Wait to monitor NTP content suggestions until new tab created (Closed)
Description[Home] Wait to monitor NTP content suggestions until new tab created
With the new Chrome Home NTP design, a new tab isn't created until
the user navigations inside the NTP. While the Chrome Home NTP is
open, the active tab is null. Wait to call
NewTabPageUma.monitorContentSuggestionsVisit() until after the
suggestion URL has been loaded, which ensures the active tab
is not null.
BUG=725688
Review-Url: https://codereview.chromium.org/2904713002
Cr-Commit-Position: refs/heads/master@{#474316}
Committed: https://chromium.googlesource.com/chromium/src/+/4a09eb44c2529b669b00191c0613db43542c7708
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check for null tab in SuggestionsBottomSheetContent #Patch Set 3 : Revert changes to SuggestionsBottomSheetContent #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
twellington@chromium.org changed reviewers: + dgn@chromium.org
ptal
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2904713002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java (right): https://codereview.chromium.org/2904713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java:139: if (windowOpenDisposition == WindowOpenDisposition.CURRENT_TAB) { Another options would be to change the "CURRENT_TAB" disposition to "NEW_WINDOW" when the new Chrome Home NTP design where the bottom sheet is shown over the tab switcher is showing.
lgtm https://codereview.chromium.org/2904713002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java (right): https://codereview.chromium.org/2904713002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java:139: if (windowOpenDisposition == WindowOpenDisposition.CURRENT_TAB) { On 2017/05/24 01:32:05, Theresa wrote: > Another options would be to change the "CURRENT_TAB" disposition to "NEW_WINDOW" > when the new Chrome Home NTP design where the bottom sheet is shown over the tab > switcher is showing. Here we use disposition as marker for the user's intent. We consider the user wants to open the suggestion in the foreground, so we map it to CURRENT_TAB. We could change it to WindowOpenDisposition.NEW_FOREGROUND_TAB when there is no active tab but I'm not convinced it would be a big improvement. The current fix looks good and is a small step towards fixing crbug.com/665915.
There is another fallout of the refactoring:
Crash when sheet opens with CONTEXTUAL_SUGGESTIONS_CAROUSEL feature enabled. The
code below would fix it. Should it be part of this CL?
if (ChromeFeatureList.isEnabled(
- ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL))
{
+ ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL)
+ && sheet.getActiveTab() != null) {
updateContextualSuggestions(sheet.getActiveTab().getUrl());
}
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
On 2017/05/24 10:57:00, dgn wrote:
> There is another fallout of the refactoring:
>
> Crash when sheet opens with CONTEXTUAL_SUGGESTIONS_CAROUSEL feature enabled.
The
> code below would fix it. Should it be part of this CL?
>
> if (ChromeFeatureList.isEnabled(
> -
ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL))
> {
> +
ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL)
> + && sheet.getActiveTab() != null) {
>
updateContextualSuggestions(sheet.getActiveTab().getUrl());
> }
Yes. Done, thank you for catching that one.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Home] Wait to monitor NTP content suggestions until new tab created With the new Chrome Home NTP design, a new tab isn't created until the user navigations inside the NTP. While the Chrome Home NTP is open, the active tab is null. Wait to call NewTabPageUma.monitorContentSuggestionsVisit() until after the suggestion URL has been loaded, which ensures the active tab is not null. BUG=725688 ========== to ========== [Home] Wait to monitor NTP content suggestions until new tab created With the new Chrome Home NTP design, a new tab isn't created until the user navigations inside the NTP. While the Chrome Home NTP is open, the active tab is null. Wait to call NewTabPageUma.monitorContentSuggestionsVisit() until after the suggestion URL has been loaded, which ensures the active tab is not null. Also checks that the active tab is not null in SuggestionsBottomSheetContent before attempting to update contextual suggestions. BUG=725688 ==========
On 2017/05/24 15:02:53, Theresa wrote:
> On 2017/05/24 10:57:00, dgn wrote:
> > There is another fallout of the refactoring:
> >
> > Crash when sheet opens with CONTEXTUAL_SUGGESTIONS_CAROUSEL feature enabled.
> The
> > code below would fix it. Should it be part of this CL?
> >
> > if (ChromeFeatureList.isEnabled(
> > -
> ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL))
> > {
> > +
> ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL)
> > + && sheet.getActiveTab() != null) {
> >
> updateContextualSuggestions(sheet.getActiveTab().getUrl());
> > }
>
> Yes. Done, thank you for catching that one.
Sorry forgot to update here, some already sent another CL:
https://codereview.chromium.org/2903583005/
On 2017/05/24 15:30:26, dgn wrote:
> On 2017/05/24 15:02:53, Theresa wrote:
> > On 2017/05/24 10:57:00, dgn wrote:
> > > There is another fallout of the refactoring:
> > >
> > > Crash when sheet opens with CONTEXTUAL_SUGGESTIONS_CAROUSEL feature
enabled.
> > The
> > > code below would fix it. Should it be part of this CL?
> > >
> > > if (ChromeFeatureList.isEnabled(
> > > -
> > ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL))
> > > {
> > > +
> > ChromeFeatureList.CONTEXTUAL_SUGGESTIONS_CAROUSEL)
> > > + && sheet.getActiveTab() != null) {
> > >
> > updateContextualSuggestions(sheet.getActiveTab().getUrl());
> > > }
> >
> > Yes. Done, thank you for catching that one.
>
> Sorry forgot to update here, some already sent another CL:
> https://codereview.chromium.org/2903583005/
No problem. I'll revert my change to SuggestionsBottomSheetContent. Thank you
for the heads up.
Description was changed from ========== [Home] Wait to monitor NTP content suggestions until new tab created With the new Chrome Home NTP design, a new tab isn't created until the user navigations inside the NTP. While the Chrome Home NTP is open, the active tab is null. Wait to call NewTabPageUma.monitorContentSuggestionsVisit() until after the suggestion URL has been loaded, which ensures the active tab is not null. Also checks that the active tab is not null in SuggestionsBottomSheetContent before attempting to update contextual suggestions. BUG=725688 ========== to ========== [Home] Wait to monitor NTP content suggestions until new tab created With the new Chrome Home NTP design, a new tab isn't created until the user navigations inside the NTP. While the Chrome Home NTP is open, the active tab is null. Wait to call NewTabPageUma.monitorContentSuggestionsVisit() until after the suggestion URL has been loaded, which ensures the active tab is not null. BUG=725688 ==========
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2904713002/#ps40001 (title: "Revert changes to SuggestionsBottomSheetContent")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495640026896200,
"parent_rev": "4cf58ca84df11f476917c1b1c8130f2400c12d96", "commit_rev":
"4a09eb44c2529b669b00191c0613db43542c7708"}
Message was sent while issue was closed.
Description was changed from ========== [Home] Wait to monitor NTP content suggestions until new tab created With the new Chrome Home NTP design, a new tab isn't created until the user navigations inside the NTP. While the Chrome Home NTP is open, the active tab is null. Wait to call NewTabPageUma.monitorContentSuggestionsVisit() until after the suggestion URL has been loaded, which ensures the active tab is not null. BUG=725688 ========== to ========== [Home] Wait to monitor NTP content suggestions until new tab created With the new Chrome Home NTP design, a new tab isn't created until the user navigations inside the NTP. While the Chrome Home NTP is open, the active tab is null. Wait to call NewTabPageUma.monitorContentSuggestionsVisit() until after the suggestion URL has been loaded, which ensures the active tab is not null. BUG=725688 Review-Url: https://codereview.chromium.org/2904713002 Cr-Commit-Position: refs/heads/master@{#474316} Committed: https://chromium.googlesource.com/chromium/src/+/4a09eb44c2529b669b00191c0613... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4a09eb44c2529b669b00191c0613... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
