Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567873002:
DevTools: Add ability to add and edit cookies (Closed)
|
Created:
4 years ago by kdzwinel Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add ability to add and edit cookies
How it looks in action: http://imgur.com/a/Oxdze
Couple of notes (1,2,3 were discussed previously with @allada):
1) There is both front-end and back-end validation
2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name)
3) When validation fails, DataGrid node is marked as "unsaved"
4) There can be more than one "unsaved" node at the same time
5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten
6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid
7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors
TODO:
- inline editors
- use multitargetNetworkManager instead of targetManager
BUG=166637
R=allada,phulce
Review-Url: https://codereview.chromium.org/2567873002
Cr-Commit-Position: refs/heads/master@{#446473}
Committed: https://chromium.googlesource.com/chromium/src/+/42d8de0ce3d18a85197a8394be1d21f0aecb6d01
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add front-end validation. Add `startEditingNextEditableColumnOfDataGridNode` method to DataGrid. Fi… #
Total comments: 10
Patch Set 3 : Code review fixes. #
Total comments: 31
Patch Set 4 : Code review fixes - part 2. #
Total comments: 7
Patch Set 5 : Code review fixes - part 3. #
Total comments: 16
Patch Set 6 : Code review fixes - part 4. #Patch Set 7 : Code review fixes - part 5. #
Total comments: 3
Patch Set 8 : Code review fixes - part 6. #Patch Set 9 : Remove EmptyWidget from CookieItemsView. #Patch Set 10 : Show entries that are inactive - do not match current URL or couldn't be saved. Remove flags from c… #Patch Set 11 : Make dirty styles of the DataGrid node more important than it's inactive styles. #Patch Set 12 : Rebase. #Patch Set 13 : Fix formatting. #
Total comments: 9
Patch Set 14 : Do not show cookies for non-http(s) origins. Fix cookie editing for URLs specifying port number. +… #Patch Set 15 : Rebase. #
Total comments: 3
Patch Set 16 : Whitelist file:// & address other review comments #Patch Set 17 : Rebase. #Messages
Total messages: 64 (33 generated)
Description was changed from ========== DevTools: Add ability to add and edit cookies BUG=166637 R=allada,phulce ========== to ========== DevTools: Add ability to add and edit cookies http://imgur.com/a/vedim BUG=166637 R=allada,phulce ==========
Description was changed from ========== DevTools: Add ability to add and edit cookies http://imgur.com/a/vedim BUG=166637 R=allada,phulce ========== to ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/PfhRz Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors BUG=166637 R=allada,phulce ==========
Description was changed from ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/PfhRz Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors BUG=166637 R=allada,phulce ========== to ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors BUG=166637 R=allada,phulce ==========
PTAL I've created a new CL, previous one (for the record) is here: https://codereview.chromium.org/2215773002 This is how I feel about this patch: http://i.imgur.com/uyGIjNq.gif https://codereview.chromium.org/2567873002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:365: if (!domain.startsWith('.')) @allada: > Ok did some research and found that we handle cases where the user would want to specify the specific domain and no sub domains the user must not specify anything in the domain field https://codereview.chromium.org/2567873002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right): https://codereview.chromium.org/2567873002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:370: var textBeforeEditing = /** @type {string} */ (this._editingNode.data[columnId] || ''); This prevents new "creation row" from being created if current "creation row" is still completely empty after user edit. https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:365: if (!domain.startsWith('.')) @allada: > Ok did some research and found that we handle cases where the user would want to specify the specific domain and no sub domains the user must not specify anything in the domain field https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right): https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:316: startEditingNextEditableColumnOfDataGridNode(node, columnIdentifier) { probably the longest method name I ever came up with (and I did some Java in the past), but this method does a very specific thing, and there is already a similar private method called `_startEditingColumnOfDataGridNode` https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:382: var textBeforeEditing = /** @type {string} */ (this._editingNode.data[columnId] || ''); This prevents new "creation row" from being created if current "creation row" is still completely empty after user edit. https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css (right): https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css:251: color: red; I stole these colors from the console's error message, but it might be a good idea to replace them with something that actually says "unsaved" instead of screaming "ERROR!".
Thanks for the updated CL! The automatic filtering by path, domain, and expiration is an awesome feature that feels unfortunately like a bug in practical usage. We might want to introduce some sort of warning down the road that the change you're making will make this item disappear from the editable list (or perhaps enforce path and domain to still be applicable and expires to be in the future) https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:232: this._dataGrid.addCreationNode(false); probably only want to do this if the table is read-only https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:387: var url = new URL(target.inspectedURL()); I don't think it necessarily makes sense to have the default domain be just the domain of the page but rather the domain that we're inspecting in the cookies table (since it could easily be a third party domain that you're currently editing). Could pass this in from front_end/resources/CookieItemsView.js _cookieDomain variable. https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:393: node.data.expires = ''; Why not session? I guess because it auto-updates on refresh to session if `Boolean(node.data.expires) === false`? https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:415: if (data.expires) { Don't we need a `&& data.expires !== 'Session'` here? I suppose the broken parse will handle it below https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:365: if (!domain.startsWith('.')) On 2016/12/11 22:07:07, kdzwinel wrote: > @allada: > > > Ok did some research and found that we handle cases where the user would want > to specify the specific domain and no sub domains the user must not specify > anything in the domain field really? that sounds kinda broken :/ https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css (right): https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css:255: background-color: red; this red is a tad harsh for the entire background IMO, maybe something more like hsl(0, 100%, 70%)?
Thanks for a review! All good points, I've addressed them. On 2016/12/12 at 17:58:40, phulce wrote: > Thanks for the updated CL! The automatic filtering by path, domain, and expiration is an awesome feature that feels unfortunately like a bug in practical usage. We might want to introduce some sort of warning down the road that the change you're making will make this item disappear from the editable list (or perhaps enforce path and domain to still be applicable and expires to be in the future) Things disappearing is definitely not a great UX. Should I tackle that in this CL? Or should I open a new issue for that? Any idea how that warning could look? > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:232: this._dataGrid.addCreationNode(false); > probably only want to do this if the table is read-only > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:387: var url = new URL(target.inspectedURL()); > I don't think it necessarily makes sense to have the default domain be just the domain of the page but rather the domain that we're inspecting in the cookies table (since it could easily be a third party domain that you're currently editing). Could pass this in from front_end/resources/CookieItemsView.js _cookieDomain variable. > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:393: node.data.expires = ''; > Why not session? I guess because it auto-updates on refresh to session if `Boolean(node.data.expires) === false`? > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:415: if (data.expires) { > Don't we need a `&& data.expires !== 'Session'` here? I suppose the broken parse will handle it below > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:365: if (!domain.startsWith('.')) > On 2016/12/11 22:07:07, kdzwinel wrote: > > @allada: > > > > > Ok did some research and found that we handle cases where the user would want > > to specify the specific domain and no sub domains the user must not specify > > anything in the domain field > > really? that sounds kinda broken :/ > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css (right): > > https://codereview.chromium.org/2567873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css:255: background-color: red; > this red is a tad harsh for the entire background IMO, maybe something more like hsl(0, 100%, 70%)?
Description was changed from ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors BUG=166637 R=allada,phulce ========== to ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors BUG=166637 R=allada,phulce ==========
allada@chromium.org changed reviewers: + allada@chromium.org - allada@google.com
> Things disappearing is definitely not a great UX. Should I tackle that in this > CL? Or should I open a new issue for that? Any idea how that warning could look? Looks good mostly, lets make sure that we send the cookie when a new cookie line is entered or the focus is blured. After that lets then focus on other UI bugs and during that time we'll do general code quality review. Thanks! https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:41: constructor(expandable, refreshCallback, selectedCallback, cookieDomain) { Lets rename "expandable" to "readOnly" since that's what it does now. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:57: editable: true This should be based on this._readOnly https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:88: this._lastEditedColumnId = /** @type {?string} */ (null); nit: Rather than type casting it like his lets declare what type it is using something like: /** @type {?string} */ this._lastEditedColumnId = null; https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:126: sameSiteSubmenu.appendCheckboxItem( Lets remove this as a checkbox item and just make it a submenuitem. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:138: sameSiteSubmenu.appendCheckboxItem( We should add one more called something like "No Restriction". This is default and is set by not setting anything in SameSite param in browser_protocol. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:373: _onUpdateCookie(editingNode, columnIdentifier, oldText, newText) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:382: _setDefaults(node) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:395: _saveNode(node) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:398: if (oldCookie && (newCookie.name() !== oldCookie.name() || newCookie.url() !== oldCookie.url())) oldCookie.remove(); nit: oldCookie.remove() should be on a new line. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:409: _createCookieFromData(data) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:410: var target = SDK.targetManager.targets(SDK.Target.Capability.Network)[0]; We don't want to use targetManager here we want to use MultitargetNetworkManager. I'd suggest adding an entry here (near the line number of the link). In multitarget file we should only dispatch to one agent instead of all agents. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:415: var secondsSinceEpoch = Date.parse(data.expires) / 1000; nit: we should floor this. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:428: _isValidCookieData(data) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:432: _isValidDomain(domain) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:433: if (domain === '') nit: We prefer if (!domain) https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:435: var url = null; nit: Lets move the definition down into the try catch. This will ensure closure compiler will know what type it is. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:437: url = new URL('http://' + domain); Lets use ParsedURL instead. We should be able to get rid of the try-catch. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:444: _isValidPath(path) { nit: doctype please. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:445: var url = null; nit: diddo from above. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:447: url = new URL('http://example.com' + path); see above. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:454: _isValidDate(date) { nit: doctype please.
I've addressed most of the issues found, but I'll need a bit more clarification regarding MultitargetNetworkManager. Thanks! https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:88: this._lastEditedColumnId = /** @type {?string} */ (null); On 2016/12/13 at 01:34:31, allada wrote: > nit: Rather than type casting it like his lets declare what type it is using something like: > > /** @type {?string} */ > this._lastEditedColumnId = null; Makes sense! I wonder why `_nextSelectedCookie` above does type casting? https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:138: sameSiteSubmenu.appendCheckboxItem( On 2016/12/13 at 01:34:31, allada wrote: > We should add one more called something like "No Restriction". This is default and is set by not setting anything in SameSite param in browser_protocol. Isn't that the same I called '<empty>' above? (I'll rename it) https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:373: _onUpdateCookie(editingNode, columnIdentifier, oldText, newText) { On 2016/12/13 at 01:34:31, allada wrote: > nit: doctype please. Sure! I can't figure out the pattern here though. Not all methods have doctype, I thought private ones don't, but now I see that some of them do. In other words, why we don't need a doctype on `_totalSize` or `_onDeleteCookie`, but want it on `_onUpdateCookie`? https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:410: var target = SDK.targetManager.targets(SDK.Target.Capability.Network)[0]; On 2016/12/13 at 01:34:30, allada wrote: > We don't want to use targetManager here we want to use MultitargetNetworkManager. I'd suggest adding an entry here (near the line number of the link). In multitarget file we should only dispatch to one agent instead of all agents. > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... So instead of getting a first target from targetManager, I should get a first network agent from MultitargetNetworkManager? SDK.Cookie takes targets in the constructor, but this can be replaced with Protocol.NetworkAgent.
Awesome! Hope this clarifies much of the questions. Thanks! https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:88: this._lastEditedColumnId = /** @type {?string} */ (null); On 2016/12/14 01:32:36, kdzwinel wrote: > On 2016/12/13 at 01:34:31, allada wrote: > > nit: Rather than type casting it like his lets declare what type it is using > something like: > > > > /** @type {?string} */ > > this._lastEditedColumnId = null; > > Makes sense! I wonder why `_nextSelectedCookie` above does type casting? It tells closure that this variable is a type... The other way implies it from inference. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:126: sameSiteSubmenu.appendCheckboxItem( On 2016/12/13 01:34:31, allada wrote: > Lets remove this as a checkbox item and just make it a submenuitem. Lets move it back to checkbox... I misread the hierarchy. (keep the rename though) https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:138: sameSiteSubmenu.appendCheckboxItem( On 2016/12/14 01:32:36, kdzwinel wrote: > On 2016/12/13 at 01:34:31, allada wrote: > > We should add one more called something like "No Restriction". This is default > and is set by not setting anything in SameSite param in browser_protocol. > > Isn't that the same I called '<empty>' above? (I'll rename it) Yes, I see. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:373: _onUpdateCookie(editingNode, columnIdentifier, oldText, newText) { On 2016/12/14 01:32:36, kdzwinel wrote: > On 2016/12/13 at 01:34:31, allada wrote: > > nit: doctype please. > > Sure! I can't figure out the pattern here though. Not all methods have doctype, > I thought private ones don't, but now I see that some of them do. In other > words, why we don't need a doctype on `_totalSize` or `_onDeleteCookie`, but > want it on `_onUpdateCookie`? It's because old code was not doctyped. We are trying to get everything doctyped, so all future code should be. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:410: var target = SDK.targetManager.targets(SDK.Target.Capability.Network)[0]; On 2016/12/14 01:32:36, kdzwinel wrote: > On 2016/12/13 at 01:34:30, allada wrote: > > We don't want to use targetManager here we want to use > MultitargetNetworkManager. I'd suggest adding an entry here (near the line > number of the link). In multitarget file we should only dispatch to one agent > instead of all agents. > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > > So instead of getting a first target from targetManager, I should get a first > network agent from MultitargetNetworkManager? > SDK.Cookie takes targets in the constructor, but this can be replaced with > Protocol.NetworkAgent. I mean, we should add a new function/method in multitargetNetworkAgent that we can call and let it figure out which target to send it to, similar to how clearBrowserCookies() works in that file. https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:57: editable: true Shouldn't we still address this? by setting it when it's readonly? I am not 100% sure how editable works in DataGrid, I thought we should always set it to false if it's readonly. https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:461: return !!parsedURL && parsedURL.domain() === domain; Any reason we are double-banging this? I thought it'll be null or an object always? I think we should just get rid of the double bang as it'll do the same thing. https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:470: return !!parsedURL && parsedURL.path === path; Same as above. https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:478: return date === '' || date === Common.UIString('Session') || !isNaN(Date.parse(date)); I think we can just this here: return !isNaN(Date.parse(date));
Thanks for all the clarifications! I've replied to your comments. https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:410: var target = SDK.targetManager.targets(SDK.Target.Capability.Network)[0]; On 2016/12/14 at 03:04:15, allada wrote: > On 2016/12/14 01:32:36, kdzwinel wrote: > > On 2016/12/13 at 01:34:30, allada wrote: > > > We don't want to use targetManager here we want to use > > MultitargetNetworkManager. I'd suggest adding an entry here (near the line > > number of the link). In multitarget file we should only dispatch to one agent > > instead of all agents. > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > > > > So instead of getting a first target from targetManager, I should get a first > > network agent from MultitargetNetworkManager? > > SDK.Cookie takes targets in the constructor, but this can be replaced with > > Protocol.NetworkAgent. > > I mean, we should add a new function/method in multitargetNetworkAgent that we can call and let it figure out which target to send it to, similar to how clearBrowserCookies() works in that file. Sorry, I still do not follow :/ `clearBrowserCookies` doesn't really figure out anything, it just calls `clearBrowserCache` on all available agents. When saving a cookie I shouldn't call it on all targets AFAIK. So, which target is "the right one"? target.name() === "Main"? https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:57: editable: true On 2016/12/14 at 03:04:15, allada wrote: > Shouldn't we still address this? by setting it when it's readonly? I am not 100% sure how editable works in DataGrid, I thought we should always set it to false if it's readonly. Ah! It wasn't intended. I just missed this one. https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:461: return !!parsedURL && parsedURL.domain() === domain; On 2016/12/14 at 03:04:15, allada wrote: > Any reason we are double-banging this? I thought it'll be null or an object always? I think we should just get rid of the double bang as it'll do the same thing. without the double-bang this method may return null (when parsing fails and parsedURL is null). IMO `is*` methods should always return boolean and, as far as I can see, there are no `is*` methods in the DT codebase that return {?boolean}. https://codereview.chromium.org/2567873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:478: return date === '' || date === Common.UIString('Session') || !isNaN(Date.parse(date)); On 2016/12/14 at 03:04:15, allada wrote: > I think we can just this here: > return !isNaN(Date.parse(date)); This will cause empty string and `Common.UIString('Session')` to be considered invalid. I don't think we want that?
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
Can we add checkboxes to DataGrid before landing this? It would be so much better! https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:484: return date === '' || date === Common.UIString('Session') || !isNaN(Date.parse(date)); Let's have a constant for UIString('Session'). https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:106: const parsedURL = this._cookieDomain.asParsedURL(); Note that asParsedURL can return null. https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:253: * @return {!Protocol.Network.CookieSameSite<string>} Never seen this syntax. What does it mean? https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:1370: set unsaved(x) { We don't use getters and setters in new code. https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css:251: color: red; This color is too bright.
@dgozman thanks for input! I've updated the code accordingly PTAL. Regarding inline editors - we need both checkboxes and selects in the DataGrid. I'll try to take a look at this over the weekend. TODO: - inline editors - use multitargetNetworkManager instead of targetManager https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:253: * @return {!Protocol.Network.CookieSameSite<string>} On 2016/12/14 at 17:20:21, dgozman wrote: > Never seen this syntax. What does it mean? AFAIK it means that we will return a 'Protocol.Network.CookieSameSite' which effectively is a string. At some point I had a fight over using {Protocol.Network.CookieSameSite} with a closure compiler, so I added <string> part after seeing it here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... I just tested, and <string> is no longer necessary, so I'll remove it. https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:1370: set unsaved(x) { On 2016/12/14 at 17:20:21, dgozman wrote: > We don't use getters and setters in new code. I replaced these two with isUnsaved() and setUnsaved(x) https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/dataGrid.css:251: color: red; On 2016/12/14 at 17:20:21, dgozman wrote: > This color is too bright. I took this background+text color combination from the console's error message to avoid adding any new colors, but I'll be happy to change it. How about something like this: http://imgur.com/a/ue2GH (top one is selected, bottom one is not).
Overall is looking really good! There are some core issues that I am trying to keep you from having to deal with, but it's our style to fix design issues before hacking around them, and you are starting to get into some of them. After some of these code comments are addressed we'll do some UI testing and give some feedback from our UI person. Thanks a lot! https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:253: * @return {!Protocol.Network.CookieSameSite<string>} On 2016/12/15 12:38:50, kdzwinel wrote: > On 2016/12/14 at 17:20:21, dgozman wrote: > > Never seen this syntax. What does it mean? > > AFAIK it means that we will return a 'Protocol.Network.CookieSameSite' which > effectively is a string. At some point I had a fight over using > {Protocol.Network.CookieSameSite} with a closure compiler, so I added <string> > part after seeing it here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > > > I just tested, and <string> is no longer necessary, so I'll remove it. Lets focus on making this work for now. Please add a todo for me as follows: // TODO(allada) This should not rely on _attributes and instead store them individually. on the line above the doctype. Then just use: @return {!Protocol.Network.CookieSameSite} (and below) [I started a patch that will update this area to modernize the code] https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:256: return this._attributes['samesite']; change this to: return /** @type {!Protocol.Network.CookieSameSite} */ (this._attributes['samesite']); https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:290: * @return {number} We also parse headers out of this from network panel. This means that if a cookie is set with an expires date in the header of a request and we display it in the network panel it might do different than expected stuff. I would suggest putting a condition where it sets the expiration in your code to convert it into RFC5322 (https://tools.ietf.org/html/rfc5322#section-3.6.1) format (what Date() uses) and convert to and from the string format. https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:346: * @param {string|number=} value I have some code that is going to be changing this a lot. If it doesn't make the rest of the code bad, lets just convert anything that would come in as a number to a string instead. https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:364: // for cookies targeting single domain and no sub domains 'domain' field should be sent empty I would suggest removing this code as it is not relevant to devtools. But, for future reference we start with caps and end in period for comments. :-) https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:1370: set unsaved(x) { On 2016/12/15 12:38:50, kdzwinel wrote: > On 2016/12/14 at 17:20:21, dgozman wrote: > > We don't use getters and setters in new code. > > I replaced these two with isUnsaved() and setUnsaved(x) Can we use isDirty and setDirty() instead? It is the naming we use in UISourceCode.
Description was changed from ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors BUG=166637 R=allada,phulce ========== to ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors TODO: - inline editors - use multitargetNetworkManager instead of targetManager BUG=166637 R=allada,phulce ==========
@allada thank you for your comments! I've adjusted the code accordingly. BTW I still need a bit more explanation regarding multitargetNetworkAgent: > > I mean, we should add a new function/method in multitargetNetworkAgent that we can call and let it figure out which target to send it to, similar to how clearBrowserCookies() works in that file. > Sorry, I still do not follow :/ `clearBrowserCookies` doesn't really figure out anything, it just calls `clearBrowserCache` on all available agents. When saving a cookie I shouldn't call it on all targets AFAIK. So, which target is "the right one"? target.name() === "Main"? https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:290: * @return {number} On 2016/12/15 at 23:45:52, allada wrote: > We also parse headers out of this from network panel. This means that if a cookie is set with an expires date in the header of a request and we display it in the network panel it might do different than expected stuff. > > I would suggest putting a condition where it sets the expiration in your code to convert it into RFC5322 (https://tools.ietf.org/html/rfc5322#section-3.6.1) format (what Date() uses) and convert to and from the string format. `expires` comes as a number from the back-end (`Network.getCookies`) and is also expected to be a number when saving a cookie (`Network.setCookie`), so IMO I'd be better to have a number here and fix the request header parser instead. Please also note that `this._attributes['expires']` already stores numbers, not strings (as jsdoc suggests), on the application panel. It only does strings on the network panel. Since, as I understand form your comment below, you will be rewriting this part of the code anyway, I switched the type back to string and updated `SDK.Cookies._parseProtocolCookie` to make sure that `expires` is a stirng in both network and application panel.
This is looking really good! I applied it today and really like it. There are some UI aspects that will likely need to be tweaked, but we can deal with that as a last measure. When dgozman@ (our Tech Lead) gets back into office I'll have him do the final code review, but chowse@ (our UI/UX Expert) may have some other change requests. For me it's lgtm after the nits are addressed. But you'll need to get chowse@ and dgozman@ to sign off on it too. Thanks a ton! https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:290: * @return {number} On 2016/12/19 01:55:04, kdzwinel wrote: > On 2016/12/15 at 23:45:52, allada wrote: > > We also parse headers out of this from network panel. This means that if a > cookie is set with an expires date in the header of a request and we display it > in the network panel it might do different than expected stuff. > > > > I would suggest putting a condition where it sets the expiration in your code > to convert it into RFC5322 (https://tools.ietf.org/html/rfc5322#section-3.6.1) > format (what Date() uses) and convert to and from the string format. > > `expires` comes as a number from the back-end (`Network.getCookies`) and is also > expected to be a number when saving a cookie (`Network.setCookie`), so IMO I'd > be better to have a number here and fix the request header parser instead. > Please also note that `this._attributes['expires']` already stores numbers, not > strings (as jsdoc suggests), on the application panel. It only does strings on > the network panel. Since, as I understand form your comment below, you will be > rewriting this part of the code anyway, I switched the type back to string and > updated `SDK.Cookies._parseProtocolCookie` to make sure that `expires` is a > stirng in both network and application panel. Ok, lets keep it a number since 'expires' already is a number and we'll just be extra careful when showing to users. https://codereview.chromium.org/2567873002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:253: * TODO(allada) This should not rely on _attributes and instead store them individually. nit: We try not to add any non-jsdoc stuff in /** comments, can we move this just above "return" below? https://codereview.chromium.org/2567873002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right): https://codereview.chromium.org/2567873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:1386: setDirty(x) { nit: We are trying not to do this anymore and name all parameters. Can we change "x" to "dirty"? https://codereview.chromium.org/2567873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:1387: this._dirty = x; nit: We usually check if we need to change the value then set it if we do. eg: if (this._dirty === dirty) return; this._dirty = dirty;
The CQ bit was checked by allada@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...
I am going to also run a buildbot dryrun to check for any tests that might need to be changed too. Don't be afraid if any of them are red, it's very common.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by allada@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.
I'm back after a short break! @allada thank you again for your review, I've addressed all your comments, rebased the code and removed UI.EmptyWidget ( https://imgur.com/JikSux0 ) from the CookieItemsView as it was making it imposible to add a new cookie on a website w/o any cookies (e.g. example.com). Now, the DataGrid is always shown behaving the same as DOMStorageItemsView.
So, I've played with this a bit, and I don't like that cookie disappears if I edit it in some ways. For example, changing domain or path to not match current page url, changing secure bit. Let's instead turn such a cookie gray to indicate it's not "alive", but keep it until I hit refresh button. Also, let's just remove flags from context menus until we have proper checkbox/select in the datagrid instead.
Thanks for checking it out! I've removed flags from context menus and introduced 'inactive' DataGrid fields. These fields look like this: https://imgur.com/a/oroQC (styles based on the 'console info'). Whenever just created, or edited, cookie doesn't come back from the backend after being saved I'm appending it to the DataGrid and marking as inactive. This happens when domain or path doesn't match the current URL, but also e.g. when cookie expiry date is in the past. Inactive cookie can be edited and 'fixed', causing it to go back to the 'active' state. Inactive node, similarly to a 'dirty' (or invalid) node, is very fragile - reloading the cookies, but also, filtering, sorting or modifying them removes the 'inactive' node. If node is both 'dirty' and 'inactive', 'dirty' styles take precedence (node is painted red). PTAL
On 2017/01/16 02:14:41, kdzwinel wrote: > Thanks for checking it out! > > I've removed flags from context menus and introduced 'inactive' DataGrid fields. > These fields look like this: https://imgur.com/a/oroQC (styles based on the > 'console info'). Whenever just created, or edited, cookie doesn't come back from > the backend after being saved I'm appending it to the DataGrid and marking as > inactive. This happens when domain or path doesn't match the current URL, but > also e.g. when cookie expiry date is in the past. Inactive cookie can be edited > and 'fixed', causing it to go back to the 'active' state. > > Inactive node, similarly to a 'dirty' (or invalid) node, is very fragile - > reloading the cookies, but also, filtering, sorting or modifying them removes > the 'inactive' node. > > If node is both 'dirty' and 'inactive', 'dirty' styles take precedence (node is > painted red). > > PTAL Sounds good! Unfortunately, I wasn't able to apply the patch to try it. Could you please rebase?
Rebased! PTAL
Works great! https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:220: this._addInactiveNode(this._dataGrid.rootNode(), selectedCookie, lastEditedColumnId); style: 2 spaces indent https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:399: node.data.domain = this._cookieDomain; You should strip down the port number. Try adding a cookie on localhost:1234 - it breaks. https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css (right): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css:265: background-color: hsl(0, 100%, 70%); Let's not make it gray, having italics and/or gray text would be enough. https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (left): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:60: cookieDomain ? Let's only show cookies for http:// and https:// origins (not for file: or data: for example). https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:361: * @param {function(?Protocol.Error, boolean)=} callback You should swallow Protocol.Error in this method, so that callback only gets a boolean.
All comments addressed. PTAL https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:220: this._addInactiveNode(this._dataGrid.rootNode(), selectedCookie, lastEditedColumnId); On 2017/01/20 at 01:54:51, dgozman wrote: > style: 2 spaces indent thanks! It looks like linter isn't configured to report indentation errors yet. https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:399: node.data.domain = this._cookieDomain; On 2017/01/20 at 01:54:51, dgozman wrote: > You should strip down the port number. Try adding a cookie on localhost:1234 - it breaks. I haven't realized that `"...".asParsedURL().domain()` result contains a port number! I replaced it with `.host`. https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css (right): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css:265: background-color: hsl(0, 100%, 70%); On 2017/01/20 at 01:54:51, dgozman wrote: > Let's not make it gray, having italics and/or gray text would be enough. This particular line sets a background color on the selected 'dirty' item (which is red). You probably meant the gray background color of the selected 'inactive' element, correct? I removed it, now the selected 'inactive' element looks like this: http://i.imgur.com/bsd5LyL.png . https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (left): https://codereview.chromium.org/2567873002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:60: cookieDomain ? On 2017/01/20 at 01:54:51, dgozman wrote: > Let's only show cookies for http:// and https:// origins (not for file: or data: for example). It looks like it's possible to set cookies on file: origins with `--enable-file-cookies` flag, are we sure we don't want to show them? I was thinking about bringing EmptyWidget back but I found out that for data URLs CookieItemsView isn't even created (which looks like this: http://i.imgur.com/47DLAz8.png ). So, I expanded the same solution to all non-http(s) origins (see ResourcesPanel.js). WDYT?
lgtm https://codereview.chromium.org/2567873002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2567873002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:395: if (!parsedURL || (parsedURL.scheme !== 'http' && parsedURL.scheme !== 'https')) Let's whitelist file://, just in case someone runs with a flag. https://codereview.chromium.org/2567873002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js (right): https://codereview.chromium.org/2567873002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:311: url() { @return {string} https://codereview.chromium.org/2567873002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js:378: callback(success); callback(error ? false : success)
I've fixed remaining issues. Thank you for your patience and help!
The CQ bit was checked by allada@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...
Awesome! Looks great. I am running them on the bots again, assuming everything comes back green you'll be gtg. Are you able to click the "commit" checkbox yourself? If you are not I'll do it for you. Thanks again!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kdzwinel@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2567873002/#ps320001 (title: "Rebase.")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdzwinel@gmail.com
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdzwinel@gmail.com
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by allada@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.
The CQ bit was checked by kdzwinel@gmail.com
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": 320001, "attempt_start_ts": 1485469745974570,
"parent_rev": "9874e16e6ccd443a33d4207619e87c6fc6ad1dcd", "commit_rev":
"42d8de0ce3d18a85197a8394be1d21f0aecb6d01"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors TODO: - inline editors - use multitargetNetworkManager instead of targetManager BUG=166637 R=allada,phulce ========== to ========== DevTools: Add ability to add and edit cookies How it looks in action: http://imgur.com/a/Oxdze Couple of notes (1,2,3 were discussed previously with @allada): 1) There is both front-end and back-end validation 2) Cookie is saved with default values (domain = current, path = /, expires = session) as soon as user fills out the first field (name) 3) When validation fails, DataGrid node is marked as "unsaved" 4) There can be more than one "unsaved" node at the same time 5) Any action that causes cookies to reload from back-end (e.g. refresh, delete, changed tab) causes all "unsaved" changes to be forgotten 6) Validation allows all valid domains, paths and dates. If user sets a domain or path different from the target's, or date that is in the past, node will disappear from the DataGrid 7) Setting flags (HTTP, Secure, SameSite) is done via Context Menu because ATM there are no inline checkbox/select editors TODO: - inline editors - use multitargetNetworkManager instead of targetManager BUG=166637 R=allada,phulce Review-Url: https://codereview.chromium.org/2567873002 Cr-Commit-Position: refs/heads/master@{#446473} Committed: https://chromium.googlesource.com/chromium/src/+/42d8de0ce3d18a85197a8394be1d... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/42d8de0ce3d18a85197a8394be1d... |
