MD Settings: implement "RESTART" button for hardware acceleration to take effect

MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779
Committed: https://crrev.com/dec8672509fbf27ad2aa14dbc83f0512cff34677
Cr-Commit-Position: refs/heads/master@{#382637}

[Dan Beam, 2016-03-18 05:18:24]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2016-03-18 05:18:30]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-03-19 03:40:40]

Patchset #1 (id:40001) has been deleted

[Dan Beam, 2016-03-19 03:40:48]

Patchset #1 (id:60001) has been deleted

[dpapad, 2016-03-21 18:17:38]

Looking pretty good, few comments/questions.

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/advanced_page/compiled_resources2.gyp
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/compiled_resources2.gyp:14:
'../system_page/compiled_resources2.gyp:system_page_browser_proxy',
advanced_page does not depend directly on system_page_browser_proxy. Is this
needed or just a remainder from when you renamed delegate to proxy?

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/system_page/system_page_browser_proxy.js:13:
changeProxySettings: assertNotReached,
Nit: Is assertNotReached() useful here (instead of simply "function() {}"? This
is not a base class, so there is no chance ever for this code to get executed.
@interface is purely a JS compiler concept. I understand that assertNotReached
would make more sense if this was a base class, and you wanted to detect methods
that have not been overridden.

After all, doesn't the compiler tell us if a class claims to implement an
interface but it does not implement all methods?

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:373:
{"powerwashDialogButton", IDS_RELAUNCH_BUTTON},
Can you add a TODO here for re-using the new IDS_SETTINGS_RESTART?

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:817:
// TODO(dbeam): we probably shouldn't name this class "strings" but add all
I guess you are referring to the file name "md_settings_localized_strings" since
there is no class. How about suggesting a renaming in the TODO to make in more
useful? Maybe
1) md_settings_localized_strings -> md_settings_load_time_data
2) AddLocalizedStrings -> AddLoadTimeData

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/system_handler.cc (right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/system_handler.cc:34: void
SystemHandler::AddLoadTimeData(content::WebUIDataSource* data_source) {
This is related to the "TODO" you are adding in md_settings_localized_strings.

In a similar case for ResetSettingsHandler, load time data that is not related
to localized strings (a boolean for example), is added via
ResetSettingsHandler::Create(), see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
instead of calling this method from md_settings_localized_strings. Perhaps you
could simply do the same here (creating SystemHandler::Create()), and remove the
TODO and this call from md_settings_localized_strings.

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

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:233:
'test_browser_proxy.js',
TestBrowserProxy depends on PromiseResolver, so you should need to also add
ROOT_PATH + 'ui/webui/resources/js/promise_resolver.js',

not sure how it works without this (maybe something else is pulling in that
dependency).

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

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/system_page_tests.js:6: var
HARDWARE_ACCELERATION_ENABLED_AT_STARTUP = true;
Nit: @const {boolean}

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/system_page_tests.js:70: var systemPage =
document.createElement('settings-system-page');
It is a bit odd that you are testing your element without attaching it to the
DOM first. I guess it works, because nothing is done in attached() (and similar
for all deps like paper-button, settings-checkbox?), but I don't see good reason
for deviating that much from the prod code.

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/system_page_tests.js:87: });
Should we also add a test to ensure that the settings-checkbox is checked when
hardware_acceleration_mode.enabled is true?

[michaelpg, 2016-03-22 00:43:49]

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

[michaelpg, 2016-03-22 00:43:50]

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

https://codereview.chromium.org/1814703004/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/system_page_tests.js:71: systemPage.set('prefs',
settingsPrefs.prefs);
this won't bind systemPage.prefs to settingsPrefs.prefs, so systemPage won't be
notified of changes to prefs.

[michaelpg, 2016-03-22 01:00:00]

https://codereview.chromium.org/1814703004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/advanced_page/advanced_page.js (left):

https://codereview.chromium.org/1814703004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/advanced_page/advanced_page.js:57:
this.scroller = this.parentElement;
noooooooooooooooooooooooo

[Dan Beam, 2016-03-22 02:17:00]

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/advanced_page/compiled_resources2.gyp
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/compiled_resources2.gyp:14:
'../system_page/compiled_resources2.gyp:system_page_browser_proxy',
On 2016/03/21 18:17:38, dpapad wrote:
> advanced_page does not depend directly on system_page_browser_proxy. Is this
> needed or just a remainder from when you renamed delegate to proxy?

remainder, removed

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/system_page/system_page_browser_proxy.js:13:
changeProxySettings: assertNotReached,
On 2016/03/21 18:17:38, dpapad wrote:
> Nit: Is assertNotReached() useful here (instead of simply "function() {}"?
This
> is not a base class, so there is no chance ever for this code to get executed.
> @interface is purely a JS compiler concept. I understand that assertNotReached
> would make more sense if this was a base class, and you wanted to detect
methods
> that have not been overridden.
> 
> After all, doesn't the compiler tell us if a class claims to implement an
> interface but it does not implement all methods?

pre-Done (I don't feel strongly enough to slow down review, so I've done this in
good faith).

I'd like to know the reverse question: what do you think the /detriment/ to
using assertNotReached is?  assertNotReached is at least re-using a function
reference (so there's slightly more possibility for the compiler to re-use it
than an anonymous function() {} afaik), and if anybody ever did actually try to
use this they'd fail.  it also makes adds an additional, enforced signal that
this class is abstract.

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:373:
{"powerwashDialogButton", IDS_RELAUNCH_BUTTON},
On 2016/03/21 18:17:38, dpapad wrote:
> Can you add a TODO here for re-using the new IDS_SETTINGS_RESTART?

Done. (just did it like we talked about on chat)

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:817:
// TODO(dbeam): we probably shouldn't name this class "strings" but add all
On 2016/03/21 18:17:38, dpapad wrote:
> I guess you are referring to the file name "md_settings_localized_strings"
since
> there is no class. How about suggesting a renaming in the TODO to make in more
> useful? Maybe
> 1) md_settings_localized_strings -> md_settings_load_time_data
> 2) AddLocalizedStrings -> AddLoadTimeData

Done.

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/system_handler.cc (right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/system_handler.cc:34: void
SystemHandler::AddLoadTimeData(content::WebUIDataSource* data_source) {
On 2016/03/21 18:17:38, dpapad wrote:
> This is related to the "TODO" you are adding in md_settings_localized_strings.
> 
> In a similar case for ResetSettingsHandler, load time data that is not related
> to localized strings (a boolean for example), is added via
> ResetSettingsHandler::Create(), see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> instead of calling this method from md_settings_localized_strings. Perhaps you
> could simply do the same here (creating SystemHandler::Create()), and remove
the
> TODO and this call from md_settings_localized_strings.

I see 2 differences with the |Create()| method you linked to:
1) that it's a factory function
2) calling it has a side effect on a shared, global-ish object in a possibly
unexpected way

I don't see how that particular pattern is helpful to this handler.

I don't have a preference between a static Create() or passing dependencies
directly into a constructor.  Most design books will point mildly in favor of a
factory function because it makes construction more flexible, but almost all of
our handlers are already constructed directly (i.e. new Handler(deps...)) and
are really only separate classes to keep our code manageably small/separated
(i.e. they're mainly only used from 1 site).  I would also say one of the main
uses of a |Create()| method is access control to constructors (i.e. a test-only
constructor), which doesn't apply to ResetSettingsHandler nor SystemHandler
currently.

More grandly: I'd prefer to add loadTimeData in one pass and "freeze" the object
(functionally) by removing mutable access to it (i.e. remove or const-ify
WebUIDataSource* pointers).  Creating a static AddLoadTimeData() in this
handler's case disallows access to |this| (good) but still allows access to
file-level globals / shards implementation details per-handler (good).

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

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:233:
'test_browser_proxy.js',
On 2016/03/21 18:17:38, dpapad wrote:
> TestBrowserProxy depends on PromiseResolver, so you should need to also add
> ROOT_PATH + 'ui/webui/resources/js/promise_resolver.js',
> 
> not sure how it works without this (maybe something else is pulling in that
> dependency).

this is probably why it worked:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

anyways, added explicitly (still a bummer to have to flatten deps, though)

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

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/system_page_tests.js:6: var
HARDWARE_ACCELERATION_ENABLED_AT_STARTUP = true;
On 2016/03/21 18:17:38, dpapad wrote:
> Nit: @const {boolean}

Done.

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/system_page_tests.js:70: var systemPage =
document.createElement('settings-system-page');
On 2016/03/21 18:17:38, dpapad wrote:
> It is a bit odd that you are testing your element without attaching it to the
> DOM first. I guess it works, because nothing is done in attached() (and
similar
> for all deps like paper-button, settings-checkbox?), but I don't see good
reason
> for deviating that much from the prod code.

Done.

https://codereview.chromium.org/1814703004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/system_page_tests.js:87: });
On 2016/03/21 18:17:38, dpapad wrote:
> Should we also add a test to ensure that the settings-checkbox is checked when
> hardware_acceleration_mode.enabled is true?

Done.

https://codereview.chromium.org/1814703004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/advanced_page/advanced_page.js (left):

https://codereview.chromium.org/1814703004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/advanced_page/advanced_page.js:57:
this.scroller = this.parentElement;
On 2016/03/22 00:59:59, michaelpg wrote:
> noooooooooooooooooooooooo

Doooooooooooooooooooooooone.

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

https://codereview.chromium.org/1814703004/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/system_page_tests.js:71: systemPage.set('prefs',
settingsPrefs.prefs);
On 2016/03/22 00:43:50, michaelpg wrote:
> this won't bind systemPage.prefs to settingsPrefs.prefs, so systemPage won't
be
> notified of changes to prefs.

Done.

[dpapad, 2016-03-22 02:40:04]

LGTM

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/system_page/system_page_browser_proxy.js:13:
changeProxySettings: assertNotReached,
On 2016/03/22 at 02:16:59, Dan Beam wrote:
> On 2016/03/21 18:17:38, dpapad wrote:
> > Nit: Is assertNotReached() useful here (instead of simply "function() {}"?
This
> > is not a base class, so there is no chance ever for this code to get
executed.
> > @interface is purely a JS compiler concept. I understand that
assertNotReached
> > would make more sense if this was a base class, and you wanted to detect
methods
> > that have not been overridden.
> > 
> > After all, doesn't the compiler tell us if a class claims to implement an
> > interface but it does not implement all methods?
> 
> pre-Done (I don't feel strongly enough to slow down review, so I've done this
in good faith).
> 
> I'd like to know the reverse question: what do you think the /detriment/ to
using assertNotReached is?  assertNotReached is at least re-using a function
reference (so there's slightly more possibility for the compiler to re-use it
than an anonymous function() {} afaik), and if anybody ever did actually try to
use this they'd fail.  it also makes adds an additional, enforced signal that
this class is abstract.

I don't think it is detrimental (which is why I prefixed my comment with "nit"
and I should have also added "optional"). It's just adding more confusion to an
already confusing situation (for the non experienced reader), explaining below.

1) JS interfaces are effectively dead code, that is only useful to the JS
compiler.
2) This dead code lives in the same file as production code (the implementation)
and gets unnecessarily evaluated.
3) Now by using assertNotReached, this dead-code somehow seems (to someone not
familiar with JS compiler) as if it can potentially be also executed under
certain circumstances.

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/system_handler.cc (right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/system_handler.cc:34: void
SystemHandler::AddLoadTimeData(content::WebUIDataSource* data_source) {
On 2016/03/22 at 02:16:59, Dan Beam wrote:
> On 2016/03/21 18:17:38, dpapad wrote:
> > This is related to the "TODO" you are adding in
md_settings_localized_strings.
> > 
> > In a similar case for ResetSettingsHandler, load time data that is not
related
> > to localized strings (a boolean for example), is added via
> > ResetSettingsHandler::Create(), see
> >
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> > instead of calling this method from md_settings_localized_strings. Perhaps
you
> > could simply do the same here (creating SystemHandler::Create()), and remove
the
> > TODO and this call from md_settings_localized_strings.
> 
> I see 2 differences with the |Create()| method you linked to:
> 1) that it's a factory function
> 2) calling it has a side effect on a shared, global-ish object in a possibly
unexpected way
> 
> I don't see how that particular pattern is helpful to this handler.
> 
> I don't have a preference between a static Create() or passing dependencies
directly into a constructor.  Most design books will point mildly in favor of a
factory function because it makes construction more flexible, but almost all of
our handlers are already constructed directly (i.e. new Handler(deps...)) and
are really only separate classes to keep our code manageably small/separated
(i.e. they're mainly only used from 1 site).  I would also say one of the main
uses of a |Create()| method is access control to constructors (i.e. a test-only
constructor), which doesn't apply to ResetSettingsHandler nor SystemHandler
currently.
> 
> More grandly: I'd prefer to add loadTimeData in one pass and "freeze" the
object (functionally) by removing mutable access to it (i.e. remove or const-ify
WebUIDataSource* pointers).  Creating a static AddLoadTimeData() in this
handler's case disallows access to |this| (good) but still allows access to
file-level globals / shards implementation details per-handler (good).

The main reason ResetSettingsHandler::Create() exists is because the boolean is
not only added to the dictionary but it is also used in the logic (which is why
it is also passed to the constructor on-top of adding to the dictionary). So
that was a way to calculate the value of that boolean (calculation is fairly
involved), only once. Additionally passing the boolean from the constructor
allows the test easily to test both cases. In any case, SystemHandler does not
match this case, so your current way is fine, I was just suggesting in case you
wanted to establish a pattern.

[Dan Beam, 2016-03-22 03:06:35]

dbeam@chromium.org changed reviewers:
+ asvitkine@chromium.org
- michaelpg@chromium.org

[Dan Beam, 2016-03-22 03:06:36]

+asvitkine@ for chrome/browser/metrics

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
(right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/system_page/system_page_browser_proxy.js:13:
changeProxySettings: assertNotReached,
On 2016/03/22 02:40:04, dpapad wrote:
> On 2016/03/22 at 02:16:59, Dan Beam wrote:
> > On 2016/03/21 18:17:38, dpapad wrote:
> > > Nit: Is assertNotReached() useful here (instead of simply "function() {}"?
> This
> > > is not a base class, so there is no chance ever for this code to get
> executed.
> > > @interface is purely a JS compiler concept. I understand that
> assertNotReached
> > > would make more sense if this was a base class, and you wanted to detect
> methods
> > > that have not been overridden.
> > > 
> > > After all, doesn't the compiler tell us if a class claims to implement an
> > > interface but it does not implement all methods?
> > 
> > pre-Done (I don't feel strongly enough to slow down review, so I've done
this
> in good faith).
> > 
> > I'd like to know the reverse question: what do you think the /detriment/ to
> using assertNotReached is?  assertNotReached is at least re-using a function
> reference (so there's slightly more possibility for the compiler to re-use it
> than an anonymous function() {} afaik), and if anybody ever did actually try
to
> use this they'd fail.  it also makes adds an additional, enforced signal that
> this class is abstract.
> 
> I don't think it is detrimental (which is why I prefixed my comment with "nit"
> and I should have also added "optional"). It's just adding more confusion to
an
> already confusing situation (for the non experienced reader), explaining
below.
> 
> 1) JS interfaces are effectively dead code, that is only useful to the JS
> compiler.
> 2) This dead code lives in the same file as production code (the
implementation)
> and gets unnecessarily evaluated.
> 3) Now by using assertNotReached, this dead-code somehow seems (to someone not
> familiar with JS compiler) as if it can potentially be also executed under
> certain circumstances.

yeah, this is what I guessed (that it just makes things look slightly more like
it's real code)

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/system_handler.cc (right):

https://codereview.chromium.org/1814703004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/system_handler.cc:34: void
SystemHandler::AddLoadTimeData(content::WebUIDataSource* data_source) {
On 2016/03/22 02:40:04, dpapad wrote:
> On 2016/03/22 at 02:16:59, Dan Beam wrote:
> > On 2016/03/21 18:17:38, dpapad wrote:
> > > This is related to the "TODO" you are adding in
> md_settings_localized_strings.
> > > 
> > > In a similar case for ResetSettingsHandler, load time data that is not
> related
> > > to localized strings (a boolean for example), is added via
> > > ResetSettingsHandler::Create(), see
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> > > instead of calling this method from md_settings_localized_strings. Perhaps
> you
> > > could simply do the same here (creating SystemHandler::Create()), and
remove
> the
> > > TODO and this call from md_settings_localized_strings.
> > 
> > I see 2 differences with the |Create()| method you linked to:
> > 1) that it's a factory function
> > 2) calling it has a side effect on a shared, global-ish object in a possibly
> unexpected way
> > 
> > I don't see how that particular pattern is helpful to this handler.
> > 
> > I don't have a preference between a static Create() or passing dependencies
> directly into a constructor.  Most design books will point mildly in favor of
a
> factory function because it makes construction more flexible, but almost all
of
> our handlers are already constructed directly (i.e. new Handler(deps...)) and
> are really only separate classes to keep our code manageably small/separated
> (i.e. they're mainly only used from 1 site).  I would also say one of the main
> uses of a |Create()| method is access control to constructors (i.e. a
test-only
> constructor), which doesn't apply to ResetSettingsHandler nor SystemHandler
> currently.
> > 
> > More grandly: I'd prefer to add loadTimeData in one pass and "freeze" the
> object (functionally) by removing mutable access to it (i.e. remove or
const-ify
> WebUIDataSource* pointers).  Creating a static AddLoadTimeData() in this
> handler's case disallows access to |this| (good) but still allows access to
> file-level globals / shards implementation details per-handler (good).
> 
> The main reason ResetSettingsHandler::Create() exists is because the boolean
is
> not only added to the dictionary but it is also used in the logic (which is
why
> it is also passed to the constructor on-top of adding to the dictionary). So
> that was a way to calculate the value of that boolean (calculation is fairly
> involved), only once. Additionally passing the boolean from the constructor
> allows the test easily to test both cases. In any case, SystemHandler does not
> match this case, so your current way is fine, I was just suggesting in case
you
> wanted to establish a pattern.

i don't really want to use the pattern here, no, but it may make more sense
somewhere else in the future (but like I said: I'd also like to freeze
loadTimeData if possible)

[Dan Beam, 2016-03-22 04:25:44]

Description was changed from

==========
MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779
==========

to

==========
MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779
==========

[Dan Beam, 2016-03-22 04:25:44]

dbeam@chromium.org changed reviewers:
+ rkaplow@chromium.org
- asvitkine@chromium.org

[Dan Beam, 2016-03-22 04:26:31]

-asvitkine (because "no revs" message)
+rkaplow for chrome/browser/metrics stamp

[rkaplow, 2016-03-22 14:57:55]

lgtm

[Dan Beam, 2016-03-22 19:15:34]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-03-22 19:15:34]

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/1814703004/#ps140001
(title: "nit")

[commit-bot: I haz the power, 2016-03-22 19:16:12]

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

[commit-bot: I haz the power, 2016-03-22 19:34:05]

Description was changed from

==========
MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779
==========

to

==========
MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779
==========

[commit-bot: I haz the power, 2016-03-22 19:34:07]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-03-22 19:36:01]

Description was changed from

==========
MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779
==========

to

==========
MD Settings: implement "RESTART" button for hardware acceleration to take effect

R=dpapad@chromium.org
BUG=595779

Committed: https://crrev.com/dec8672509fbf27ad2aa14dbc83f0512cff34677
Cr-Commit-Position: refs/heads/master@{#382637}
==========

[commit-bot: I haz the power, 2016-03-22 19:36:02]

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