Show online policy on CrOS EULA page

CL modifies behavior of 'chrome://terms'.
It should show online policy on CrOS. In case of timeout (10 sec proposed by Nikita) the static version is shown. 
The real online resource isn't available at the moment.


BUG=174271
TBR=cpu@chromium.org

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

[vasilii, 2013-04-15 17:24:25]

Hi,

Please review the implementation and share your thoughts.

[Evan Stade, 2013-04-15 19:35:08]

ping me when the others are done reviewing it. If it changes I'd have to redo
the OWNERS review.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:78: get buttons() {
this function should not use the getter style because it's pretty complicated
and creates stuff in addition to returning it.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:102: const int kOnlineTermsTimeout = 5; //
seconds
two spaces before //. Also please document what this is rather than just the
unit.

[Nikita (slow), 2013-04-16 04:36:35]

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:78: get buttons() {
On 2013/04/15 19:35:08, Evan Stade wrote:
> this function should not use the getter style because it's pretty complicated
> and creates stuff in addition to returning it.

Evan, thanks for the comment. I agree that this should be refactored. We're in 2
weeks refactoring hackathon and we'll take this into consideration.

This method naming is common across several UI flows so it has to be updated at
once.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:102: const int kOnlineTermsTimeout = 5; //
seconds
nit: You could just rename it to kOnlineTermsTimeoutSec and change comment as
Evan suggested.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:151: // TODO(vasilii): request a real EULA
specific to locale.
Is this part of the upcoming CL? I think this should be trivial with the only
exception that some of the Chrome locale constants will not match those that are
used on server side.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:159: // Abort the download attempt if it
takes longer than one minute.
What would be UX during this time? Is there some feedback on UI? In case of
public accounts TOS there's "Loading" string.

Also 5 seconds seems a bit short timeframe for this.
10-15 seconds is preferable.

[battre, 2013-04-16 08:34:22]

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:150: fetch_callback_(callback) {
nit:
: in next line, +2 indentation

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:154: "privacy/eula_text.html"),
net::URLFetcher::GET, this));
This should be an i18n-ized URL

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:162: this,
what's the lifetime of |this|? Is it guaranteed to survive until the callback is
triggered?

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:178: // This object is to be allocated on
the heap only.
I don't understand this comment. I think what you want to say is that |this|
takes care of destruction.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:195: }
nit newline.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:198:
base::OneShotTimer<ChromeOSOnlineTermsHandler> download_timer_;
nit: newline.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:200: FetchCallback fetch_callback_;
nit: newline.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:241:
base::Bind(&ChromeOSTermsHandler::OnOnlineEULAFetched, this));
Is it guaranteed that |this| survives the callback?

[vasilii, 2013-04-16 15:40:18]

Dominic, Nikita: I addressed your comments. Added localization support and
"Loading" notification.

Carlos: please review chromeos_strings.grdp.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:102: const int kOnlineTermsTimeout = 5; //
seconds
On 2013/04/16 04:36:35, Nikita Kostylev wrote:
> nit: You could just rename it to kOnlineTermsTimeoutSec and change comment as
> Evan suggested.

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:150: fetch_callback_(callback) {
On 2013/04/16 08:34:22, battre wrote:
> nit:
> : in next line, +2 indentation

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:151: // TODO(vasilii): request a real EULA
specific to locale.
On 2013/04/16 04:36:35, Nikita Kostylev wrote:
> Is this part of the upcoming CL? I think this should be trivial with the only
> exception that some of the Chrome locale constants will not match those that
are
> used on server side.

Added localization support. But URL is still fake (relevant to Desktop Chrome).
The real one is unavailable.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:154: "privacy/eula_text.html"),
net::URLFetcher::GET, this));
On 2013/04/16 08:34:22, battre wrote:
> This should be an i18n-ized URL

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:159: // Abort the download attempt if it
takes longer than one minute.
On 2013/04/16 04:36:35, Nikita Kostylev wrote:
> What would be UX during this time? Is there some feedback on UI? In case of
> public accounts TOS there's "Loading" string.
> 
> Also 5 seconds seems a bit short timeframe for this.
> 10-15 seconds is preferable.

Changed timeout to 10 sec. Added "Loading" message as TOS screen shows.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:162: this,
On 2013/04/16 08:34:22, battre wrote:
> what's the lifetime of |this|? Is it guaranteed to survive until the callback
is
> triggered?

Object destroys itself in either OnURLFetchComplete() or OnDownloadTimeout().
Both OneShotTimer and URLFetcher are destroyed and the second callback is
cancelled automatically.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:178: // This object is to be allocated on
the heap only.
On 2013/04/16 08:34:22, battre wrote:
> I don't understand this comment. I think what you want to say is that |this|
> takes care of destruction.

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:195: }
On 2013/04/16 08:34:22, battre wrote:
> nit newline.

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:198:
base::OneShotTimer<ChromeOSOnlineTermsHandler> download_timer_;
On 2013/04/16 08:34:22, battre wrote:
> nit: newline.

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:200: FetchCallback fetch_callback_;
On 2013/04/16 08:34:22, battre wrote:
> nit: newline.

Done.

https://codereview.chromium.org/13820014/diff/2001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:241:
base::Bind(&ChromeOSTermsHandler::OnOnlineEULAFetched, this));
On 2013/04/16 08:34:22, battre wrote:
> Is it guaranteed that |this| survives the callback?

ChromeOSTermsHandler is ref-counted class. base::Bind returns something that
holds a scoped_refptr to it. Therefore when OnOnlineEULAFetched is called, there
is still alive scoped_refptr<ChromeOSTermsHandler>.

[cpu_(ooo_6.6-7.5), 2013-04-16 21:28:52]

grd changes can be TBR thanks.

[battre, 2013-04-17 10:41:19]

lgtm

https://codereview.chromium.org/13820014/diff/12001/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/13820014/diff/12001/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:145: // Helper class that fetches the online
Chrome OS terms.
Add comment that it returns an empty string in case fetching the online Chrome
OS terms fails or times out after |kOnlineTermsTimeoutSec|?

[Nikita (slow), 2013-04-17 12:49:41]

lgtm

[vasilii, 2013-04-17 16:58:56]

Evan: it's time. The only pending thing is a real URL in chromeos_strings.grdp.

https://codereview.chromium.org/13820014/diff/12001/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/13820014/diff/12001/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:145: // Helper class that fetches the online
Chrome OS terms.
On 2013/04/17 10:41:19, battre wrote:
> Add comment that it returns an empty string in case fetching the online Chrome
> OS terms fails or times out after |kOnlineTermsTimeoutSec|?

Done.

[Evan Stade, 2013-04-17 23:02:43]

lgtm

https://codereview.chromium.org/13820014/diff/26001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.html (right):

https://codereview.chromium.org/13820014/diff/26001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/oobe_screen_eula.html:6:
src="about:blank"></iframe>
2 more indent

[battre, 2013-04-18 15:59:43]

still LGTM after checking the return code.

[commit-bot: I haz the power, 2013-04-19 09:20:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/13820014/40001

[commit-bot: I haz the power, 2013-04-19 11:08:13]

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

[commit-bot: I haz the power, 2013-04-19 12:00:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/13820014/40001

[commit-bot: I haz the power, 2013-04-19 13:07:42]

Change committed as 195152

[battre, 2013-04-22 08:13:19]

Sorry that I spotted this so late.

https://chromiumcodereview.appspot.com/13820014/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/webui/about_ui.cc (right):

https://chromiumcodereview.appspot.com/13820014/diff/40001/chrome/browser/ui/...
chrome/browser/ui/webui/about_ui.cc:159:
eula_fetcher_->AddExtraRequestHeader("Accept: text/html");
Can you please add

  eula_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                              net::LOAD_DO_NOT_SAVE_COOKIES);

Optionally you can add | net::LOAD_DISABLE_CACHE you can discuss this with
Sumit).

Thanks.