Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2379913003:
Site Settings Desktop: Polish the Site Data details dialog. (Closed)
|
Created:
4 years, 2 months ago by Finnur Modified:
4 years, 2 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSite Settings Desktop: Polish the Site Data details dialog.
BUG=633070
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6b3fff125d013deb5453241246baf3dafd7ddf6b
Cr-Commit-Position: refs/heads/master@{#422077}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address feedback #
Messages
Total messages: 19 (12 generated)
Description was changed from ========== Site Settings Desktop: Polish the Site Data details dialog. BUG=633070 ========== to ========== Site Settings Desktop: Polish the Site Data details dialog. BUG=633070 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by finnur@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...
Patchset #1 (id:1) has been deleted
finnur@chromium.org changed reviewers: + dschuyler@chromium.org
I'll annotate this a bit in a follow-up in case the fixes aren't immediately obvious.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:182: dataType = descriptionNode.children_[0].data_.type; This is not listed in the bug, but was discovered while debugging the original issue. Some background: A CookieTreeNode for a site consists of a root node for the site and a container node (called descriptionNode here) that contains all the actual data nodes of a specific type, i.e. ChannelID. As in: - Site - Generic container - Leaf node (Cookie) - Leaf node (Cookie) - Generic container - Leaf node (ChannelID) Each time a leaf node is deleted, the underlying Site Data list gets notified and updates the summary list. But when the last/only data node within a container node is deleted by the Site Data Dialog, the container node is also deleted. This means we'll get two delete notifications: One for the leaf node and another for the container (in that order). However, since we label the container nodes (in the summary list) by the data type of the nodes it contains we get an error in the console about 'data_ not being defined on undefined'. Which makes sense, because before the second delete comes in we'll have a container node that is empty but about to be destroyed. But since the container node is empty and therefore has no data type, the data type can be blank. https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data_details_dialog.html (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:17: } This is mentioned in passing in the bug -- the remove button was out of alignment with the combobox and with the button at the bottom right corner. https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:74: }, This was also not mentioned in the bug, but discovered while debugging. This change is to get rid of an error in the console complaining about HTMLDialogElement not having an open property and therefore can't be closed. I took a look at how another dialog does this (bluetooth) and did the same here. https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:148: if (this.site_.children_.length == 0 || this.entries_.length == 0) { This is to fix the bug with the dialog closing prematurely (see comment 2 of the bug). Before closing the dialog we should be checking to see if site_ has any remaining children left, because, when a container node is deleted, the onTreeItemRemoved_ args will say a node with ID 'foo' deleted nodes n to n+x. When a container node is deleted (as opposed to a leaf), the ID provided is always going to match the top level (site) ID, so the old code thought that the site itself had been deleted, when in fact only a child of site had been deleted.
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
If the comment that starts with "Please let me know if this doesn't work..." works out (or something in a similar vein) then LGTM. If not, then please let me know so we can look at again (rather than keeping the -7px; changes). https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:178: else { If the |else| has braces {} the |if| should also (even though it's a one line block). https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:180: // being deleted. optional: // A description node might not have children when it's deleted. (The 'can have no' on line 179 could mean 'might not have' or it could mean 'absolutely will not have'. The difference can be discerned by context, so it's still correct to leave it as is. It's just nicer to not have to work out what is intended). :) https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:182: dataType = descriptionNode.children_[0].data_.type; On 2016/09/29 15:54:01, Finnur wrote: > This is not listed in the bug, but was discovered while debugging the original > issue. Some background: > > A CookieTreeNode for a site consists of a root node for the site and a container > node (called descriptionNode here) that contains all the actual data nodes of a > specific type, i.e. ChannelID. > > As in: > > - Site > - Generic container > - Leaf node (Cookie) > - Leaf node (Cookie) > - Generic container > - Leaf node (ChannelID) > > Each time a leaf node is deleted, the underlying Site Data list gets notified > and updates the summary list. But when the last/only data node within a > container node is deleted by the Site Data Dialog, the container node is also > deleted. > > This means we'll get two delete notifications: One for the leaf node and another > for the container (in that order). However, since we label the container nodes > (in the summary list) by the data type of the nodes it contains we get an error > in the console about 'data_ not being defined on undefined'. Which makes sense, > because before the second delete comes in we'll have a container node that is > empty but about to be destroyed. But since the container node is empty and > therefore has no data type, the data type can be blank. Thanks for the explanation! https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data_details_dialog.html (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:17: } On 2016/09/29 15:54:01, Finnur wrote: > This is mentioned in passing in the bug -- the remove button was out of > alignment with the combobox and with the button at the bottom right corner. Please let me know if this doesn't work, but I think this would be better solved by - remove the shift by -7px; in both places above. - removing the margin on the paper-button (that is inside the button-container) - adding "align-self: center;" to the paper-dropdown-menu-light - adding "display: flex;" to the div just above paper-dropdown-menu-light (I tried that locally, and it looks nice imo). That should line the pieces up nicely, with well formed rectangles and centering, which (I'm hoping) will be more robust against other changes in the UI (more robust than fixed values shifting of the pieces). https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:74: }, On 2016/09/29 15:54:01, Finnur wrote: > This was also not mentioned in the bug, but discovered while debugging. > > This change is to get rid of an error in the console complaining about > HTMLDialogElement not having an open property and therefore can't be closed. I > took a look at how another dialog does this (bluetooth) and did the same here. I think this can be inlined above like: var dialog = /** @type {!CrDialogElement} */(this.getDialog_()); If that works, could we skip having this function? https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:148: if (this.site_.children_.length == 0 || this.entries_.length == 0) { On 2016/09/29 15:54:01, Finnur wrote: > This is to fix the bug with the dialog closing prematurely (see comment 2 > of the bug). > > Before closing the dialog we should be checking to see if site_ has any > remaining children left, because, when a container node is deleted, the > onTreeItemRemoved_ args will say a node with ID 'foo' deleted nodes > n to n+x. > > When a container node is deleted (as opposed to a leaf), the ID provided > is always going to match the top level (site) ID, so the old code thought that > the > site itself had been deleted, when in fact only a child of site had been > deleted. Acknowledged.
https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/cookie_tree_node.js (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:178: else { Ah, yes, I indeed. https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/cookie_tree_node.js:180: // being deleted. On 2016/09/29 19:40:27, dschuyler wrote: > optional: > // A description node might not have children when it's deleted. > > (The 'can have no' on line 179 could mean > 'might not have' or it could > mean 'absolutely will not have'. The difference > can be discerned by context, so it's still correct > to leave it as is. It's just nicer to not have > to work out what is intended). :) Done. https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data_details_dialog.html (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:17: } Yup. Looks fine. Thanks. https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js (right): https://codereview.chromium.org/2379913003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:74: }, Done, with minor modifications. :)
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2379913003/#ps40001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Site Settings Desktop: Polish the Site Data details dialog. BUG=633070 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Polish the Site Data details dialog. BUG=633070 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6b3fff125d013deb5453241246baf3dafd7ddf6b Cr-Commit-Position: refs/heads/master@{#422077} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6b3fff125d013deb5453241246baf3dafd7ddf6b Cr-Commit-Position: refs/heads/master@{#422077} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
