MD Settings: About page, implement help and feedback.

MD Settings: About page, implement help and feedback.

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

Committed: https://crrev.com/b52de52a794f126220c2a671334279f9fc3b65fe
Cr-Commit-Position: refs/heads/master@{#394039}

[dpapad, 2016-05-16 21:41:41]

Description was changed from

==========
MD Settings: About page, hook up, help and feedback.

BUG=603625
==========

to

==========
MD Settings: About page, hook up, help and feedback.

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

[dpapad, 2016-05-16 22:01:36]

Description was changed from

==========
MD Settings: About page, hook up, help and feedback.

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

to

==========
MD Settings: About page, implement help and feedback.

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

[dpapad, 2016-05-16 22:05:07]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-05-16 22:05:12]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-05-17 00:28:42]

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

[tommycli, 2016-05-17 00:36:48]

lgtm sans below

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

https://codereview.chromium.org/1981033003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:24: /** @private
{?settings.AboutPageBrowserProxy} */
Was this supposed to be under properties?

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

https://codereview.chromium.org/1981033003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:15: 'openFeedbackDialog',
nit: Can we make this match the interface order?

[dpapad, 2016-05-17 00:45:13]

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

https://codereview.chromium.org/1981033003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:24: /** @private
{?settings.AboutPageBrowserProxy} */
On 2016/05/17 at 00:36:48, tommycli wrote:
> Was this supposed to be under properties?

We have talked about this stylistic issue in a previous standup. Since this is
not used by HTML, there is no need to have it as a Polymer property. IIRC
correctly in the standup we said it is fine to put it either in "properties" or
outside. I have been using the "outside" approach in a few places, see
https://code.google.com/p/chromium/codesearch#search/&q=%22browserProxy_:%20n....

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

https://codereview.chromium.org/1981033003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:15: 'openFeedbackDialog',
On 2016/05/17 at 00:36:48, tommycli wrote:
> nit: Can we make this match the interface order?

Done.

[tommycli, 2016-05-17 01:07:28]

Ok thanks for linking to the other examples. Lgtm still

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

https://codereview.chromium.org/1981033003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:24: /** @private
{?settings.AboutPageBrowserProxy} */
On 2016/05/17 00:45:13, dpapad wrote:
> On 2016/05/17 at 00:36:48, tommycli wrote:
> > Was this supposed to be under properties?
> 
> We have talked about this stylistic issue in a previous standup. Since this is
> not used by HTML, there is no need to have it as a Polymer property. IIRC
> correctly in the standup we said it is fine to put it either in "properties"
or
> outside. I have been using the "outside" approach in a few places, see
>
https://code.google.com/p/chromium/codesearch#search/&q=%22browserProxy_:%20n....

Acknowledged.

[dpapad, 2016-05-17 01:12:19]

On 2016/05/17 at 01:07:28, tommycli wrote:
> Ok thanks for linking to the other examples. Lgtm still
> 
>
https://codereview.chromium.org/1981033003/diff/80001/chrome/browser/resource...
> File chrome/browser/resources/settings/about_page/about_page.js (right):
> 
>
https://codereview.chromium.org/1981033003/diff/80001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.js:24: /** @private
{?settings.AboutPageBrowserProxy} */
> On 2016/05/17 00:45:13, dpapad wrote:
> > On 2016/05/17 at 00:36:48, tommycli wrote:
> > > Was this supposed to be under properties?
> > 
> > We have talked about this stylistic issue in a previous standup. Since this
is
> > not used by HTML, there is no need to have it as a Polymer property. IIRC
> > correctly in the standup we said it is fine to put it either in "properties"
or
> > outside. I have been using the "outside" approach in a few places, see
> >
https://code.google.com/p/chromium/codesearch#search/&q=%22browserProxy_:%20n....
> 
> Acknowledged.

Thanks for the fast review!

[dpapad, 2016-05-17 01:32:46]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-05-17 01:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1981033003/#ps140001
(title: "Tweak strings to match the mocks."")

[commit-bot: I haz the power, 2016-05-17 01:33:15]

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

[commit-bot: I haz the power, 2016-05-17 03:13:26]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-05-17 03:16:07]

Description was changed from

==========
MD Settings: About page, implement help and feedback.

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

to

==========
MD Settings: About page, implement help and feedback.

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

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

[commit-bot: I haz the power, 2016-05-17 03:16:08]

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