Chrome Frame turndown prompt.

Chrome Frame turndown prompt.

Display a prompt to users informing them that Chrome Frame is going away.

TBR=cpu@chromium.org
BUG=
TEST=visit a site rendered in GCF. notice the bar. interact with it. once it goes away, it stays away for a week.

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

[robertshield, 2013-06-19 23:36:33]

Reviewed up to but not including turndown_prompt_content.h, more coming in a
bit.

https://codereview.chromium.org/17153006/diff/1/chrome/app/policy/policy_temp...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/17153006/diff/1/chrome/app/policy/policy_temp...
chrome/app/policy/policy_templates.json:4799: 'dynamic_refresh': False,
False is probably fine, but does this make it clear to admins that a restart is
needed for this to take effect? We should mention the restart requirement in any
post announcing the group policy setting too.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/test/policy_sett...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/test/policy_sett...
chrome_frame/test/policy_settings_unittest.cc:120: }
The return value for this function is never used, but were it to be, I would
expect it to return policy_key.WriteValue() == ERROR_SUCCESS

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/reshow_state.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/reshow_state.cc:43: FILETIME last_shown_filetime =
last_shown_time.ToFileTime();
Any reason not to use Time::ToInternalValue() and FromInternalValue()? Those are
recommended in time.h for serializing and would simplify things a bit here.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/turndown_prompt.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:32: // Time between showingings
of the turndown prompt.
Too many ings

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:50: // Shows the turndown prompt
if it hasn't been seen since the reshow delta.
reshow delta -> kTurndownPromptReshowDeltaMinutes

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:53: InfobarManager*
GetInfobarManager();
The pointer this returns has a non-obvious life-cycle as stated below. Please
mention that here too.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:128: // Windows events.
As an unfamiliar reader, I don't know how to parse that comment. Which events
need I be wary of?
Would it be clearer (and accurate) to say that the pointer is safe to use in
this scope, but not to be held on to for future use?

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:142:
reshow_state.MarkShown(base::Time::Now());
Random thought: if I open two tabs both rendering in CF is it possible that the
first tab that is opened will display the prompt while the second tab is
visible? Should we somehow check if the tab is visible when calling MarkShown()?
Is this overkill? Am I asking too many questions?

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:164: HWND web_browserhwnd =
NULL;
nit: web_browser_hwnd

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:176: bool
CreateWebBrowserAdapter(ReadyModeWebBrowserAdapter** pointer) {
nit: pointer -> adapter

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:220: // multi-install).
I wonder if we should check the MSI bit as well?

[grt (UTC plus 2), 2013-06-20 19:55:29]

Thanks for the comments, PTAL.

The TODO list is down to:
- implement uninstall
- change Dismiss button to x
- somehow neuter the current npchrome_frame on uninstall

Do you think these are suitable for follow-on CLs?

https://codereview.chromium.org/17153006/diff/1/chrome/app/policy/policy_temp...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/17153006/diff/1/chrome/app/policy/policy_temp...
chrome/app/policy/policy_templates.json:4799: 'dynamic_refresh': False,
On 2013/06/19 23:36:33, robertshield wrote:
> False is probably fine, but does this make it clear to admins that a restart
is
> needed for this to take effect? We should mention the restart requirement in
any
> post announcing the group policy setting too.

Good point. This setting is surfaced in the documentation as "Dynamic Policy
Refresh: No". I don't know if those words are intuitively clear to someone
familiar with Group Policy or not.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/test/policy_sett...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/test/policy_sett...
chrome_frame/test/policy_settings_unittest.cc:120: }
On 2013/06/19 23:36:33, robertshield wrote:
> The return value for this function is never used, but were it to be, I would
> expect it to return policy_key.WriteValue() == ERROR_SUCCESS

Rejiggered.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/reshow_state.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/reshow_state.cc:43: FILETIME last_shown_filetime =
last_shown_time.ToFileTime();
On 2013/06/19 23:36:33, robertshield wrote:
> Any reason not to use Time::ToInternalValue() and FromInternalValue()? Those
are
> recommended in time.h for serializing and would simplify things a bit here.

Ah. I used FILETIME since that's common in the Windows registry. Simpler code is
simpler, however, so I'll change it.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/turndown_prompt.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:32: // Time between showingings
of the turndown prompt.
On 2013/06/19 23:36:33, robertshield wrote:
> Too many ings

Done.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:50: // Shows the turndown prompt
if it hasn't been seen since the reshow delta.
On 2013/06/19 23:36:33, robertshield wrote:
> reshow delta -> kTurndownPromptReshowDeltaMinutes

Done.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:53: InfobarManager*
GetInfobarManager();
On 2013/06/19 23:36:33, robertshield wrote:
> The pointer this returns has a non-obvious life-cycle as stated below. Please
> mention that here too.

Done.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:128: // Windows events.
On 2013/06/19 23:36:33, robertshield wrote:
> As an unfamiliar reader, I don't know how to parse that comment. Which events
> need I be wary of?
> Would it be clearer (and accurate) to say that the pointer is safe to use in
> this scope, but not to be held on to for future use?

Done.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:142:
reshow_state.MarkShown(base::Time::Now());
On 2013/06/19 23:36:33, robertshield wrote:
> Random thought: if I open two tabs both rendering in CF is it possible that
the
> first tab that is opened will display the prompt while the second tab is
> visible?

I think this is possible.

> Should we somehow check if the tab is visible when calling MarkShown()?

I think simple wins over perfect here. How simple would that be?

> Is this overkill?

Maybe.

> Am I asking too many questions?

No.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:164: HWND web_browserhwnd =
NULL;
On 2013/06/19 23:36:33, robertshield wrote:
> nit: web_browser_hwnd

Done.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:176: bool
CreateWebBrowserAdapter(ReadyModeWebBrowserAdapter** pointer) {
On 2013/06/19 23:36:33, robertshield wrote:
> nit: pointer -> adapter

Done.

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:220: // multi-install).
On 2013/06/19 23:36:33, robertshield wrote:
> I wonder if we should check the MSI bit as well?

Done.

[robertshield, 2013-06-20 21:45:10]

change lgtm

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/turndown_prompt.cc (right):

https://codereview.chromium.org/17153006/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt.cc:142:
reshow_state.MarkShown(base::Time::Now());
On 2013/06/20 19:55:29, grt wrote:
> On 2013/06/19 23:36:33, robertshield wrote:
> > Random thought: if I open two tabs both rendering in CF is it possible that
> the
> > first tab that is opened will display the prompt while the second tab is
> > visible?
> 
> I think this is possible.

Yeah, was simply musing. I don't have an immediately trivial way of doing this
handy, so it's probably not worth the complexity.

> 
> > Should we somehow check if the tab is visible when calling MarkShown()?
> 
> I think simple wins over perfect here. How simple would that be?
> 
> > Is this overkill?
> 
> Maybe.
> 
> > Am I asking too many questions?
> 
> No.

https://codereview.chromium.org/17153006/diff/13001/chrome_frame/turndown_pro...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/17153006/diff/13001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:51: // Substitue the
proper text given the current IE version.
Substitute

[grt (UTC plus 2), 2013-06-21 19:25:57]

PTAL, now with uninstall.

https://codereview.chromium.org/17153006/diff/13001/chrome_frame/turndown_pro...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/17153006/diff/13001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:51: // Substitue the
proper text given the current IE version.
On 2013/06/20 21:45:10, robertshield wrote:
> Substitute

Done.

[Joao da Silva, 2013-06-21 20:00:15]

policy/ lgtm

[robertshield, 2013-06-21 20:06:44]

lgtm

https://codereview.chromium.org/17153006/diff/23001/chrome_frame/turndown_pro...
File chrome_frame/turndown_prompt/turndown_prompt.cc (right):

https://codereview.chromium.org/17153006/diff/23001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt.cc:214: // Attempts to install
Turnown prompts in the provided web browser.
Turndown

[grt (UTC plus 2), 2013-06-21 20:10:39]

thanks.

https://codereview.chromium.org/17153006/diff/23001/chrome_frame/turndown_pro...
File chrome_frame/turndown_prompt/turndown_prompt.cc (right):

https://codereview.chromium.org/17153006/diff/23001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt.cc:214: // Attempts to install
Turnown prompts in the provided web browser.
On 2013/06/21 20:06:45, robertshield wrote:
> Turndown

Done.

[commit-bot: I haz the power, 2013-06-21 20:11:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/36001

[commit-bot: I haz the power, 2013-06-21 20:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/16005

[commit-bot: I haz the power, 2013-06-21 21:35:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 23:20:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/33003

[commit-bot: I haz the power, 2013-06-22 00:54:10]

Retried try job too often on mac_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-22 12:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/28002

[commit-bot: I haz the power, 2013-06-22 12:25:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/28002

[commit-bot: I haz the power, 2013-06-22 14:39:24]

Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/app/policy/policy_templates.json
  Hunk #1 FAILED at 112.
  Hunk #2 succeeded at 4905 (offset 15 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

Patch:       chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
diff --git a/chrome/app/policy/policy_templates.json
b/chrome/app/policy/policy_templates.json
index
52680ca8ee898a0f1a81c00a7cef669e38fcac51..5f0085692248a494be6a6dc90fc6fd9f85eb0b2d
100644
--- a/chrome/app/policy/policy_templates.json
+++ b/chrome/app/policy/policy_templates.json
@@ -112,7 +112,7 @@
 #   persistent IDs for all fields (but not for groups!) are needed. These are
 #   specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
 #   because doing so would break the deployed wire format!
-#   For your editing convenience: highest ID currently used: 219
+#   For your editing convenience: highest ID currently used: 220
 #
 # Placeholders:
 #   The following placeholder strings are automatically substituted:
@@ -4890,6 +4890,19 @@
       ],
     },
     {
+      'name': 'SuppressChromeFrameTurndownPrompt',
+      'caption': '''Suppress the <ph name="PRODUCT_FRAME_NAME">$3<ex>Google
Chrome Frame</ex></ph> turndown prompt''',
+      'desc': '''Suppresses the turndown prompt that appears when a site is
rendered by <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome
Frame</ex></ph>.''',
+      'type': 'main',
+      'schema': { 'type': 'boolean' },
+      'supported_on': ['chrome_frame:29-'],
+      'features': {
+        'dynamic_refresh': False,
+      },
+      'example_value': True,
+      'id': 220,
+    },
+    {
       'name': 'LocallyManagedUsers',
       'type': 'group',
       'caption': '''Locally managed users settings''',

[commit-bot: I haz the power, 2013-06-22 18:05:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/28002

[commit-bot: I haz the power, 2013-06-22 18:05:15]

Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/app/policy/policy_templates.json
  Hunk #1 FAILED at 112.
  Hunk #2 succeeded at 4905 (offset 15 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

Patch:       chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
diff --git a/chrome/app/policy/policy_templates.json
b/chrome/app/policy/policy_templates.json
index
52680ca8ee898a0f1a81c00a7cef669e38fcac51..5f0085692248a494be6a6dc90fc6fd9f85eb0b2d
100644
--- a/chrome/app/policy/policy_templates.json
+++ b/chrome/app/policy/policy_templates.json
@@ -112,7 +112,7 @@
 #   persistent IDs for all fields (but not for groups!) are needed. These are
 #   specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
 #   because doing so would break the deployed wire format!
-#   For your editing convenience: highest ID currently used: 219
+#   For your editing convenience: highest ID currently used: 220
 #
 # Placeholders:
 #   The following placeholder strings are automatically substituted:
@@ -4890,6 +4890,19 @@
       ],
     },
     {
+      'name': 'SuppressChromeFrameTurndownPrompt',
+      'caption': '''Suppress the <ph name="PRODUCT_FRAME_NAME">$3<ex>Google
Chrome Frame</ex></ph> turndown prompt''',
+      'desc': '''Suppresses the turndown prompt that appears when a site is
rendered by <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome
Frame</ex></ph>.''',
+      'type': 'main',
+      'schema': { 'type': 'boolean' },
+      'supported_on': ['chrome_frame:29-'],
+      'features': {
+        'dynamic_refresh': False,
+      },
+      'example_value': True,
+      'id': 220,
+    },
+    {
       'name': 'LocallyManagedUsers',
       'type': 'group',
       'caption': '''Locally managed users settings''',

[commit-bot: I haz the power, 2013-06-22 18:09:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/48004

[commit-bot: I haz the power, 2013-06-22 19:17:50]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-22 19:34:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/17153006/48004

[commit-bot: I haz the power, 2013-06-22 22:16:35]

Change committed as 208093