MD Settings: Preserve search URL param in subpages.

MD Settings: Preserve search URL param in subpages.

BUG=661835, 663708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/6da08c5ddf401d99c58878e00726875ce72691ec
Cr-Commit-Position: refs/heads/master@{#431152}

[dpapad, 2016-11-03 23:12:31]

Description was changed from

==========
WIP, preserve search URL param in subpages.

BUG=
==========

to

==========
WIP, preserve search URL param in subpages.

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

[dpapad, 2016-11-03 23:25:50]

Description was changed from

==========
WIP, preserve search URL param in subpages.

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

to

==========
MD Settings: Preserve search URL param in subpages.

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

[tommycli, 2016-11-03 23:31:33]

tommycli@chromium.org changed reviewers:
+ tommycli@chromium.org

[tommycli, 2016-11-03 23:31:33]

lgtm overall approach of preserving the search param is exactly what I would
have done.

I have some code tweak suggested below:

https://codereview.chromium.org/2469393004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2469393004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:344: 
I would recommend doing something like this on line 329:

if (preserveSearch && oldSearchParam && !newSearchParam)
  params.append('search', oldSearchParam)

I think then the URL will be taken care of by 334 automatically, and you don't
need this block.

[dpapad, 2016-11-03 23:58:28]

On 2016/11/03 at 23:31:33, tommycli wrote:
> lgtm overall approach of preserving the search param is exactly what I would
have done.
> 
> I have some code tweak suggested below:
> 
>
https://codereview.chromium.org/2469393004/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/route.js (right):
> 
>
https://codereview.chromium.org/2469393004/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/route.js:344: 
> I would recommend doing something like this on line 329:
> 
> if (preserveSearch && oldSearchParam && !newSearchParam)
>   params.append('search', oldSearchParam)
> 
> I think then the URL will be taken care of by 334 automatically, and you don't
need this block.

Thanks Tommy. I'll address the comment and attempt to add some tests for the URL
search parameter preservation.

[dpapad, 2016-11-04 02:00:33]

https://codereview.chromium.org/2469393004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2469393004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:344: 
On 2016/11/03 at 23:31:33, tommycli wrote:
> I would recommend doing something like this on line 329:
> 
> if (preserveSearch && oldSearchParam && !newSearchParam)
>   params.append('search', oldSearchParam)
> 
> I think then the URL will be taken care of by 334 automatically, and you don't
need this block.

Done. I also removed the "if (opt_dynamicParameters) {" check completely.

[dpapad, 2016-11-08 19:32:57]

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

[commit-bot: I haz the power, 2016-11-08 19:33:23]

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-11-08 21:09:35]

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

[commit-bot: I haz the power, 2016-11-08 21:09:36]

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

[dpapad, 2016-11-08 21:40:29]

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

[commit-bot: I haz the power, 2016-11-08 21:41:12]

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

[dpapad, 2016-11-08 22:48:02]

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

[dpapad, 2016-11-08 22:48:03]

@dschuyler: Tommy had LGed this CL, before going OOO, but I had not add tests at
the time. Can you take a look?

[commit-bot: I haz the power, 2016-11-09 00:25:13]

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

[commit-bot: I haz the power, 2016-11-09 00:25:15]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-11-09 21:48:47]

Description was changed from

==========
MD Settings: Preserve search URL param in subpages.

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

to

==========
MD Settings: Preserve search URL param in subpages.

BUG=661835,663708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-11-09 22:44:24]

On 2016/11/08 at 22:48:03, dpapad wrote:
> @dschuyler: Tommy had LGed this CL, before going OOO, but I had not add tests
at the time. Can you take a look?

Friendly ping.

[dschuyler, 2016-11-09 22:52:13]

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

https://codereview.chromium.org/2469393004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_ui_browsertest.js:131:
settings.navigateTo(settings.Route.BASIC, null, false);
What are null, false representing here?

[dschuyler, 2016-11-09 23:10:14]

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:327: var preserveSearch =
opt_preserveSearch == undefined ?
Can we go with either 
!opt_preserveSearch
or 
opt_preserveSearch === undefined
in place of 
opt_preserveSearch == undefined

or maybe

var preserveSearch = !!opt_preserveSearch;

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.js:78:
settings.navigateTo(route, null, false);
I find null, false unclear. Could we do

settings.navigateTo(route, /*dynamicParams*/null, /*preserveSearch*/false);

or 

settings.navigateTo(route, new URLSearchParams(), /*preserveSearch*/false);

[dpapad, 2016-11-09 23:19:16]

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:327: var preserveSearch =
opt_preserveSearch == undefined ?
On 2016/11/09 at 23:10:14, dschuyler wrote:
> Can we go with either 
> !opt_preserveSearch
> or 
> opt_preserveSearch === undefined
> in place of 
> opt_preserveSearch == undefined
> 
> or maybe
> 
> var preserveSearch = !!opt_preserveSearch;

Default value of opt_preserveSearch is true, if the caller does not pass
anything, so the suggestion is not equivalent I think (a value of undefined will
be interpreted as false instead of true).

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2469393004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.js:78:
settings.navigateTo(route, null, false);
On 2016/11/09 at 23:10:14, dschuyler wrote:
> I find null, false unclear. Could we do
> 
> settings.navigateTo(route, /*dynamicParams*/null, /*preserveSearch*/false);
> 
> or 
> 
> settings.navigateTo(route, new URLSearchParams(), /*preserveSearch*/false);

Done.

[dpapad, 2016-11-09 23:34:22]

https://codereview.chromium.org/2469393004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2469393004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:325: var navigateTo = function(route,
opt_dynamicParameters, opt_removeSearch) {
Per our discussion, I reversed preserveSearch to removeSearch, such that the
default value can be false, which makes it more readable.

[dpapad, 2016-11-09 23:38:28]

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

[commit-bot: I haz the power, 2016-11-09 23:39:03]

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

[dschuyler, 2016-11-09 23:45:36]

lgtm

https://codereview.chromium.org/2469393004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_ui_browsertest.js (right):

https://codereview.chromium.org/2469393004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_ui_browsertest.js:133: /* dynamicParams
*/ null, /* removeSearch */ true);
optional:
I've gotten the advice to fill the prior line before wrapping.
I'm passing that along as a suggestion, though I don't recall 
that being in the style guide, so this is optional.

settings.navigateTo(settings.Route.BASIC, /* dynamicParams */ null,
    /* removeSearch */ true);

[dpapad, 2016-11-09 23:52:09]

https://codereview.chromium.org/2469393004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_ui_browsertest.js (right):

https://codereview.chromium.org/2469393004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_ui_browsertest.js:133: /* dynamicParams
*/ null, /* removeSearch */ true);
On 2016/11/09 at 23:45:36, dschuyler wrote:
> optional:
> I've gotten the advice to fill the prior line before wrapping.
> I'm passing that along as a suggestion, though I don't recall 
> that being in the style guide, so this is optional.
> 
> settings.navigateTo(settings.Route.BASIC, /* dynamicParams */ null,
>     /* removeSearch */ true);

This conflicts with this guideline (from the C++ guide, which JS inherits from,
or used to inherit from).

"If the arguments do not all fit on one line, they should be broken up onto
multiple lines, with each subsequent line aligned with the first argument."
https://google.github.io/styleguide/cppguide.html#Function_Calls

In other words the 1st parameter in each line should start at the same column.
fooMethod(bar,
          baz);
OR
fooMethod(
    bar,
    baz);

The latter has a slight advantage, that renaming the method later can be
automated with a script, without having to adjust the 2nd line's indentation.

[dpapad, 2016-11-10 00:11:14]

Thanks!

[dpapad, 2016-11-10 00:18:50]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-10 00:18:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2469393004/#ps140001
(title: "Function params nit.")

[commit-bot: I haz the power, 2016-11-10 00:19:39]

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-11-10 02:54:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-10 03:08:31]

Description was changed from

==========
MD Settings: Preserve search URL param in subpages.

BUG=661835,663708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Preserve search URL param in subpages.

BUG=661835,663708
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/6da08c5ddf401d99c58878e00726875ce72691ec
Cr-Commit-Position: refs/heads/master@{#431152}
==========

[commit-bot: I haz the power, 2016-11-10 03:08:32]

Patchset 8 (id:??) landed as
https://crrev.com/6da08c5ddf401d99c58878e00726875ce72691ec
Cr-Commit-Position: refs/heads/master@{#431152}