Hide the "downloads" page policy disabled UI elements from supervised users

Hide the "downloads" page policy disabled UI elements from supervised users


Also, add a browser test for that, and add GYP rules for the downloads page browser tests.

BUG=264184
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222054

[ibra, 2013-09-06 08:44:31]

Bernhard, can you please review?

Thanks!

[Bernhard Bauer, 2013-09-06 11:23:26]

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:129: * @param
expectControlsHidden True if the controls to delete download history
Add the type?

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:132:
deleteProhibitedTestHelper: function(expectControlsHidden) {
This is not the best name for the function; does it delete a prohibited test
helper? :)

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:141: var clearAllLinks =
document.querySelectorAll('clear-all-link');
This selects all <clear-all-link> elements, which is probably not what you want.

You should 
* use getElementsByClassName() instead of relying on the CSS selector syntax
* make the base test case above go through the same code paths, but test the
opposite condition. Maybe you could even reuse this method for the base case
too?

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:187:
TEST_F('DownloadsWebUIForSupervisedUsersTest', 'supervisedUsers', function() {
Test cases should start with an uppercase letter (SupervisedUsers); also above.

[ibra, 2013-09-06 14:18:45]

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:141: var clearAllLinks =
document.querySelectorAll('clear-all-link');
On 2013/09/06 11:23:26, Bernhard Bauer wrote:
> This selects all <clear-all-link> elements, which is probably not what you
want.
> 
> You should 
> * use getElementsByClassName() instead of relying on the CSS selector syntax
> * make the base test case above go through the same code paths, but test the
> opposite condition.
As far as I understand the above test goes through the opposite code paths.
 
>Maybe you could even reuse this method for the base case
> too?
I can use a single helper method but that may be somewhat messy since basically
we query different names for the CSS classes each case and assertions are not
perfectly the opposite, are you ok with that?

[Bernhard Bauer, 2013-09-06 15:12:36]

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:141: var clearAllLinks =
document.querySelectorAll('clear-all-link');
On 2013/09/06 14:18:46, ibra wrote:
> On 2013/09/06 11:23:26, Bernhard Bauer wrote:
> > This selects all <clear-all-link> elements, which is probably not what you
> want.
> > 
> > You should 
> > * use getElementsByClassName() instead of relying on the CSS selector syntax
> > * make the base test case above go through the same code paths, but test the
> > opposite condition.
> As far as I understand the above test goes through the opposite code paths.

No, it doesn't. This test selects all <clear-all-link> elements (of which there
are none, because what it should do is select all elements with the
clear-all-link class), and the above one doesn't. If it would, it would not find
any in the case where deletion is allowed either, which would cause the test to
fail and alert the author that something is wrong.

>  
> >Maybe you could even reuse this method for the base case
> > too?
> I can use a single helper method but that may be somewhat messy since
basically
> we query different names for the CSS classes each case and assertions are not
> perfectly the opposite, are you ok with that?

My point is, we should not do different queries (see above for why).

[ibra, 2013-09-06 16:56:42]

PTAL, thanks.

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:129: * @param
expectControlsHidden True if the controls to delete download history
On 2013/09/06 11:23:26, Bernhard Bauer wrote:
> Add the type?

Done.

https://codereview.chromium.org/23851007/diff/7001/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_ui_browsertest.js:187:
TEST_F('DownloadsWebUIForSupervisedUsersTest', 'supervisedUsers', function() {
On 2013/09/06 11:23:26, Bernhard Bauer wrote:
> Test cases should start with an uppercase letter (SupervisedUsers); also
above.

Done.

[Bernhard Bauer, 2013-09-06 17:12:20]

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:96: var removeLinkElements =
document.getElementsByClassName(
Nit: breaking after the equals sign might make it a bit more readable

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:101: // "Clear all" should
not be hidden.
Nit: empty lines before comments please.

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:102:
expectEquals(expectControlsHidden, clearAllElements[0].hidden);
You're passing in a flag that says whether you expect them to be hidden, but it
will only ever be false in this branch, and the comment also states that we
expect them not to be hidden. You could just directly expectFalse().

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:115: expectEquals(0,
clearAllElements.length);
You could maybe pull some more expectations out (i.e. set a variable with the
expected length in here, and do the checks outside of the if-statement, or
compute the expected value inline). Also, below

[ibra, 2013-09-09 09:40:20]

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:96: var removeLinkElements =
document.getElementsByClassName(
On 2013/09/06 17:12:20, Bernhard Bauer wrote:
> Nit: breaking after the equals sign might make it a bit more readable

Done.

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:101: // "Clear all" should
not be hidden.
On 2013/09/06 17:12:20, Bernhard Bauer wrote:
> Nit: empty lines before comments please.

Done.

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:102:
expectEquals(expectControlsHidden, clearAllElements[0].hidden);
On 2013/09/06 17:12:20, Bernhard Bauer wrote:
> You're passing in a flag that says whether you expect them to be hidden, but
it
> will only ever be false in this branch, and the comment also states that we
> expect them not to be hidden. You could just directly expectFalse().

Done.

https://codereview.chromium.org/23851007/diff/16001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:115: expectEquals(0,
clearAllElements.length);
On 2013/09/06 17:12:20, Bernhard Bauer wrote:
> You could maybe pull some more expectations out (i.e. set a variable with the
> expected length in here, and do the checks outside of the if-statement, or
> compute the expected value inline). Also, below 

Done.

[Bernhard Bauer, 2013-09-09 13:49:52]

LGTM w/ a comment nit:

https://codereview.chromium.org/23851007/diff/23001/chrome/browser/ui/webui/d...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/23001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:118: // All "Remove from
list" items should not be hidden.
Nit: This would read better as "No [...] items should be hidden".

[ibra, 2013-09-09 14:57:00]

https://codereview.chromium.org/23851007/diff/23001/chrome/browser/ui/webui/d...
File chrome/browser/ui/webui/downloads_ui_browsertest.js (right):

https://codereview.chromium.org/23851007/diff/23001/chrome/browser/ui/webui/d...
chrome/browser/ui/webui/downloads_ui_browsertest.js:118: // All "Remove from
list" items should not be hidden.
On 2013/09/09 13:49:52, Bernhard Bauer wrote:
> Nit: This would read better as "No [...] items should be hidden".

Done.

[commit-bot: I haz the power, 2013-09-09 14:57:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@chromium.org/23851007/29001

[commit-bot: I haz the power, 2013-09-09 17:48:02]

Change committed as 222054