Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668693007:
[MD Bookmarks] Double Click (Closed)
Description[MD Bookmarks] Double Click
This CL adds double click behaviours for items inside the list.
When item gets clicked, if it's a folder, it will open the
folder. Otherwise it will open the bookmark in a new tab.
BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2668693007
Cr-Commit-Position: refs/heads/master@{#448900}
Committed: https://chromium.googlesource.com/chromium/src/+/5827f99e6bf79d952a90edc4bc783e7a1c4fa690
Patch Set 1 : clean up #
Total comments: 2
Patch Set 2 : clean up and add test #
Total comments: 2
Patch Set 3 : renaming #
Total comments: 2
Patch Set 4 : change behaviour back to single item #
Total comments: 2
Patch Set 5 : clean up #
Total comments: 2
Patch Set 6 : change the loop and add tab changes #
Total comments: 5
Patch Set 7 : fix closure #
Depends on Patchset: Messages
Total messages: 34 (14 generated)
Description was changed from ========== [MD Bookmarks] Double Click This CL adds double click behaviours for items inside the list. When single selected item gets clicked, if it's a folder, it will open the folder. Otherwise it will open the bookmark in a new tab. When multiple items are selected, it will open all bookmarks and remain the selected folders untouched. BUG=658980 ========== to ========== [MD Bookmarks] Double Click This CL adds double click behaviours for items inside the list. When single selected item gets clicked, if it's a folder, it will open the folder. Otherwise it will open the bookmark in a new tab. When multiple items are selected, it will open all bookmarks and remain the selected folders untouched. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
jiaxi@google.com changed reviewers: + angelayang@google.com
Cool looking good- makes sense https://codereview.chromium.org/2668693007/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2668693007/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:67: item: this.item, did you need to fire any other properties here? if not, can we do this.fire('open-item', this.item) and just capture on other side as var item = e.detail etc.
https://codereview.chromium.org/2668693007/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/item.js (right): https://codereview.chromium.org/2668693007/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/item.js:67: item: this.item, On 2017/02/01 04:31:23, angelayang wrote: > did you need to fire any other properties here? if not, can we do > this.fire('open-item', this.item) and just capture on other side as var item = > e.detail etc. Done.
lgtm
jiaxi@google.com changed reviewers: + calamity@chromium.org, tsergeant@chromium.org
one thing https://codereview.chromium.org/2668693007/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:423: this.fire('selected-folder-changed', e.detail.id); Sorry just my preference but could you make var item = /** BookmarkTreeNode */ (e.detail). just easier to read for me
https://codereview.chromium.org/2668693007/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/40001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:423: this.fire('selected-folder-changed', e.detail.id); On 2017/02/01 04:50:42, angelayang wrote: > Sorry just my preference but could you make var item = /** BookmarkTreeNode */ > (e.detail). just easier to read for me Done.
https://codereview.chromium.org/2668693007/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:428: for (let index of this.selectedIndexSet_) { Chris and I just tried the old bookmark manager and it seems like double clicking an item when you've got multiple items selected just opens the one you're clicking on. It seems good to just do the basics for now in the interest of getting this in before the end of the internship. If you get time, we can come back and do the more complex behavior. If you just want to do the simple behavior, you probably don't need to add this new event in store -- you can just call tabs.create or selected-folder-changed from list directly.
Description was changed from ========== [MD Bookmarks] Double Click This CL adds double click behaviours for items inside the list. When single selected item gets clicked, if it's a folder, it will open the folder. Otherwise it will open the bookmark in a new tab. When multiple items are selected, it will open all bookmarks and remain the selected folders untouched. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Double Click This CL adds double click behaviours for items inside the list. When item gets clicked, if it's a folder, it will open the folder. Otherwise it will open the bookmark in a new tab. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2668693007/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:428: for (let index of this.selectedIndexSet_) { On 2017/02/02 02:02:38, tsergeant wrote: > Chris and I just tried the old bookmark manager and it seems like double > clicking an item when you've got multiple items selected just opens the one > you're clicking on. > > It seems good to just do the basics for now in the interest of getting this in > before the end of the internship. If you get time, we can come back and do the > more complex behavior. > > If you just want to do the simple behavior, you probably don't need to add this > new event in store -- you can just call tabs.create or selected-folder-changed > from list directly. Done.
lgtm https://codereview.chromium.org/2668693007/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:253: for (var i = startIndex; i <= endIndex; i++) { Nit: May as well revert these {} changes for now.
https://codereview.chromium.org/2668693007/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:253: for (var i = startIndex; i <= endIndex; i++) { On 2017/02/02 03:21:01, tsergeant wrote: > Nit: May as well revert these {} changes for now. Done.
calamity@chromium.org changed reviewers: + benwells@chromium.org
lgtm, +benwells for _api_features.json OWNERS. https://codereview.chromium.org/2668693007/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:375: } I'm not a super fan of the loop structure here and the reuse of the folder variable (if someone wants to use folder beneath this line, they might get bitten by the fact it's changed from the selected folder to the root node). I think you should just stop when something is a child of the root node. This loop will actually tell the root node to open itself too. I'm also just used to loop variable updates at the end of the loop. How about: var parent = this.idToNodeMap_[folder.parentId]; while (parent.id != this.rootNode.id) { if (!parent.isOpen) this.fire(...); parent = this.idToNodeMap_[parent.parentId]; }
https://codereview.chromium.org/2668693007/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2668693007/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:375: } On 2017/02/03 04:12:22, calamity wrote: > I'm not a super fan of the loop structure here and the reuse of the folder > variable (if someone wants to use folder beneath this line, they might get > bitten by the fact it's changed from the selected folder to the root node). > > I think you should just stop when something is a child of the root node. This > loop will actually tell the root node to open itself too. > > I'm also just used to loop variable updates at the end of the loop. > > How about: > > var parent = this.idToNodeMap_[folder.parentId]; > while (parent.id != this.rootNode.id) { > if (!parent.isOpen) > this.fire(...); > > parent = this.idToNodeMap_[parent.parentId]; > } This will return a "cannot read id of undefined" if we're selecting the rootNode. Also I don't think this loop tell the root node to open itselt cause it stops when parent == rootNode. I've made some changes to this loop. Please take a look.
https://codereview.chromium.org/2668693007/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2668693007/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_tab_util.cc:445: if (extension) { It looks like this change is just making it so this function can be called with a null extension, and do nothing (but not DCHECK). Is that right? If so can you just do something like if (!extension) return; at the beginning? https://codereview.chromium.org/2668693007/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2668693007/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:753: "channel": "stable", It looks like the other webui things under development limit this to trunk (e.g. usersPrivate). Any reason this shouldn't do the same?
https://codereview.chromium.org/2668693007/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2668693007/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_tab_util.cc:445: if (extension) { On 2017/02/07 07:13:07, benwells wrote: > It looks like this change is just making it so this function can be called with > a null extension, and do nothing (but not DCHECK). Is that right? If so can you > just do something like > > if (!extension) > return; > > at the beginning? This function removes privacy sensitive fields if the extension and tab do not have the appropriate permissions. No knowing what any of this does, it seems safer to scrub this data for null extensions than the alternative. https://codereview.chromium.org/2668693007/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2668693007/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:753: "channel": "stable", On 2017/02/07 07:13:07, benwells wrote: > It looks like the other webui things under development limit this to trunk (e.g. > usersPrivate). Any reason this shouldn't do the same? Most in development things use stable (eg inputMessagePrivate, languageSettingsPrivate), and all the other bookmark stuff (eg: bookmarks, bookmarkManagerPrivate) use stable.
lgtm https://codereview.chromium.org/2668693007/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2668693007/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_tab_util.cc:445: if (extension) { On 2017/02/08 00:04:00, jiaxi wrote: > On 2017/02/07 07:13:07, benwells wrote: > > It looks like this change is just making it so this function can be called > with > > a null extension, and do nothing (but not DCHECK). Is that right? If so can > you > > just do something like > > > > if (!extension) > > return; > > > > at the beginning? > > This function removes privacy sensitive fields if the extension and tab do not > have the appropriate permissions. No knowing what any of this does, it seems > safer to scrub this data for null extensions than the alternative. Ah right, it isn't equivalent to that snippet I put in.
The CQ bit was checked by jiaxi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from angelayang@google.com, tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2668693007/#ps140001 (title: "change the loop and add tab changes")
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
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by jiaxi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, calamity@chromium.org, angelayang@google.com, benwells@chromium.org Link to the patchset: https://codereview.chromium.org/2668693007/#ps150001 (title: "fix closure")
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": 150001, "attempt_start_ts": 1486521487258270,
"parent_rev": "ea12101af3165ed67b9bebd37e8a0ddcc43b3e04", "commit_rev":
"5827f99e6bf79d952a90edc4bc783e7a1c4fa690"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Double Click This CL adds double click behaviours for items inside the list. When item gets clicked, if it's a folder, it will open the folder. Otherwise it will open the bookmark in a new tab. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Double Click This CL adds double click behaviours for items inside the list. When item gets clicked, if it's a folder, it will open the folder. Otherwise it will open the bookmark in a new tab. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2668693007 Cr-Commit-Position: refs/heads/master@{#448900} Committed: https://chromium.googlesource.com/chromium/src/+/5827f99e6bf79d952a90edc4bc78... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:150001) as https://chromium.googlesource.com/chromium/src/+/5827f99e6bf79d952a90edc4bc78... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
