Site Settings: Implement dialog for adding site exceptions.

Site Settings: Implement dialog for adding site exceptions.

BUG=543635
Committed: https://crrev.com/386784d9b65071d107fbb98fb4004fb633d0e17d
Cr-Commit-Position: refs/heads/master@{#389728}

[Finnur, 2016-04-22 15:50:02]

finnur@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-04-24 19:58:40]

mostly nits, but it would be nice if you could create the lazy-dialog in JS
instead of always loading it in HTML

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/add_site_dialog.html
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.html:12: <div
class="title" i18n-content="addSiteHeader"></div>
since this is new code, could you use $i18n{} instead of i18n-foo?

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:9: *
nit: remove blank line

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:17: * Opens
the dialog.
nit: /** Opens... */

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:40:
ensurePatternWildcard_: function(pattern) {
nit... "ensure" is misleading, makes me think this checks if the subdomain
wildcard is already there when really this always adds it. "addWildcard" or
something would be less confusing

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:55: var
pattern = this.ensurePatternWildcard_(this.site_);
add "site_: String" to |properties|, please

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:59: // Make
sure the text box is empty when you open it next time.
not needed if you create/remove the dialog on demand :-)

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:22:
<add-site-dialog id="dialog" category="[[category]]"></add-site-dialog>
can you wrap this in a dom-if, or better yet, create it dynamically?
eg:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:48:
<paper-button on-tap="onAddSiteTap_" class="primary-button"
indented too far

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:118:
*/
@private

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:89:
* @return {Promise<boolean>} True if the pattern is valid.
nit: !Promise

[michaelpg, 2016-04-24 19:58:57]

On 2016/04/24 19:58:40, michaelpg wrote:
> mostly nits, but it would be nice if you could create the lazy-dialog in JS

*lazy-create the dialog

> instead of always loading it in HTML
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/site_settings/add_site_dialog.html
> (right):
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/add_site_dialog.html:12: <div
> class="title" i18n-content="addSiteHeader"></div>
> since this is new code, could you use $i18n{} instead of i18n-foo?
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/site_settings/add_site_dialog.js
(right):
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/add_site_dialog.js:9: *
> nit: remove blank line
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/add_site_dialog.js:17: * Opens
> the dialog.
> nit: /** Opens... */
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/add_site_dialog.js:40:
> ensurePatternWildcard_: function(pattern) {
> nit... "ensure" is misleading, makes me think this checks if the subdomain
> wildcard is already there when really this always adds it. "addWildcard" or
> something would be less confusing
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/add_site_dialog.js:55: var
> pattern = this.ensurePatternWildcard_(this.site_);
> add "site_: String" to |properties|, please
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/add_site_dialog.js:59: // Make
> sure the text box is empty when you open it next time.
> not needed if you create/remove the dialog on demand :-)
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> File
chrome/browser/resources/settings/site_settings/site_settings_category.html
> (right):
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/site_settings/site_settings_category.html:22:
> <add-site-dialog id="dialog" category="[[category]]"></add-site-dialog>
> can you wrap this in a dom-if, or better yet, create it dynamically?
> eg:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/site_settings/site_settings_category.html:48:
> <paper-button on-tap="onAddSiteTap_" class="primary-button"
> indented too far
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/site_settings/site_settings_category.js
> (right):
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/site_settings_category.js:118:
> */
> @private
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
> File
>
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
> (right):
> 
>
https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:89:
> * @return {Promise<boolean>} True if the pattern is valid.
> nit: !Promise

[Finnur, 2016-04-25 10:23:13]

All done. PTAL.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/add_site_dialog.html
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.html:12: <div
class="title" i18n-content="addSiteHeader"></div>
On 2016/04/24 19:58:39, michaelpg wrote:
> since this is new code, could you use $i18n{} instead of i18n-foo?

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:9: *
On 2016/04/24 19:58:40, michaelpg wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:17: * Opens
the dialog.
On 2016/04/24 19:58:40, michaelpg wrote:
> nit: /** Opens... */

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:40:
ensurePatternWildcard_: function(pattern) {
On 2016/04/24 19:58:40, michaelpg wrote:
> nit... "ensure" is misleading, makes me think this checks if the subdomain
> wildcard is already there when really this always adds it. "addWildcard" or
> something would be less confusing

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:55: var
pattern = this.ensurePatternWildcard_(this.site_);
On 2016/04/24 19:58:40, michaelpg wrote:
> add "site_: String" to |properties|, please

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:59: // Make
sure the text box is empty when you open it next time.
On 2016/04/24 19:58:40, michaelpg wrote:
> not needed if you create/remove the dialog on demand :-)

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:22:
<add-site-dialog id="dialog" category="[[category]]"></add-site-dialog>
On 2016/04/24 19:58:40, michaelpg wrote:
> can you wrap this in a dom-if, or better yet, create it dynamically?
> eg:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:48:
<paper-button on-tap="onAddSiteTap_" class="primary-button"
On 2016/04/24 19:58:40, michaelpg wrote:
> indented too far

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:118:
*/
On 2016/04/24 19:58:40, michaelpg wrote:
> @private

Done.

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/1909413002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:89:
* @return {Promise<boolean>} True if the pattern is valid.
On 2016/04/24 19:58:40, michaelpg wrote:
> nit: !Promise

Done.

[michaelpg, 2016-04-25 17:03:39]

lgtm

[Finnur, 2016-04-26 09:20:13]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-04-26 09:20:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909413002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909413002/60001

[commit-bot: I haz the power, 2016-04-26 09:25:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-26 09:26:27]

Description was changed from

==========
Site Settings: Implement dialog for adding site exceptions.

BUG=543635
==========

to

==========
Site Settings: Implement dialog for adding site exceptions.

BUG=543635

Committed: https://crrev.com/386784d9b65071d107fbb98fb4004fb633d0e17d
Cr-Commit-Position: refs/heads/master@{#389728}
==========

[commit-bot: I haz the power, 2016-04-26 09:26:28]

Patchset 4 (id:??) landed as
https://crrev.com/386784d9b65071d107fbb98fb4004fb633d0e17d
Cr-Commit-Position: refs/heads/master@{#389728}