Show an extra privacy warning on Win8

Show an extra privacy warning on Win8

BUG=141723
TEST=Make sure the Win8 privacy warning is shown in the advances settings UI, but ONLY when running on Win8.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152333

[MAD, 2012-08-13 19:18:39]

Can you look at this before I ask for an OWNER's review?

Thanks!

BYE
MAD...

[Roger Tawa OOO till Jul 10th, 2012-08-13 19:40:37]

lgtm

http://codereview.chromium.org/10854126/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10854126/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/browser_options.html:196: <div
id="privacyWin8Data-settings" class="settings-row" hidden>
Itd weird to see both MixedCase and dashes.  Maybe rename this to
"privacyWin8DataSettings" or "privacy-win8-data-settings" ?

http://codereview.chromium.org/10854126/diff/1/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/10854126/diff/1/chrome/common/url_constants.cc...
chrome/common/url_constants.cc:399:
"https://support.google.com/chrome/?p=win8_directories";
This URL does not seem to take me to a page the describes win8 privacy settings
in more detail.  Is the page not up yet?

[MAD, 2012-08-13 19:57:10]

Thanks! Good catch... :-)

I got confused by mixed usage of CamelCase and dash-separator in that file, so I
unconsciously went bezerk... :-P

Fixed now...

BYE
MAD!

http://codereview.chromium.org/10854126/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10854126/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/browser_options.html:196: <div
id="privacyWin8Data-settings" class="settings-row" hidden>
On 2012/08/13 19:40:37, Roger Tawa wrote:
> Itd weird to see both MixedCase and dashes.  Maybe rename this to
> "privacyWin8DataSettings" or "privacy-win8-data-settings" ?

Done.

http://codereview.chromium.org/10854126/diff/1/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/10854126/diff/1/chrome/common/url_constants.cc...
chrome/common/url_constants.cc:399:
"https://support.google.com/chrome/?p=win8_directories";
On 2012/08/13 19:40:37, Roger Tawa wrote:
> This URL does not seem to take me to a page the describes win8 privacy
settings
> in more detail.  Is the page not up yet?

Not up yet...

[MAD, 2012-08-14 15:45:51]

OWNER's review please?

Thanks!

BYE
MAD...

[arv (Not doing code reviews), 2012-08-15 14:33:31]

http://codereview.chromium.org/10854126/diff/6002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10854126/diff/6002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:195: <if expr="is_win">
This will cause unbalanced <div>s. You probably want the <if> on line 196
instead.

http://codereview.chromium.org/10854126/diff/6002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10854126/diff/6002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.js:1198:
$('privacy-win8-data-settings').hidden = false;
How about, in initializePage we do:

var m = /Windows NT (\d+(?:\.\d+))?/.exec(navigator.userAgent)
var isWin8 =  m && m[1] > 6.2;
var win8Element = $('privacy-win8-data-settings');
if (win8Element)
  win8Element.hidden = !isWin8;

http://codereview.chromium.org/10854126/diff/6002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/browser_options_handler.cc (right):

http://codereview.chromium.org/10854126/diff/6002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler.cc:646:
web_ui()->CallJavascriptFunction("BrowserOptions.showWin8DataPrivacy");
This doesn't seem right to me. We don't really need an "RPC" to do this. We can
easily check if we are using win8 on the "client".

[MAD, 2012-08-15 19:29:41]

All done, please take another look...

Thanks!

BYE
MAD

https://chromiumcodereview.appspot.com/10854126/diff/6002/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/10854126/diff/6002/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:195: <if expr="is_win">
On 2012/08/15 14:33:31, arv wrote:
> This will cause unbalanced <div>s. You probably want the <if> on line 196
> instead.

Oupss... I missed one... Sorry...
Fixed :-)

https://chromiumcodereview.appspot.com/10854126/diff/6002/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/10854126/diff/6002/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.js:1198:
$('privacy-win8-data-settings').hidden = false;
On 2012/08/15 14:33:31, arv wrote:
> How about, in initializePage we do:
> 
> var m = /Windows NT (\d+(?:\.\d+))?/.exec(navigator.userAgent)
> var isWin8 =  m && m[1] > 6.2;
> var win8Element = $('privacy-win8-data-settings');
> if (win8Element)
>   win8Element.hidden = !isWin8;

Ho! Cool, I didn't think about that...
Thanks!... Done!

https://chromiumcodereview.appspot.com/10854126/diff/6002/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/browser_options_handler.cc (right):

https://chromiumcodereview.appspot.com/10854126/diff/6002/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/browser_options_handler.cc:646:
web_ui()->CallJavascriptFunction("BrowserOptions.showWin8DataPrivacy");
On 2012/08/15 14:33:31, arv wrote:
> This doesn't seem right to me. We don't really need an "RPC" to do this. We
can
> easily check if we are using win8 on the "client".

Done.

[arv (Not doing code reviews), 2012-08-15 19:57:48]

LGTM

[commit-bot: I haz the power, 2012-08-15 20:30:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10854126/10008

[commit-bot: I haz the power, 2012-08-15 20:31:03]

Presubmit check for 10854126-10008 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.0s to calculate.

[MAD, 2012-08-15 20:38:05]

Need chrome\OWNERS review... James?

Thanks!

BYE
MAD

[MAD, 2012-08-16 19:21:20]

Ping?

Or should I ask someone else?

On Wed, Aug 15, 2012 at 4:38 PM, <mad@chromium.org> wrote:

> Need chrome\OWNERS review... James?
>
> Thanks!
>
> BYE
> MAD
>
>
>
https://chromiumcodereview.**appspot.com/10854126/<https://chromiumcodereview...
>

[MAD, 2012-08-16 19:28:19]

Ho! James is on vacation...

Lei, can you OWNERS validate this CL or should I ask someone in the
chrome/OWNERS file?

Thanks!

BYE
MAD...

[Lei Zhang, 2012-08-17 02:28:13]

I'm on vacation too. :)

LGTM with nits below.

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/app/generat...
chrome/app/generated_resources.grd:10908: +      <message
name="IDS_OPTIONS_PRIVACY_WIN8_DATA" desc="The text in the options panel
describes that the user data is handled differently on Windows 8.">
Can you put this inside an 'is_win' tag so this isn't included on !WIN
platforms?

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/browser/res...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/browser/res...
chrome/browser/resources/options2/browser_options.js:248: var winVerMatch =
/Windows NT (\d+(?:\.\d+))?/.exec(navigator.userAgent);
If this is generally useful, please consider moving into into a common .js file
to share with other WebUIs.

[MAD, 2012-08-17 17:34:09]

Thanks...
Sorry about the vacation, I didn't know... :-/

I added more Windows-only conditional compile stuff and also noticed that a
string had already been added to grit for this. But had to modify the existing
string to not include HTML (cause I couldn't get it to work, and I think it's
better to have the URL in url_constant.h, and also reuse the existing "Learn
more" text, right?)...

I'll give this a shot through the try server first, and then hit the CQ if you
have no objections...

BYE
MAD

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/app/generat...
chrome/app/generated_resources.grd:10908: +      <message
name="IDS_OPTIONS_PRIVACY_WIN8_DATA" desc="The text in the options panel
describes that the user data is handled differently on Windows 8.">
On 2012/08/17 02:28:13, Lei Zhang wrote:
> Can you put this inside an 'is_win' tag so this isn't included on !WIN
> platforms?

Done.

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/browser/res...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/10854126/diff/10008/chrome/browser/res...
chrome/browser/resources/options2/browser_options.js:248: var winVerMatch =
/Windows NT (\d+(?:\.\d+))?/.exec(navigator.userAgent);
On 2012/08/17 02:28:13, Lei Zhang wrote:
> If this is generally useful, please consider moving into into a common .js
file
> to share with other WebUIs.

I doubt (and actually hope) it won't be used much...
And since we want to merge this to M22, I would prefer to keep the CL as small
as possible...

But if I get to need this again, I promise I won't copy the code, I'll extract
it... :-)

[commit-bot: I haz the power, 2012-08-17 20:23:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10854126/1010

[commit-bot: I haz the power, 2012-08-18 00:55:42]

Try job failure for 10854126-1010 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-18 01:49:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10854126/1010

[commit-bot: I haz the power, 2012-08-18 03:25:15]

Try job failure for 10854126-1010 (retry) on linux_rel for step
"interactive_ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-20 13:22:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10854126/1010

[commit-bot: I haz the power, 2012-08-20 13:22:31]

Failed to apply patch for
chrome/browser/resources/options2/browser_options.html:
While running patch -p1 --forward --force;
A         chrome/browser/resources/options2
patching file chrome/browser/resources/options2/browser_options.html
Hunk #1 FAILED at 193.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/resources/options2/browser_options.html.rej

[commit-bot: I haz the power, 2012-08-20 13:52:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10854126/2004

[commit-bot: I haz the power, 2012-08-20 16:17:42]

Change committed as 152333