MD Settings: change settings-input to home-url-input due to reduced usage.

MD Settings: change settings-input to home-url-input due to reduced usage.

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

Review-Url: https://codereview.chromium.org/2773233002
Cr-Commit-Position: refs/heads/master@{#461487}
Committed: https://chromium.googlesource.com/chromium/src/+/13fa47f5a9d4d63eae97046a278e890f59e84c95

[scottchen, 2017-03-24 21:45:14]

Description was changed from

==========
MD Settings: change settings-input to home-url-input due to reduced usage.

BUG=None
==========

to

==========
MD Settings: change settings-input to home-url-input due to reduced usage.

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

[scottchen, 2017-03-24 22:05:54]

scottchen@chromium.org changed reviewers:
+ dpapad@chromium.org

[scottchen, 2017-03-24 22:05:55]

Follow-up to the home url validation
CL.https://codereview.chromium.org/2766093002/

[dpapad, 2017-03-24 23:17:04]

https://codereview.chromium.org/2773233002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/home_url_input.html
(right):

https://codereview.chromium.org/2773233002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/home_url_input.html:6: <link
rel="import" href="pref_control_behavior.html">
The URL needs to be updated (and I am pretty sure that things should be broken
as it is now).

[scottchen, 2017-03-25 00:54:32]

https://codereview.chromium.org/2773233002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/home_url_input.html
(right):

https://codereview.chromium.org/2773233002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/home_url_input.html:6: <link
rel="import" href="pref_control_behavior.html">
On 2017/03/24 23:17:04, dpapad wrote:
> The URL needs to be updated (and I am pretty sure that things should be broken
> as it is now).

Done.

[dpapad, 2017-03-28 00:09:27]

LGTM

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:19: <link
rel="import" href="home_url_input.html">
Nit (optional) Move this one line below to keep alphabetical order.

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/home_url_input.js
(right):

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/home_url_input.js:129: /**
Nit: /** @private */

[scottchen, 2017-03-30 07:05:03]

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:19: <link
rel="import" href="home_url_input.html">
On 2017/03/28 00:09:27, dpapad wrote:
> Nit (optional) Move this one line below to keep alphabetical order.

Done.

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/home_url_input.js
(right):

https://codereview.chromium.org/2773233002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/home_url_input.js:129: /**
On 2017/03/28 00:09:27, dpapad wrote:
> Nit: /** @private */

Done.

[scottchen, 2017-03-30 07:05:07]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-30 07:05:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2773233002/#ps60001
(title: "update based on comments")

[commit-bot: I haz the power, 2017-03-30 07:05:26]

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, 2017-03-30 07:58:43]

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

[commit-bot: I haz the power, 2017-03-30 07:58:44]

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

[scottchen, 2017-03-30 19:39:48]

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

[commit-bot: I haz the power, 2017-03-30 19:40:37]

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

[scottchen, 2017-03-30 19:41:50]

dpapad@ I had to update the tests after we moved more stuff into the
home-url-input element. PTAL

[dpapad, 2017-03-30 20:27:00]

https://codereview.chromium.org/2773233002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2773233002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:259:
document.body.appendChild(input);
This test no longer uses appearancePage. Let's call PolymerTest.clearBody()
before appending to body (test should still work).

Moreover (optional), you could also move this test to its own suite() in this
file.

[commit-bot: I haz the power, 2017-03-30 21:42:00]

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

[commit-bot: I haz the power, 2017-03-30 21:42:01]

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

[scottchen, 2017-03-31 00:01:06]

https://codereview.chromium.org/2773233002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2773233002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:259:
document.body.appendChild(input);
On 2017/03/30 20:27:00, dpapad wrote:
> This test no longer uses appearancePage. Let's call PolymerTest.clearBody()
> before appending to body (test should still work).
> 
> Moreover (optional), you could also move this test to its own suite() in this
> file.

Done.

[scottchen, 2017-03-31 00:01:16]

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

[commit-bot: I haz the power, 2017-03-31 00:01:54]

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, 2017-03-31 01:09:57]

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

[commit-bot: I haz the power, 2017-03-31 01:09:58]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-03-31 20:31:12]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-31 20:31:13]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2773233002/#ps100001
(title: "move home url buttons test to its own suite")

[scottchen, 2017-03-31 20:31:25]

The CQ bit was unchecked by scottchen@chromium.org

[dpapad, 2017-03-31 20:38:52]

Still LG with nits.

https://codereview.chromium.org/2773233002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/home_url_input.js
(right):

https://codereview.chromium.org/2773233002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/home_url_input.js:41: created:
function() {
Nit: @override

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:262:
appearanceBrowserProxy.reset();
Nit: There is no need to reset() the proxy if you are always creating a new one.

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:282: .then(function() {
Can you add an assertion that checks that the string sent to the proxy is the
right one? 

...
  .then(function(url) {
    assertEquals(homeUrlInput.value, url);
    ...

[scottchen, 2017-03-31 21:02:46]

https://codereview.chromium.org/2773233002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/home_url_input.js
(right):

https://codereview.chromium.org/2773233002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/home_url_input.js:41: created:
function() {
On 2017/03/31 20:38:52, dpapad wrote:
> Nit: @override

Done.

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:262:
appearanceBrowserProxy.reset();
On 2017/03/31 20:38:52, dpapad wrote:
> Nit: There is no need to reset() the proxy if you are always creating a new
one.

Done.

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:282: .then(function() {
On 2017/03/31 20:38:52, dpapad wrote:
> Can you add an assertion that checks that the string sent to the proxy is the
> right one? 
> 
> ...
>   .then(function(url) {
>     assertEquals(homeUrlInput.value, url);
>     ...

The Promise only resolves with a boolean value, it doesn't send the original url
back.

[dpapad, 2017-03-31 21:05:08]

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:282: .then(function() {
On 2017/03/31 at 21:02:46, scottchen wrote:
> On 2017/03/31 20:38:52, dpapad wrote:
> > Can you add an assertion that checks that the string sent to the proxy is
the
> > right one? 
> > 
> > ...
> >   .then(function(url) {
> >     assertEquals(homeUrlInput.value, url);
> >     ...
> 
> The Promise only resolves with a boolean value, it doesn't send the original
url back.

Need to change this line,
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/appearan...

to this.methodCalled('validateStartupPage', value);
|value| will be forwarded to the promise returned by whenCalled(). We use that
pattern in several places to check that the arguments passed to the proxy are
correct.

[scottchen, 2017-03-31 21:20:43]

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2773233002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:282: .then(function() {
On 2017/03/31 21:05:08, dpapad wrote:
> On 2017/03/31 at 21:02:46, scottchen wrote:
> > On 2017/03/31 20:38:52, dpapad wrote:
> > > Can you add an assertion that checks that the string sent to the proxy is
> the
> > > right one? 
> > > 
> > > ...
> > >   .then(function(url) {
> > >     assertEquals(homeUrlInput.value, url);
> > >     ...
> > 
> > The Promise only resolves with a boolean value, it doesn't send the original
> url back.
> 
> Need to change this line,
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/appearan...
> 
> to this.methodCalled('validateStartupPage', value);
> |value| will be forwarded to the promise returned by whenCalled(). We use that
> pattern in several places to check that the arguments passed to the proxy are
> correct.

Done.

[scottchen, 2017-03-31 21:20:48]

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

[commit-bot: I haz the power, 2017-03-31 21:21:39]

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, 2017-03-31 23:54:14]

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

[commit-bot: I haz the power, 2017-03-31 23:54:15]

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

[dpapad, 2017-03-31 23:57:27]

Still LGTM

[scottchen, 2017-04-03 17:26:22]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-04-03 17:26:44]

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, 2017-04-03 18:43:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491240382217680,
"parent_rev": "04f617af016ca074f249a17183a0eb79d7a2ad60", "commit_rev":
"13fa47f5a9d4d63eae97046a278e890f59e84c95"}

[commit-bot: I haz the power, 2017-04-03 18:44:36]

Description was changed from

==========
MD Settings: change settings-input to home-url-input due to reduced usage.

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

to

==========
MD Settings: change settings-input to home-url-input due to reduced usage.

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

Review-Url: https://codereview.chromium.org/2773233002
Cr-Commit-Position: refs/heads/master@{#461487}
Committed:
https://chromium.googlesource.com/chromium/src/+/13fa47f5a9d4d63eae97046a278e...
==========

[commit-bot: I haz the power, 2017-04-03 18:44:37]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/13fa47f5a9d4d63eae97046a278e...