Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
510543002:
Introduce ItemPosition class for enhanced bookmarks. (Closed)
|
Created:
6 years, 3 months ago by Rune Fevang Modified:
6 years, 3 months ago CC:
Yaron, Mark Base URL:
https://chromium.googlesource.com/chromium/src@master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ItemPosition class for enhanced bookmarks.
This class will be used for setting/reading the string positions
used by the enhanced bookmarks.
BUG=411412
Committed: https://crrev.com/a3db1c4f5c38b56b7aa7bd7bf8ff48e54e24587a
Cr-Commit-Position: refs/heads/master@{#293674}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 5
Patch Set 4 : Comments and comparison functions #
Total comments: 11
Patch Set 5 : Yaron comments #Patch Set 6 : Rename to ItemPosition #
Messages
Total messages: 29 (7 generated)
rfevang@chromium.org changed reviewers: + noyau@chromium.org
rfevang@chromium.org changed reviewers: + yfriedman@chromium.org
Ping
mcolbert@chromium.org changed reviewers: + mcolbert@chromium.org
https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.cc (right): https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:12: ".0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"; So the key question here is consistency. What happens when a JS client sets the FolioInfo.position field? Will that work consistently with what is being done here? What happens if two items end up with the same position field? How is that resolved. https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.h (right): https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... components/enhanced_bookmarks/stars_position.h:13: class StarsPosition { Please add class level comment as to what this is used for.
https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.cc (right): https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:12: ".0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"; On 2014/09/04 21:51:24, Mark wrote: > So the key question here is consistency. What happens when a JS client sets the > FolioInfo.position field? Will that work consistently with what is being done > here? What happens if two items end up with the same position field? How is > that resolved. It should be similar, though not necessarily an exact match. The alphabet here is the same as used on the JS side. I wrote a section on duplicate placements in the doc, basically the only thing we can do is write a new position field using the underlying bookmark model position to figure out which order they should be in. BTW, the sync string position clients append a random suffix to avoid two clients setting the same position for different nodes, we could consider doing something similar. https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.h (right): https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... components/enhanced_bookmarks/stars_position.h:13: class StarsPosition { On 2014/09/04 21:51:24, Mark wrote: > Please add class level comment as to what this is used for. Done.
Looks good to me, but we still need OWNERS approval. https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.cc (right): https://codereview.chromium.org/510543002/diff/40001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:12: ".0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"; On 2014/09/04 22:56:47, Rune Fevang wrote: > On 2014/09/04 21:51:24, Mark wrote: > > So the key question here is consistency. What happens when a JS client sets > the > > FolioInfo.position field? Will that work consistently with what is being done > > here? What happens if two items end up with the same position field? How is > > that resolved. > > It should be similar, though not necessarily an exact match. The alphabet here > is the same as used on the JS side. I wrote a section on duplicate placements in > the doc, basically the only thing we can do is write a new position field using > the underlying bookmark model position to figure out which order they should be > in. > > BTW, the sync string position clients append a random suffix to avoid two > clients setting the same position for different nodes, we could consider doing > something similar. That sounds like a good idea.
https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.cc (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:33: bool StarsPosition::IsValid() const { nit: ordering should match header file https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:47: DCHECK(from_index < other.size()); DCHECK_LT (and similar changes throughout) https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:67: // This will cause the returned string will be twice as long as the input one, Needs slight reversing. "returned string to be twice" https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.h (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.h:15: class StarsPosition { StarPosition/ItemPosition? Why is it plural?
https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.cc (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:67: // This will cause the returned string will be twice as long as the input one, On 2014/09/05 03:52:02, Yaron wrote: > Needs slight reversing. "returned string to be twice" err. "revising"
also, please add a tracking bug
https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.cc (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:33: bool StarsPosition::IsValid() const { On 2014/09/05 03:52:02, Yaron wrote: > nit: ordering should match header file Done. https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:47: DCHECK(from_index < other.size()); On 2014/09/05 03:52:02, Yaron wrote: > DCHECK_LT (and similar changes throughout) Done. https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.cc:67: // This will cause the returned string will be twice as long as the input one, On 2014/09/05 03:52:02, Yaron wrote: > Needs slight reversing. "returned string to be twice" Done. https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.h (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.h:15: class StarsPosition { On 2014/09/05 03:52:02, Yaron wrote: > StarPosition/ItemPosition? Why is it plural? Because it refers to the Stars name, and is stored under the key 'stars.position' (for folders). That said, I don't mind changing the name, which do you prefer?
https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.h (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.h:15: class StarsPosition { On 2014/09/05 19:02:30, Rune Fevang wrote: > On 2014/09/05 03:52:02, Yaron wrote: > > StarPosition/ItemPosition? Why is it plural? > > Because it refers to the Stars name, and is stored under the key > 'stars.position' (for folders). That said, I don't mind changing the name, which > do you prefer? It just reads weirdly to me and can be used without the notion of "Stars". I'd say go for ItemPosition.
https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/stars_position.h (right): https://codereview.chromium.org/510543002/diff/60001/components/enhanced_book... components/enhanced_bookmarks/stars_position.h:15: class StarsPosition { On 2014/09/05 19:52:13, Yaron wrote: > On 2014/09/05 19:02:30, Rune Fevang wrote: > > On 2014/09/05 03:52:02, Yaron wrote: > > > StarPosition/ItemPosition? Why is it plural? > > > > Because it refers to the Stars name, and is stored under the key > > 'stars.position' (for folders). That said, I don't mind changing the name, > which > > do you prefer? > > It just reads weirdly to me and can be used without the notion of "Stars". I'd > say go for ItemPosition. Done.
lgtm
On 2014/09/05 21:15:39, Yaron wrote: > lgtm Thanks! Could you submit this for me Yaron? The CQ doesn't work for private issues and I'm not a committer.
On 2014/09/05 21:58:33, Rune Fevang wrote: > On 2014/09/05 21:15:39, Yaron wrote: > > lgtm > > Thanks! > > Could you submit this for me Yaron? The CQ doesn't work for private issues and > I'm not a committer. Or I guess I could re-upload as a new non-private issue and submit from there.
On 2014/09/05 22:02:45, Rune Fevang wrote: > On 2014/09/05 21:58:33, Rune Fevang wrote: > > On 2014/09/05 21:15:39, Yaron wrote: > > > lgtm > > > > Thanks! > > > > Could you submit this for me Yaron? The CQ doesn't work for private issues and > > I'm not a committer. > > Or I guess I could re-upload as a new non-private issue and submit from there. I made it private in case we would discussed internal details in the code review, but I guess we didn't really.
The CQ bit was checked by rfevang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/510543002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by rfevang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/510543002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by rfevang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/510543002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 6e17967ca948d11bdeb9890dad788f4fbf888784
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a3db1c4f5c38b56b7aa7bd7bf8ff48e54e24587a Cr-Commit-Position: refs/heads/master@{#293674} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
