Make chrome://site-engagement non-polymer and re-enable on Android.

Make chrome://site-engagement non-polymer and re-enable on Android.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857
Committed: https://crrev.com/727837fc15a5f59e0a8a3b3655998c6398167bee
Cr-Commit-Position: refs/heads/master@{#380939}

[calamity, 2016-03-02 06:28:14]

Description was changed from

==========
Make chrome://site-engagement non-polymer.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857
==========

to

==========
Make chrome://site-engagement non-polymer and re-enable on Android.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857
==========

[calamity, 2016-03-02 07:10:33]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[calamity, 2016-03-02 07:10:34]

+tsergeant for initial review.

[tsergeant, 2016-03-03 04:00:57]

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/site_engagement.html (right):

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.html:75: <th onclick=""
sort-key="origin">
Remove the onclick=""

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.html:78: <th
onclick="handleSortColumnTap" sort-key="score" class="sort-column"
Remove this one too

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:28: headers[i].onclick =
function(e) {
use 

headers[i].addEventListener('click', function() {..})

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:37:
oldSortColumn.className = '';
Should probably use classList instead of className:

oldSortColumn.classList.remove('sort-column')
e.target.classList.add('sort-column')

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:52: function
createRow(info) {
You could do

var row = document.createElement('tr')
row.innerHTML = '<td class="origin-cell"></td><td>...</td>'

and then go through and update the elements that need changing.

I think this might end up nicer since lots of the work here is boilerplate which
doesn't need much changing, but I'll leave it up to you.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:59: scoreInput.onchange =
handleScoreChange.bind(undefined, info.origin);
scoreInput.addEventListener('change', handleScoreChange.bind...)

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:97: while
(engagementTableBody.firstChild)
How about just using engagementTableBody.innerHTML = '' ?

[tsergeant, 2016-03-03 22:36:42]

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:52: function
createRow(info) {
On 2016/03/03 04:00:56, tsergeant wrote:
> You could do
> 
> var row = document.createElement('tr')
> row.innerHTML = '<td class="origin-cell"></td><td>...</td>'
> 
> and then go through and update the elements that need changing.
> 
> I think this might end up nicer since lots of the work here is boilerplate
which
> doesn't need much changing, but I'll leave it up to you.

Alternatively, util.js has a function 'createElementWithClassName' which you
could use.

[calamity, 2016-03-08 07:35:57]

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/site_engagement.html (right):

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.html:75: <th onclick=""
sort-key="origin">
On 2016/03/03 04:00:56, tsergeant wrote:
> Remove the onclick=""

Done.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.html:78: <th
onclick="handleSortColumnTap" sort-key="score" class="sort-column"
On 2016/03/03 04:00:56, tsergeant wrote:
> Remove this one too

Done.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:28: headers[i].onclick =
function(e) {
On 2016/03/03 04:00:56, tsergeant wrote:
> use 
> 
> headers[i].addEventListener('click', function() {..})

Done.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:37:
oldSortColumn.className = '';
On 2016/03/03 04:00:56, tsergeant wrote:
> Should probably use classList instead of className:
> 
> oldSortColumn.classList.remove('sort-column')
> e.target.classList.add('sort-column')

Done.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:52: function
createRow(info) {
On 2016/03/03 22:36:42, tsergeant wrote:
> On 2016/03/03 04:00:56, tsergeant wrote:
> > You could do
> > 
> > var row = document.createElement('tr')
> > row.innerHTML = '<td class="origin-cell"></td><td>...</td>'
> > 
> > and then go through and update the elements that need changing.
> > 
> > I think this might end up nicer since lots of the work here is boilerplate
> which
> > doesn't need much changing, but I'll leave it up to you.
> 
> Alternatively, util.js has a function 'createElementWithClassName' which you
> could use.

Done.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:59: scoreInput.onchange =
handleScoreChange.bind(undefined, info.origin);
On 2016/03/03 04:00:56, tsergeant wrote:
> scoreInput.addEventListener('change', handleScoreChange.bind...)

Done.

https://codereview.chromium.org/1758573002/diff/1/chrome/browser/resources/en...
chrome/browser/resources/engagement/site_engagement.js:97: while
(engagementTableBody.firstChild)
On 2016/03/03 04:00:56, tsergeant wrote:
> How about just using engagementTableBody.innerHTML = '' ?

Done.

[tsergeant, 2016-03-10 06:13:21]

lgtm

https://codereview.chromium.org/1758573002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1758573002/diff/20001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:79: * Sets the engagement
score when a score input it changed. Also resets the
Nit: 'is changed'

[calamity, 2016-03-10 07:24:40]

https://codereview.chromium.org/1758573002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1758573002/diff/20001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:79: * Sets the engagement
score when a score input it changed. Also resets the
On 2016/03/10 06:13:21, tsergeant wrote:
> Nit: 'is changed'

Done.

[calamity, 2016-03-10 07:25:38]

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

[calamity, 2016-03-10 07:25:39]

+dbeam for chrome_web_ui_controller_factory.cc, browser_resources.grd and
site_engagement_ui.cc OWNERS.

[Dan Beam, 2016-03-11 22:46:36]

lgtm

https://codereview.chromium.org/1758573002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui.cc (right):

https://codereview.chromium.org/1758573002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:7: #include <math.h>
https://chromium-cpp.appspot.com/ mentions <cmath> instead of <math.h>, don't
really know the difference myself...

[calamity, 2016-03-14 05:38:30]

https://codereview.chromium.org/1758573002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui.cc (right):

https://codereview.chromium.org/1758573002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:7: #include <math.h>
On 2016/03/11 22:46:36, Dan Beam wrote:
> https://chromium-cpp.appspot.com/ mentions <cmath> instead of <math.h>, don't
> really know the difference myself...
Fixed.

[calamity, 2016-03-14 05:38:45]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-03-14 05:38:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1758573002/#ps60001
(title: "address nit")

[commit-bot: I haz the power, 2016-03-14 05:38:56]

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

[commit-bot: I haz the power, 2016-03-14 06:43:16]

Description was changed from

==========
Make chrome://site-engagement non-polymer and re-enable on Android.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857
==========

to

==========
Make chrome://site-engagement non-polymer and re-enable on Android.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857
==========

[commit-bot: I haz the power, 2016-03-14 06:43:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-14 06:44:31]

Description was changed from

==========
Make chrome://site-engagement non-polymer and re-enable on Android.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857
==========

to

==========
Make chrome://site-engagement non-polymer and re-enable on Android.

This CL unpolymerizes the site engagement WebUI so it can be re-enabled
on Android where Polymer is not available for WebUI.

BUG=524857

Committed: https://crrev.com/727837fc15a5f59e0a8a3b3655998c6398167bee
Cr-Commit-Position: refs/heads/master@{#380939}
==========

[commit-bot: I haz the power, 2016-03-14 06:44:32]

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