Fix Material Settings Help and Settings redirect

Fix Material Settings Help and Settings redirect

"About in Settings" shouldn't affect loading of MD Settings or
prevent Help from loading at all.

On Chrome OS, chrome://settings wasn't redirecting; on other
platforms, chrome://help wasn't loading.

BUG=621390, 621344
Committed: https://crrev.com/8e75cccd2ff5224bdd82662d47af49a26e247d39
Cr-Commit-Position: refs/heads/master@{#401092}

[Dan Beam, 2016-06-20 21:05:27]

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

[Dan Beam, 2016-06-20 21:05:28]

<3 tests

[michaelpg, 2016-06-20 21:34:46]

On 2016/06/20 21:05:28, Dan Beam wrote:
> <3 tests

hmm, in browser_about_handler_unittest.cc? or md_settings_ui_browsertest.cc?

[Dan Beam, 2016-06-20 23:27:06]

On 2016/06/20 21:34:46, michaelpg wrote:
> On 2016/06/20 21:05:28, Dan Beam wrote:
> > <3 tests
> 
> hmm, in browser_about_handler_unittest.cc? or md_settings_ui_browsertest.cc?

could we just update this?
https://cs.chromium.org/chromium/src/chrome/browser/browser_about_handler_uni...

[michaelpg, 2016-06-21 03:17:46]

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

[michaelpg, 2016-06-21 03:17:47]

test added, function tweaked. PTAL.

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:111: return true;  // Prevent further
rewriting - this is a valid URL.
note the change from patch 3 -- there's no need to rewrite this to
chrome://settings/help

[michaelpg, 2016-06-21 03:29:59]

Description was changed from

==========
Fix Material Settings Help and Settings redirect

Help should redirect to chrome://settings/help when
--enable-features=MaterialDesignSettings is present.

On Chrome OS, chrome://settings wasn't redirecting because About in Settings
is enabled; that shouldn't affect this.

BUG=621390,621344
R=groby@chromium.org
==========

to

==========
Fix Material Settings Help and Settings redirect

"About in Settings" shouldn't affect loading of MD Settings or
prevent Help from loading at all.

On Chrome OS, chrome://settings wasn't redirecting; on other
platforms, chrome://help wasn't loading.

BUG=621390,621344
==========

[Dan Beam, 2016-06-21 05:24:45]

lgtm

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler_unittest.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:9: #include <memory>
nit: #include <utility>

for std::move()

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:33: struct AboutURLTestData {
nit: why does this need to be inside BrowserAboutHandlerTest

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:103: settings_url_result =
this test might be slightly easier to read if the whole thing was keyed of the
OS

[Lei Zhang, 2016-06-21 07:54:00]

nitty rs lgtm

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:101: } else if
(::switches::AboutInSettingsEnabled()) {
There's no need for an else after a return.

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler_unittest.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:35: GURL result_url;
Maybe expected_result_url?

[michaelpg, 2016-06-21 19:15:58]

Patchset #6 (id:100001) has been deleted

[michaelpg, 2016-06-21 19:19:34]

Thanks all. Landing now for fix, but if you have add'l feedback I'm happy to
follow up.

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:101: } else if
(::switches::AboutInSettingsEnabled()) {
On 2016/06/21 07:54:00, Lei Zhang wrote:
> There's no need for an else after a return.

the whole file, including this line before my patch, does it this way :-\

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler_unittest.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:9: #include <memory>
On 2016/06/21 05:24:45, Dan Beam wrote:
> nit: #include <utility>
> 
> for std::move()

Done.

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:33: struct AboutURLTestData {
On 2016/06/21 05:24:45, Dan Beam wrote:
> nit: why does this need to be inside BrowserAboutHandlerTest

ummm dunno, is an anonymous namespace better?

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:35: GURL result_url;
On 2016/06/21 07:54:00, Lei Zhang wrote:
> Maybe expected_result_url?

Done, also renamed TestData => TestCase[s]

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:103: settings_url_result =
On 2016/06/21 05:24:45, Dan Beam wrote:
> this test might be slightly easier to read if the whole thing was keyed of the
> OS

changed to 2 separate tests, does seem more readable

[michaelpg, 2016-06-21 19:19:47]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-06-21 19:19:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2088473002/#ps130001
(title: "git cl format")

[commit-bot: I haz the power, 2016-06-21 19:20:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088473002/130001

[Dan Beam, 2016-06-21 19:22:50]

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler_unittest.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler_unittest.cc:33: struct AboutURLTestData {
On 2016/06/21 19:19:33, michaelpg wrote:
> On 2016/06/21 05:24:45, Dan Beam wrote:
> > nit: why does this need to be inside BrowserAboutHandlerTest
> 
> ummm dunno, is an anonymous namespace better?

i would say so because shorter names, yes

[Dan Beam, 2016-06-21 19:23:18]

https://codereview.chromium.org/2088473002/diff/130001/chrome/browser/browser...
File chrome/browser/browser_about_handler_unittest.cc (right):

https://codereview.chromium.org/2088473002/diff/130001/chrome/browser/browser...
chrome/browser/browser_about_handler_unittest.cc:33: struct AboutURLTestCase {
sweet, lgtm

[Lei Zhang, 2016-06-21 19:30:58]

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2088473002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:101: } else if
(::switches::AboutInSettingsEnabled()) {
On 2016/06/21 19:19:33, michaelpg wrote:
> On 2016/06/21 07:54:00, Lei Zhang wrote:
> > There's no need for an else after a return.
> 
> the whole file, including this line before my patch, does it this way :-\

gahhhhhhhh. Fix later.

[commit-bot: I haz the power, 2016-06-21 20:26:55]

Description was changed from

==========
Fix Material Settings Help and Settings redirect

"About in Settings" shouldn't affect loading of MD Settings or
prevent Help from loading at all.

On Chrome OS, chrome://settings wasn't redirecting; on other
platforms, chrome://help wasn't loading.

BUG=621390,621344
==========

to

==========
Fix Material Settings Help and Settings redirect

"About in Settings" shouldn't affect loading of MD Settings or
prevent Help from loading at all.

On Chrome OS, chrome://settings wasn't redirecting; on other
platforms, chrome://help wasn't loading.

BUG=621390,621344
==========

[commit-bot: I haz the power, 2016-06-21 20:26:56]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2016-06-21 20:31:21]

Description was changed from

==========
Fix Material Settings Help and Settings redirect

"About in Settings" shouldn't affect loading of MD Settings or
prevent Help from loading at all.

On Chrome OS, chrome://settings wasn't redirecting; on other
platforms, chrome://help wasn't loading.

BUG=621390,621344
==========

to

==========
Fix Material Settings Help and Settings redirect

"About in Settings" shouldn't affect loading of MD Settings or
prevent Help from loading at all.

On Chrome OS, chrome://settings wasn't redirecting; on other
platforms, chrome://help wasn't loading.

BUG=621390,621344

Committed: https://crrev.com/8e75cccd2ff5224bdd82662d47af49a26e247d39
Cr-Commit-Position: refs/heads/master@{#401092}
==========

[commit-bot: I haz the power, 2016-06-21 20:31:24]

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