Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2069183002:
[Sync] Add option to remove device from "Other devices" on desktop (Closed)
|
Created:
4 years, 6 months ago by pavely Modified:
4 years, 4 months ago CC:
chromium-reviews, Patrick Dubroy, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, calamity Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Add option to remove device from "Other devices" on desktop
It is possible on Android/iOS to remove device entry from "Recent tabs"
page. I'm adding item to drop-down menu on history page that offers
similar functionality for desktop.
BUG=280665
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6e61e14818be83050cedff17a26d373dd2657085
Cr-Commit-Position: refs/heads/master@{#409051}
Patch Set 1 #Patch Set 2 : Fix build. #
Total comments: 1
Patch Set 3 : Fix string order #
Total comments: 8
Patch Set 4 : Address Dan's comments. #
Messages
Total messages: 32 (11 generated)
Description was changed from ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 ========== to ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
pavely@chromium.org changed reviewers: + dbeam@chromium.org, isherman@chromium.org, sdefresne@chromium.org
PTAL. dbeam@chromium.org chrome/browser/resources/history/other_devices.js chrome/browser/ui/webui/history_ui.cc sdefresne@chromium.org components/history_strings.grdp isherman@chromium.org tools/metrics/histograms/histograms.xml
histograms.xml lgtm
fyi: i don't think other devices show up on mobile, and we're replacing the desktop UI very soon (M53-54)
/cc tsergeant@ and calamity@
On 2016/06/15 20:50:13, Dan Beam wrote: > fyi: i don't think other devices show up on mobile, and we're replacing the > desktop UI very soon (M53-54) On mobile they are exposed on NTP->"Recent tabs" grouped by device. Long press on device offers removing it from the list. The bug is to bring this functionality to desktop. Are other devices exposed in new history UI? Is there a good place to add this functionality there?
components/history_string.grp lgtm if the answer to the question is no https://codereview.chromium.org/2069183002/diff/20001/components/history_stri... File components/history_strings.grdp (right): https://codereview.chromium.org/2069183002/diff/20001/components/history_stri... components/history_strings.grdp:114: Hide for now Do we need a titlecase version?
On 2016/06/16 09:16:57, sdefresne wrote: > components/history_string.grp lgtm if the answer to the question is no > > https://codereview.chromium.org/2069183002/diff/20001/components/history_stri... > File components/history_strings.grdp (right): > > https://codereview.chromium.org/2069183002/diff/20001/components/history_stri... > components/history_strings.grdp:114: Hide for now > Do we need a titlecase version? I'm adding menu item, other items in this menu don't have titlecase versions. I'm assuming this one doesn't need it either.
i'm pretty sure "Other devices" are now called "Synced tabs" flip chrome://flags#enable-md-history to see for yourself I wouldn't invest in chrome/browser/resources/history at this point in time.
On 2016/06/18 01:47:53, Dan Beam wrote: > i'm pretty sure "Other devices" are now called "Synced tabs" > > flip chrome://flags#enable-md-history to see for yourself > > I wouldn't invest in chrome/browser/resources/history at this point in time. Dan, I sketched a change that adds the same option to md_history. I think I can get it to production quality. How do you think I should proceed? I can: - add md_history changes to this CL (they reuse the same string from history_strings.grdp) - or abandon this change and start a new one for md_history only - or simply defer this change to md_history owners
i would just port to md_history at this point, but I'll let tsergeant/calamity weigh in
On 2016/06/18 01:47:53, Dan Beam wrote: > i'm pretty sure "Other devices" are now called "Synced tabs" > > flip chrome://flags#enable-md-history to see for yourself > > I wouldn't invest in chrome/browser/resources/history at this point in time. I think this change is still useful. It adds a string that will be reused in md_history UI and provides reference on how to hook menu item to handler. Could you review this CL?
On 2016/06/23 23:52:41, Dan Beam wrote: > i would just port to md_history at this point, but I'll let tsergeant/calamity > weigh in I'm still happy for this CL to go ahead. It's quite small and straight-forward, plus we're not launching MD History in M53 anymore.
On 2016/06/23 23:52:41, Dan Beam wrote: > i would just port to md_history at this point, but I'll let tsergeant/calamity > weigh in Dan, Could you review this change? Tim is Ok with landing it.
https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/other_devices.js (right): https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/other_devices.js:73: this.deleteItem_ = this.appendMenuItem_('deleteSessionMenuItemText'); why do we need to set to this.deleteItem_ rather than just var deleteItem? https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:81: IDS_HISTORY_OTHER_SESSIONS_OPEN_ALL); shouldn't this be done only on !android? https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:83: IDS_HISTORY_OTHER_SESSIONS_HIDE_FOR_NOW); indent off https://codereview.chromium.org/2069183002/diff/40001/components/history_stri... File components/history_strings.grdp (right): https://codereview.chromium.org/2069183002/diff/40001/components/history_stri... components/history_strings.grdp:112: </message> can this be wrapped in an <if expr="not is_android">?
Description was changed from ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by pavely@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/other_devices.js (right): https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/other_devices.js:73: this.deleteItem_ = this.appendMenuItem_('deleteSessionMenuItemText'); On 2016/07/30 00:54:08, Dan Beam wrote: > why do we need to set to this.deleteItem_ rather than just var deleteItem? You are right, in contrast with collapseItem_ and expandItem_ this one is not referenced outside of this function. https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:81: IDS_HISTORY_OTHER_SESSIONS_OPEN_ALL); On 2016/07/30 00:54:08, Dan Beam wrote: > shouldn't this be done only on !android? Done. https://codereview.chromium.org/2069183002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:83: IDS_HISTORY_OTHER_SESSIONS_HIDE_FOR_NOW); On 2016/07/30 00:54:08, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/2069183002/diff/40001/components/history_stri... File components/history_strings.grdp (right): https://codereview.chromium.org/2069183002/diff/40001/components/history_stri... components/history_strings.grdp:112: </message> On 2016/07/30 00:54:08, Dan Beam wrote: > can this be wrapped in an <if expr="not is_android">? I wrapped four strings related to "other devices" ui.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by pavely@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2069183002/#ps60001 (title: "Address Dan's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== [Sync] Add option to remove device from "Other devices" on desktop It is possible on Android/iOS to remove device entry from "Recent tabs" page. I'm adding item to drop-down menu on history page that offers similar functionality for desktop. BUG=280665 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6e61e14818be83050cedff17a26d373dd2657085 Cr-Commit-Position: refs/heads/master@{#409051} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6e61e14818be83050cedff17a26d373dd2657085 Cr-Commit-Position: refs/heads/master@{#409051} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
