Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389383007:
[sync] Open tabs from other devices are shown after disabling "Open Tabs" (Closed)
|
Created:
4 years, 2 months ago by Patrick Noland Modified:
4 years, 2 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sync] Open tabs from other devices are shown after disabling "Open Tabs"
Check the isTabSyncEnabled flag before fetching foreign sessions in ForeignSessionHelper
BUG=651972
R=peconn
Committed: https://crrev.com/90380f52bb9fb0a6e833701cdfe38541b08a0265
Cr-Commit-Position: refs/heads/master@{#423951}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (10 generated)
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
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.
Description was changed from ========== [sync] Open tabs from other devices are shown after disabling "Open Tabs" BUG=651972 ========== to ========== [sync] Open tabs from other devices are shown after disabling "Open Tabs" Check the isTabSyncEnabled flag before fetching foreign sessions in ForeignSessionHelper BUG=651972 R=peconn ==========
pnoland@chromium.org changed reviewers: + peconn@chromium.org
Peter, PTAL
peconn@chromium.org changed reviewers: + tedchoc@chromium.org
I'm not very familiar with this code (I'm assuming you picked me from the OWNERS file), I've added in tedchoc who's also in OWNERS who will probably have more knowledge about this than me.
lgtm I was thinking that this would be better suited of a check in: SessionsSyncManager::GetAllForeignSessions But maybe that component isn't aware of the enabled state and this check he is better. I'll defer to you on that, but in general, I think it would be nicer to have this logic closer to the underlying logic to ensure it is consistent across platforms. https://codereview.chromium.org/2389383007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java (right): https://codereview.chromium.org/2389383007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java:163: if (!isTabSyncEnabled()) { in java if the statement and condition fit one line, then you can do that here it would be easily readable as well, so I would opt for that. with that said, you're being consistent with the lines a few down, so that is ok as well
On 2016/10/07 17:39:47, Ted C wrote: > lgtm > > I was thinking that this would be better suited of a check in: > SessionsSyncManager::GetAllForeignSessions > > But maybe that component isn't aware of the enabled state and this check he is > better. I'll defer to you on that, but in general, I think it would be nicer to > have this logic closer to the underlying logic to ensure it is consistent across > platforms. > > https://codereview.chromium.org/2389383007/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java > (right): > > https://codereview.chromium.org/2389383007/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java:163: > if (!isTabSyncEnabled()) { > in java if the statement and condition fit one line, then you can do that > > here it would be easily readable as well, so I would opt for that. > > with that said, you're being consistent with the lines a few down, so that is ok > as well I agree that it's preferable to have the check closer to the underlying logic. The problem is twofold: SessionsSyncManager doesn't have access to the state currently, and GetAllForeignSessions is used elsewhere in a context that doesn't care about whether or not Open Tabs are enabled or not. The check I've added is analogous to the check currently done by the history ui.
https://codereview.chromium.org/2389383007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java (right): https://codereview.chromium.org/2389383007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java:163: if (!isTabSyncEnabled()) { On 2016/10/07 17:39:47, Ted C wrote: > in java if the statement and condition fit one line, then you can do that > > here it would be easily readable as well, so I would opt for that. > > with that said, you're being consistent with the lines a few down, so that is ok > as well This is the result of "git cl format" and it's consistent with the below block, so I'm going to leave it as is.
The CQ bit was checked by pnoland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [sync] Open tabs from other devices are shown after disabling "Open Tabs" Check the isTabSyncEnabled flag before fetching foreign sessions in ForeignSessionHelper BUG=651972 R=peconn ========== to ========== [sync] Open tabs from other devices are shown after disabling "Open Tabs" Check the isTabSyncEnabled flag before fetching foreign sessions in ForeignSessionHelper BUG=651972 R=peconn ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [sync] Open tabs from other devices are shown after disabling "Open Tabs" Check the isTabSyncEnabled flag before fetching foreign sessions in ForeignSessionHelper BUG=651972 R=peconn ========== to ========== [sync] Open tabs from other devices are shown after disabling "Open Tabs" Check the isTabSyncEnabled flag before fetching foreign sessions in ForeignSessionHelper BUG=651972 R=peconn Committed: https://crrev.com/90380f52bb9fb0a6e833701cdfe38541b08a0265 Cr-Commit-Position: refs/heads/master@{#423951} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/90380f52bb9fb0a6e833701cdfe38541b08a0265 Cr-Commit-Position: refs/heads/master@{#423951} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
