Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714913002:
DevTools: Fixes to Storage panel inconsistencies (Closed)
|
Created:
3 years, 10 months ago by phulce Modified:
3 years, 9 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Fixes to Storage panel inconsistencies
* Fixes Backspace delete in CookiesTable
* Fixes selection preservation when 'Delete Selected' is used in CookiesTable
* Fixes selection removal when 'Clear All' is used in Storage panes
BUG=694399, 694741
Review-Url: https://codereview.chromium.org/2714913002
Cr-Commit-Position: refs/heads/master@{#454002}
Committed: https://chromium.googlesource.com/chromium/src/+/63095504581ef0e664faf74614e5ca98f9cb9047
Patch Set 1 #Patch Set 2 : fix merge conflicts #
Total comments: 9
Patch Set 3 : more robust selection preservation #
Total comments: 2
Patch Set 4 : more feedback #
Messages
Total messages: 33 (13 generated)
phulce@chromium.org changed reviewers: + eostroukhov@chromium.org
lgtm
lgtm lgtm
phulce@chromium.org changed reviewers: + dgozman@chromium.org
lgtm
The CQ bit was checked by phulce@chromium.org
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fixed merge conflicts, please take another look eostroukhov
https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:116: this._cookiesTable.removeSelectedCookie(); The call ultimately gets back to this view. Maybe delete it here and then refresh the table? https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:70: * @param {!Common.Event=} event I think you can remove the event from the parameter list. https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:210: this._domStorageItemsCleared(); This should be called in response to event after the storage had been cleared. Is there anything broken?
https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:116: this._cookiesTable.removeSelectedCookie(); On 2017/02/24 18:13:46, eostroukhov wrote: > The call ultimately gets back to this view. Maybe delete it here and then > refresh the table? Done, sort key wasn't robust enough to handle all of the identical value cases (domain for example) so went with neighbor preservation. https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:70: * @param {!Common.Event=} event On 2017/02/24 18:13:46, eostroukhov wrote: > I think you can remove the event from the parameter list. Do you think it's better to have no documentation of the inputs when they're unused? https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:210: this._domStorageItemsCleared(); On 2017/02/24 18:13:47, eostroukhov wrote: > This should be called in response to event after the storage had been cleared. > Is there anything broken? AFAICT the target simply doesn't call the methods on the dispatcher if there were no cookies to clear, which isn't necessarily buggy behavior IMO
https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:70: * @param {!Common.Event=} event On 2017/02/28 18:55:53, phulce wrote: > On 2017/02/24 18:13:46, eostroukhov wrote: > > I think you can remove the event from the parameter list. > > Do you think it's better to have no documentation of the inputs when they're > unused? This function is not meant to be called with an argument. https://codereview.chromium.org/2714913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2714913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:416: this._overrideNextSelectedCookie = newCookie; Can you turn this into an optional parameter to _refresh?
https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:70: * @param {!Common.Event=} event On 2017/02/28 20:22:51, eostroukhov wrote: > On 2017/02/28 18:55:53, phulce wrote: > > On 2017/02/24 18:13:46, eostroukhov wrote: > > > I think you can remove the event from the parameter list. > > > > Do you think it's better to have no documentation of the inputs when they're > > unused? > > This function is not meant to be called with an argument. Except it will be when invoked through event listener :) Added a comment to explain why we also clear expliticly https://codereview.chromium.org/2714913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2714913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:416: this._overrideNextSelectedCookie = newCookie; On 2017/02/28 20:22:51, eostroukhov wrote: > Can you turn this into an optional parameter to _refresh? Just removed it entirely since it's not actually necessary for this usage
https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2714913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:70: * @param {!Common.Event=} event On 2017/02/28 21:23:58, phulce wrote: > On 2017/02/28 20:22:51, eostroukhov wrote: > > On 2017/02/28 18:55:53, phulce wrote: > > > On 2017/02/24 18:13:46, eostroukhov wrote: > > > > I think you can remove the event from the parameter list. > > > > > > Do you think it's better to have no documentation of the inputs when they're > > > unused? > > > > This function is not meant to be called with an argument. > > Except it will be when invoked through event listener :) Added a comment to > explain why we also clear expliticly I don't think it matters from the API point of view. You can always pass any number of parameters to any JS function...
lgtm
The CQ bit was checked by phulce@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2714913002/#ps60001 (title: "more feedback")
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_chromium_chromeos_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 phulce@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 phulce@chromium.org
The CQ bit was checked by phulce@chromium.org
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": 60001, "attempt_start_ts": 1488397144307710,
"parent_rev": "25b8bef72694dc18f3dcd8825cce32554f0116cb", "commit_rev":
"63095504581ef0e664faf74614e5ca98f9cb9047"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Fixes to Storage panel inconsistencies * Fixes Backspace delete in CookiesTable * Fixes selection preservation when 'Delete Selected' is used in CookiesTable * Fixes selection removal when 'Clear All' is used in Storage panes BUG=694399,694741 ========== to ========== DevTools: Fixes to Storage panel inconsistencies * Fixes Backspace delete in CookiesTable * Fixes selection preservation when 'Delete Selected' is used in CookiesTable * Fixes selection removal when 'Clear All' is used in Storage panes BUG=694399,694741 Review-Url: https://codereview.chromium.org/2714913002 Cr-Commit-Position: refs/heads/master@{#454002} Committed: https://chromium.googlesource.com/chromium/src/+/63095504581ef0e664faf74614e5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/63095504581ef0e664faf74614e5...
Message was sent while issue was closed.
On 2017/03/01 at 19:50:48, commit-bot wrote: > Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/63095504581ef0e664faf74614e5... This CL breaks 'inactive' cookie functionality. Before: https://i.imgur.com/dAQaY2M.gif , after: https://i.imgur.com/8d5Ytpr.gif .
Message was sent while issue was closed.
On 2017/03/02 at 10:16:25, kdzwinel wrote: > On 2017/03/01 at 19:50:48, commit-bot wrote: > > Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/63095504581ef0e664faf74614e5... > > This CL breaks 'inactive' cookie functionality. Before: https://i.imgur.com/dAQaY2M.gif , after: https://i.imgur.com/8d5Ytpr.gif . I've fixed it here: https://codereview.chromium.org/2726823004 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
