Reflect the status of a request for accessing a blacklisted url.

When requesting access to a blacklisted url from the interstitial page, update the page with the request sent status.

Note:
I am exposing RenderFrameHost in the interstitial page in order to execute javascript as a means to communicate with the webpage.
The RenderViewHost was already being used in android for routing gestures. This is makes the exposure for all platforms.

BUG=443166
Committed: https://crrev.com/f3b271970f1a27ccc743d9d0ff08dde21d760076
Cr-Commit-Position: refs/heads/master@{#310789}

[khannan, 2014-12-17 14:05:09]

khannans@google.com changed reviewers:
+ bauerb@chromium.org, khannans@google.com

[khannan, 2014-12-17 14:05:10]

WIP. Yet to test properly.

To add:
chromium-reviews, pam+watch@chromium.org, arv+watch@chromium.org

[Bernhard Bauer, 2014-12-17 17:05:18]

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
File chrome/browser/resources/supervised_user_block_interstitial.html (right):

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
chrome/browser/resources/supervised_user_block_interstitial.html:25: <div
id="request-failed-message" i18n-content="requestFailedMessage" hidden></div>
Break this line so it fits into 80 columns.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
File chrome/browser/resources/supervised_user_block_interstitial.js (right):

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
chrome/browser/resources/supervised_user_block_interstitial.js:48: * @param
{boolean} isSuccessful
Document the parameter please.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
chrome/browser/resources/supervised_user_block_interstitial.js:50: function
requestStatus(isSuccessful) {
Name this method so it's in imperative?

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:234:
base::string16 request_sent_message, request_failed_message;
Declare each variable on its own line.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:240:
request_failed_message = l10n_util::GetStringUTF16(
I think it starts making sense to add another case to the if-statement here
(something like this:)

  if (is_child_account) {
    if (second_custodian.empty()) {
      request_failed_message = ...;
      reqest_send_message = ...;
    } else {
      ...
    }
  } else {
    ...
  }

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:317:
std::stringstream jsFunc;
Don't use string streams. Just concatenate the strings, or use 
base::StringPrintf. Also, local variables are named with underscore_style.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:319:
interstitial_page_->GetRenderViewHostForTesting()
I assume you are going to ensure this method will be available for production
use before landing this CL?

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:322: VLOG(1) <<
"Executed: " + jsFunc.str();
I don't think this log statement is going to be useful in production.

[Bernhard Bauer, 2014-12-17 17:09:17]

Also, it's not necessary to remove chromium-reviews@ from the CC list. The CL is
still public, so it makes sense to include chromium-reviews@ in the review
discussion, even if the CL is not in a state yet to land it.

[khannan, 2014-12-17 17:39:09]

+chromium-reviews@chromium.org

Addressed your comments. Some are left unchanged for testing.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
File chrome/browser/resources/supervised_user_block_interstitial.html (right):

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
chrome/browser/resources/supervised_user_block_interstitial.html:25: <div
id="request-failed-message" i18n-content="requestFailedMessage" hidden></div>
On 2014/12/17 17:05:17, Bernhard Bauer wrote:
> Break this line so it fits into 80 columns.

Done.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
File chrome/browser/resources/supervised_user_block_interstitial.js (right):

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
chrome/browser/resources/supervised_user_block_interstitial.js:48: * @param
{boolean} isSuccessful
On 2014/12/17 17:05:17, Bernhard Bauer wrote:
> Document the parameter please.

I tried emulating a bad js file so did not know how. Fixed

https://codereview.chromium.org/789703004/diff/1/chrome/browser/resources/sup...
chrome/browser/resources/supervised_user_block_interstitial.js:50: function
requestStatus(isSuccessful) {
On 2014/12/17 17:05:17, Bernhard Bauer wrote:
> Name this method so it's in imperative?

Done.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:234:
base::string16 request_sent_message, request_failed_message;
On 2014/12/17 17:05:18, Bernhard Bauer wrote:
> Declare each variable on its own line.

Done.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:240:
request_failed_message = l10n_util::GetStringUTF16(
On 2014/12/17 17:05:18, Bernhard Bauer wrote:
> I think it starts making sense to add another case to the if-statement here
> (something like this:)
> 
>   if (is_child_account) {
>     if (second_custodian.empty()) {
>       request_failed_message = ...;
>       reqest_send_message = ...;
>     } else {
>       ...
>     }
>   } else {
>     ...
>   }

Done.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:317:
std::stringstream jsFunc;
On 2014/12/17 17:05:18, Bernhard Bauer wrote:
> Don't use string streams. Just concatenate the strings, or use 
> base::StringPrintf. Also, local variables are named with underscore_style.

Done.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:319:
interstitial_page_->GetRenderViewHostForTesting()
On 2014/12/17 17:05:18, Bernhard Bauer wrote:
> I assume you are going to ensure this method will be available for production
> use before landing this CL?

Yes.

https://codereview.chromium.org/789703004/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_interstitial.cc:322: VLOG(1) <<
"Executed: " + jsFunc.str();
On 2014/12/17 17:05:18, Bernhard Bauer wrote:
> I don't think this log statement is going to be useful in production.

Yup. Test code.

[Bernhard Bauer, 2014-12-17 18:27:34]

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/supervised_user_block_interstitial.html (right):

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/supervised_user_block_interstitial.html:26:
hidden></div>
Just indent the following line four spaces, don't align parameters.

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/supervised_user_block_interstitial.js (right):

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/supervised_user_block_interstitial.js:50: function
updateOnGettingRequestStatus(isSuccessful) {
This method name does not parse very well (what does "update on getting request
status" mean?[*])

[*] Unless it means, "An update on the request status.

The update is, it failed." ;-)

[khannan, 2014-12-17 18:41:54]

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/supervised_user_block_interstitial.html (right):

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/supervised_user_block_interstitial.html:26:
hidden></div>
On 2014/12/17 18:27:34, Bernhard Bauer wrote:
> Just indent the following line four spaces, don't align parameters.

Done.

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/supervised_user_block_interstitial.js (right):

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/supervised_user_block_interstitial.js:50: function
updateOnGettingRequestStatus(isSuccessful) {
On 2014/12/17 18:27:34, Bernhard Bauer wrote:
> This method name does not parse very well (what does "update on getting
request
> status" mean?[*])
> 
> [*] Unless it means, "An update on the request status.
> 
> The update is, it failed." ;-)

Very Homonymic, How about:
update[Interstitial/Message]OnGettingRequestStatus()?
onRequestStatusReceipt()?
or something else?

[Bernhard Bauer, 2014-12-17 18:50:33]

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/supervised_user_block_interstitial.js (right):

https://codereview.chromium.org/789703004/diff/20001/chrome/browser/resources...
chrome/browser/resources/supervised_user_block_interstitial.js:50: function
updateOnGettingRequestStatus(isSuccessful) {
On 2014/12/17 18:41:53, khannan wrote:
> On 2014/12/17 18:27:34, Bernhard Bauer wrote:
> > This method name does not parse very well (what does "update on getting
> request
> > status" mean?[*])
> > 
> > [*] Unless it means, "An update on the request status.
> > 
> > The update is, it failed." ;-)
> 
> Very Homonymic, How about:
> update[Interstitial/Message]OnGettingRequestStatus()?
> onRequestStatusReceipt()?
> or something else?

How about just setRequestStatus()?

[khannan, 2014-12-22 14:45:52]

khannans@google.com changed reviewers:
+ jochen@chromium.org

[khannan, 2014-12-22 14:45:53]

+Jochen, Please add others who may want to have a look.

The crux of the change is exposing RenderViewHost in
content/public/browser/interstitial_page.h

[jochen (gone - plz use gerrit), 2014-12-22 14:49:21]

jochen@chromium.org changed reviewers:
+ creis@chromium.org

[jochen (gone - plz use gerrit), 2014-12-22 14:49:21]

+creis

[Bernhard Bauer, 2014-12-22 14:50:05]

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
content/public/browser/interstitial_page.h:83: // Used for routing gestures to
the interstitial in Android as it shares a
Nit: I would probably rephrase this slightly, to not create the impression that
this is *only* used on Android. Or maybe we can even delete that sentence
completely, now that this method is not Android-only anymore.

[Charlie Reis, 2015-01-07 20:55:38]

[+nasko,kenrb,site-isolation-reviews to CC]

Just one request about exposing RenderFrameHost rather than RenderViewHost, but
otherwise seems ok to me.

https://codereview.chromium.org/789703004/diff/40001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/789703004/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:345:
web_contents_->GetInterstitialPage()->GetRenderViewHost()->GetView();
This can probably become
GetInterstitialPage()->GetMainFrame()->GetRenderViewHost().  We'll be moving
GetView() to RenderFrameHost's widget soon in the effort to eliminate
RenderViewHost.

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
content/public/browser/interstitial_page.h:85: virtual RenderViewHost*
GetRenderViewHost() const = 0;
Can you expose RenderFrameHost* GetMainFrame() instead?  (That would be
frame_tree_.root()->current_frame_host() inside InterstitialPageImpl.)

The RenderViewHost class is going away as part of the out-of-process iframes /
Site Isolation work, and we shouldn't be adding new dependencies on it.  It
looks like you just need the main frame anyway.

[nasko, 2015-01-07 21:28:41]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2015-01-07 21:28:41]

nit: Neither the description of the CL, nor the bug contain reasoning why
RenderViewHost needs to be exposed. It requires reading one specific line of the
entire CL and it should be clearer.

[knn, 2015-01-07 22:27:18]

On 2015/01/07 21:28:41, nasko wrote:
> nit: Neither the description of the CL, nor the bug contain reasoning why
> RenderViewHost needs to be exposed. It requires reading one specific line of
the
> entire CL and it should be clearer.

Thanks, I'll be sure to specify the points I am asking review for below, the
change description from next time.

[nasko, 2015-01-07 22:47:51]

On 2015/01/07 22:27:18, knn wrote:
> On 2015/01/07 21:28:41, nasko wrote:
> > nit: Neither the description of the CL, nor the bug contain reasoning why
> > RenderViewHost needs to be exposed. It requires reading one specific line of
> the
> > entire CL and it should be clearer.
> 
> Thanks, I'll be sure to specify the points I am asking review for below, the
> change description from next time.

Why not on this CL? You can click "Edit Issue" and it will allow you to change
the CL description.
You could also add details in the bug itself, which currently has only vague one
sentence content.

[knn, 2015-01-07 23:50:28]

knn@chromium.org changed reviewers:
+ knn@chromium.org

[knn, 2015-01-07 23:50:29]

PTAL

https://codereview.chromium.org/789703004/diff/40001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/789703004/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:345:
web_contents_->GetInterstitialPage()->GetRenderViewHost()->GetView();
On 2015/01/07 20:55:38, Charlie Reis wrote:
> This can probably become
> GetInterstitialPage()->GetMainFrame()->GetRenderViewHost().  We'll be moving
> GetView() to RenderFrameHost's widget soon in the effort to eliminate
> RenderViewHost.

Done. Thanks!

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
content/public/browser/interstitial_page.h:83: // Used for routing gestures to
the interstitial in Android as it shares a
On 2014/12/22 14:50:05, Bernhard Bauer wrote:
> Nit: I would probably rephrase this slightly, to not create the impression
that
> this is *only* used on Android. Or maybe we can even delete that sentence
> completely, now that this method is not Android-only anymore.

Done.

https://codereview.chromium.org/789703004/diff/40001/content/public/browser/i...
content/public/browser/interstitial_page.h:85: virtual RenderViewHost*
GetRenderViewHost() const = 0;
On 2015/01/07 20:55:38, Charlie Reis wrote:
> Can you expose RenderFrameHost* GetMainFrame() instead?  (That would be
> frame_tree_.root()->current_frame_host() inside InterstitialPageImpl.)
> 
> The RenderViewHost class is going away as part of the out-of-process iframes /
> Site Isolation work, and we shouldn't be adding new dependencies on it.  It
> looks like you just need the main frame anyway.

Done.

[knn, 2015-01-07 23:54:47]

On 2015/01/07 22:47:51, nasko wrote:
> On 2015/01/07 22:27:18, knn wrote:
> > On 2015/01/07 21:28:41, nasko wrote:
> > > nit: Neither the description of the CL, nor the bug contain reasoning why
> > > RenderViewHost needs to be exposed. It requires reading one specific line
of
> > the
> > > entire CL and it should be clearer.
> > 
> > Thanks, I'll be sure to specify the points I am asking review for below, the
> > change description from next time.
> 
> Why not on this CL? You can click "Edit Issue" and it will allow you to change
> the CL description.
> You could also add details in the bug itself, which currently has only vague
one
> sentence content.

I have updated the issue. Should I add a comment to the bug as well?
I made the CL and bug with the intention of describing the change. (I'm sorry if
I did not capture it well)
While I agree that I should point relevant things to review in my CL for the
reviewers, should an implementation detail be mentioned in the context of a bug
as well?
I am new to the project and appreciate your feedback thanks!

[nasko, 2015-01-08 00:01:59]

On 2015/01/07 23:54:47, knn wrote:
> On 2015/01/07 22:47:51, nasko wrote:
> > On 2015/01/07 22:27:18, knn wrote:
> > > On 2015/01/07 21:28:41, nasko wrote:
> > > > nit: Neither the description of the CL, nor the bug contain reasoning
why
> > > > RenderViewHost needs to be exposed. It requires reading one specific
line
> of
> > > the
> > > > entire CL and it should be clearer.
> > > 
> > > Thanks, I'll be sure to specify the points I am asking review for below,
the
> > > change description from next time.
> > 
> > Why not on this CL? You can click "Edit Issue" and it will allow you to
change
> > the CL description.
> > You could also add details in the bug itself, which currently has only vague
> one
> > sentence content.
> 
> I have updated the issue. Should I add a comment to the bug as well?

The update you made to the CL is fine. No need to add it to the bug, as the two
are linked.

> I made the CL and bug with the intention of describing the change. (I'm sorry
if
> I did not capture it well)
> While I agree that I should point relevant things to review in my CL for the
> reviewers, should an implementation detail be mentioned in the context of a
bug
> as well?

Implementation details are totally fine in the bug report. In this case, the bug
report didn't explain much of what needs to be reflected where. Having some
details that for supervised users we don't show correct messaging and needs to
be fixed would've given at least some context as to what the problem is.

> I am new to the project and appreciate your feedback thanks!

No problem. Welcome to Chromium! I hope you enjoy it!

[Charlie Reis, 2015-01-08 00:05:01]

I don't see the new patch yet.  Can you upload it and run try jobs?  Thanks!

[khannans, 2015-01-08 19:29:27]

On 2015/01/08 00:05:01, Charlie Reis wrote:
> I don't see the new patch yet.  Can you upload it and run try jobs?  Thanks!

Sorry about that. Now you should be able to see the try job results.

[Charlie Reis, 2015-01-08 21:06:03]

Thanks!  content/ LGTM with one nit.

https://codereview.chromium.org/789703004/diff/80001/content/public/browser/i...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/789703004/diff/80001/content/public/browser/i...
content/public/browser/interstitial_page.h:82: // Gets the RenderFrameHost
associated with the Interstitial Page.
nit: with the interstitial page's main frame.

(Not capitalized, and emphasize main frame.)

[Bernhard Bauer, 2015-01-08 21:19:14]

LGTM!

[knn, 2015-01-08 21:46:13]

Done!

https://codereview.chromium.org/789703004/diff/80001/content/public/browser/i...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/789703004/diff/80001/content/public/browser/i...
content/public/browser/interstitial_page.h:82: // Gets the RenderFrameHost
associated with the Interstitial Page.
On 2015/01/08 21:06:02, Charlie Reis wrote:
> nit: with the interstitial page's main frame.
> 
> (Not capitalized, and emphasize main frame.)

Done.

[knn, 2015-01-08 22:00:18]

The CQ bit was checked by knn@chromium.org

[commit-bot: I haz the power, 2015-01-08 22:00:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789703004/100001

[knn, 2015-01-08 22:07:03]

The CQ bit was unchecked by knn@chromium.org

[jochen (gone - plz use gerrit), 2015-01-09 13:01:18]

lgtm

[knn, 2015-01-09 14:14:59]

The CQ bit was checked by knn@chromium.org

[commit-bot: I haz the power, 2015-01-09 14:15:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789703004/100001

[commit-bot: I haz the power, 2015-01-09 15:00:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-09 15:00:23]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[knn, 2015-01-09 15:15:25]

The CQ bit was checked by knn@chromium.org

[commit-bot: I haz the power, 2015-01-09 15:16:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789703004/120001

[knn, 2015-01-09 15:26:30]

The CQ bit was unchecked by knn@chromium.org

[knn, 2015-01-09 16:24:54]

The CQ bit was checked by knn@chromium.org

[commit-bot: I haz the power, 2015-01-09 16:25:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789703004/160001

[commit-bot: I haz the power, 2015-01-09 17:28:59]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-09 17:30:02]

Patchset 9 (id:??) landed as
https://crrev.com/f3b271970f1a27ccc743d9d0ff08dde21d760076
Cr-Commit-Position: refs/heads/master@{#310789}