Site Settings Desktop: Fix bug with deleting manually added exceptions.

Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/58420c6d82b10a7d1fcd636694f8b50dae3b3f3b
Cr-Commit-Position: refs/heads/master@{#414698}

[Finnur, 2016-08-23 15:37:21]

Description was changed from

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

BUG=639208
==========

to

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-08-23 15:37:32]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 15:38:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Finnur, 2016-08-23 15:38:24]

Description was changed from

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more 
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-23 16:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 16:24:18]

Dry run: 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_...)

[Finnur, 2016-08-24 12:16:42]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 12:16:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 12:57:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 12:57:34]

Dry run: 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...)

[Finnur, 2016-08-24 16:20:21]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 16:20:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Finnur, 2016-08-24 16:21:31]

finnur@chromium.org changed reviewers:
+ dschuyler@chromium.org, sky@chromium.org

[Finnur, 2016-08-24 16:21:31]

Scott, can you review the changes to test_web_ui.*
Dave, can you review the rest (MD-Settings parts)

https://codereview.chromium.org/2269963003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2269963003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:355:
this.getEmbedderString(embeddingOrigin, this.category);
I really want to use the embedder string also in the page title for Site
Details, so that I can go to Location, click on an entry that looks like this:

  http://foo.com
  embedded on http://bar.com

And see a page title of:
"<- http://foo.com (embedded on http://bar.com)" 
instead of just:
"<- http://foo.com" 

Do we support dynamically changing the page title like that?

[commit-bot: I haz the power, 2016-08-24 18:25:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:25:57]

Dry run: 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...)

[sky, 2016-08-24 19:27:31]

https://codereview.chromium.org/2269963003/diff/40001/content/public/test/tes...
File content/public/test/test_web_ui.h (right):

https://codereview.chromium.org/2269963003/diff/40001/content/public/test/tes...
content/public/test/test_web_ui.h:76: void TakeAsArg4(base::Value* arg);
Seems like this should take a unique_ptr. Is there a reason not to do that now?
I realize the others don't, but that shouldn't stop conversion.

[dschuyler, 2016-08-24 22:27:38]

LGTM with optional nits.

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

https://codereview.chromium.org/2269963003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:56:
exceptionList[i].embeddingOrigin == site.embeddingOrigin) {
Optional:
function calls are slow, so this may be faster
if the tests on either side of the '&&' are reversed.
So that the function call doesn't happen if the first
test fails.

https://codereview.chromium.org/2269963003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:74:
if (this.site == undefined)
Optional: I'd go for === when comparing to undefined.

(if you're really going for 'similar to undefined' 
rather than 'exactly undefined' then maybe go with 
(!this.site) instead).

[Finnur, 2016-08-25 10:28:06]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 10:28:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Finnur, 2016-08-25 10:29:04]

All comments from Dave addressed.

Scott, PTAL.

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

https://codereview.chromium.org/2269963003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:56:
exceptionList[i].embeddingOrigin == site.embeddingOrigin) {
On 2016/08/24 22:27:38, dschuyler wrote:
> Optional:
> function calls are slow, so this may be faster
> if the tests on either side of the '&&' are reversed.
> So that the function call doesn't happen if the first
> test fails.

Done.

https://codereview.chromium.org/2269963003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:74:
if (this.site == undefined)
On 2016/08/24 22:27:38, dschuyler wrote:
> Optional: I'd go for === when comparing to undefined.
> 
> (if you're really going for 'similar to undefined' 
> rather than 'exactly undefined' then maybe go with 
> (!this.site) instead).

Done.

https://codereview.chromium.org/2269963003/diff/40001/content/public/test/tes...
File content/public/test/test_web_ui.h (right):

https://codereview.chromium.org/2269963003/diff/40001/content/public/test/tes...
content/public/test/test_web_ui.h:76: void TakeAsArg4(base::Value* arg);
Sure. Hope I got it right. :)

[commit-bot: I haz the power, 2016-08-25 11:17:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 11:17:39]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2016-08-25 16:10:03]

Nice, LGTM

[Finnur, 2016-08-25 22:52:50]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-08-25 22:52:51]

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/2269963003/#ps60001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-08-25 22:54:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:43:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 00:43:44]

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_...)

[Finnur, 2016-08-26 10:14:53]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-08-26 10:15:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 13:41:42]

Description was changed from

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more 
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more 
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-26 13:41:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-26 13:43:35]

Description was changed from

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more 
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Fix bug with deleting manually added exceptions.

This is accomplished by using the embedderOrigin more
consistently throughout.

BUG=639208
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/58420c6d82b10a7d1fcd636694f8b50dae3b3f3b
Cr-Commit-Position: refs/heads/master@{#414698}
==========

[commit-bot: I haz the power, 2016-08-26 13:43:36]

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