Move the supervised user error page to a component

Move the supervised user error page to a component

This is a refactoring to allow WebView to use the supervised user error
page.

BUG=594973
Committed: https://crrev.com/c987fed5acb7cad3d489ebe7c31fd5a6bd265823
Cr-Commit-Position: refs/heads/master@{#382015}

[aberent, 2016-03-16 13:49:53]

aberent@chromium.org changed reviewers:
+ bauerb@chromium.org

[aberent, 2016-03-16 13:49:54]

Still needs GYP files, and possibly some sort of test, but please take a first
look.

[Bernhard Bauer, 2016-03-16 13:59:45]

https://codereview.chromium.org/1808653003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1808653003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7667: +       <message
name="IDS_BLOCK_INTERSTITIAL_DEFAULT_FEEDBACK_TEXT" desc="The default text for
feedback about a blocked site.">
Why do we keep this message?

https://codereview.chromium.org/1808653003/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_interstitial.h (right):

https://codereview.chromium.org/1808653003/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_interstitial.h:85:
supervised_user_error_page::FilteringBehaviorReason reason);
Add an empty line before comments please.

But actually, this method doesn't seem to be implemented?

https://codereview.chromium.org/1808653003/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_interstitial.h:86: // Owns the
interstitial, which owns us.
Add an empty line before comments please.

https://codereview.chromium.org/1808653003/diff/1/components/resources/compon...
File components/resources/components_resources.grd (right):

https://codereview.chromium.org/1808653003/diff/1/components/resources/compon...
components/resources/components_resources.grd:23: <part
file="supervised_user_error_page_resources.grdp" />
Ordering nit: move this one line up.

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_error_page.cc
(right):

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_error_page.cc:56: return
is_child_account
Could you split these up into several if () { return } blocks?

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_error_page.h (right):

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_error_page.h:22: extern
int GetBlockMessageID(
Why extern? Do you need to export this for a component build? (In which case,
use a macro).

[aberent, 2016-03-16 16:06:41]

https://codereview.chromium.org/1808653003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1808653003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7667: +       <message
name="IDS_BLOCK_INTERSTITIAL_DEFAULT_FEEDBACK_TEXT" desc="The default text for
feedback about a blocked site.">
On 2016/03/16 13:59:44, bauerb catching up on reviews wrote:
> Why do we keep this message?

It is used in SupervisedUserInterstitial::CommandReceived, and not in building
error page, so doesn't belong in the component (at least in its current minimal
form). I haven't yet looked at whether Khannan had to duplicate this in WebView,
in which case it might be sensible to move this, and the associated code, into
the component.

https://codereview.chromium.org/1808653003/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_interstitial.h (right):

https://codereview.chromium.org/1808653003/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_interstitial.h:85:
supervised_user_error_page::FilteringBehaviorReason reason);
On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> Add an empty line before comments please.
> 
> But actually, this method doesn't seem to be implemented?

Removed. Forgot to clean up an intermediate stage in my refactoring.

https://codereview.chromium.org/1808653003/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_interstitial.h:86: // Owns the
interstitial, which owns us.
On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> Add an empty line before comments please.

Done.

https://codereview.chromium.org/1808653003/diff/1/components/resources/compon...
File components/resources/components_resources.grd (right):

https://codereview.chromium.org/1808653003/diff/1/components/resources/compon...
components/resources/components_resources.grd:23: <part
file="supervised_user_error_page_resources.grdp" />
On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> Ordering nit: move this one line up.

Done.

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_error_page.cc
(right):

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_error_page.cc:56: return
is_child_account
On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> Could you split these up into several if () { return } blocks?

You want me to improve the code as well as moving it!!!

Done.

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_error_page.h (right):

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_error_page.h:22: extern
int GetBlockMessageID(
On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> Why extern? Do you need to export this for a component build? (In which case,
> use a macro).

I think my C++ just went back 20 years! Some linkers used to create multiple
definitions if functions in header files were not declared extern. Modern C++
forbids this.

I don't think it is needed for component builds; as I have defined the GN file
this is built as part of the browser or WebView, rather than as a separate
library.

Removed.

[Bernhard Bauer, 2016-03-16 17:12:27]

LGTM with some final cleanup nits:

https://codereview.chromium.org/1808653003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1808653003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7667: +       <message
name="IDS_BLOCK_INTERSTITIAL_DEFAULT_FEEDBACK_TEXT" desc="The default text for
feedback about a blocked site.">
On 2016/03/16 16:06:40, aberent wrote:
> On 2016/03/16 13:59:44, bauerb catching up on reviews wrote:
> > Why do we keep this message?
> 
> It is used in SupervisedUserInterstitial::CommandReceived, and not in building
> error page, so doesn't belong in the component (at least in its current
minimal
> form). I haven't yet looked at whether Khannan had to duplicate this in
WebView,
> in which case it might be sensible to move this, and the associated code, into
> the component.

Oh right, it's part of the feedback dialog, not the interstitial page. Sorry for
the confusion!

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_error_page.cc
(right):

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_error_page.cc:56: return
is_child_account
On 2016/03/16 16:06:41, aberent wrote:
> On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> > Could you split these up into several if () { return } blocks?
> 
> You want me to improve the code as well as moving it!!!
> 
> Done.

I know, I'm a slave driver. Thanks!

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_error_page.h (right):

https://codereview.chromium.org/1808653003/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_error_page.h:22: extern
int GetBlockMessageID(
On 2016/03/16 16:06:41, aberent wrote:
> On 2016/03/16 13:59:45, bauerb catching up on reviews wrote:
> > Why extern? Do you need to export this for a component build? (In which
case,
> > use a macro).
> 
> I think my C++ just went back 20 years! Some linkers used to create multiple
> definitions if functions in header files were not declared extern. Modern C++
> forbids this.

Yes, these days only constants get internal linkage by default.

> I don't think it is needed for component builds; as I have defined the GN file
> this is built as part of the browser or WebView, rather than as a separate
> library.
> 
> Removed.

https://codereview.chromium.org/1808653003/diff/20001/components/supervised_u...
File components/supervised_user_error_page/supervised_user_error_page.cc
(right):

https://codereview.chromium.org/1808653003/diff/20001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.cc:56: if
(is_child_account) {
Invert this condition so you can return early?

https://codereview.chromium.org/1808653003/diff/20001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.cc:58: return
IDS_CHILD_BLOCK_MESSAGE_DEFAULT_SINGLE_PARENT;
No braces necessary for one-line bodies, and no else necessary if the preceding
branch returns.

[aberent, 2016-03-16 19:47:40]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 19:48:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808653003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808653003/20001

[commit-bot: I haz the power, 2016-03-16 20:03:04]

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

[commit-bot: I haz the power, 2016-03-16 20:03:05]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aberent, 2016-03-18 12:50:00]

aberent@chromium.org changed reviewers:
+ jochen@chromium.org, thakis@chromium.org

[aberent, 2016-03-18 12:50:01]

jochen@chromium.org: Please approve, as //components owner, the addition of a
new component, and the related changes to //component level files. Also please
approve changes to //chrome/browser/BUILD.gn

thakis@chromium.org: Please approve, as //ui owner, the new dependency of the
component on //ui/base.

bauerb@chromium.org: PTAL, particularly at the new tests.

https://codereview.chromium.org/1808653003/diff/20001/components/supervised_u...
File components/supervised_user_error_page/supervised_user_error_page.cc
(right):

https://codereview.chromium.org/1808653003/diff/20001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.cc:56: if
(is_child_account) {
On 2016/03/16 17:12:27, Bernhard Bauer wrote:
> Invert this condition so you can return early?

Done.

https://codereview.chromium.org/1808653003/diff/20001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.cc:58: return
IDS_CHILD_BLOCK_MESSAGE_DEFAULT_SINGLE_PARENT;
On 2016/03/16 17:12:27, Bernhard Bauer wrote:
> No braces necessary for one-line bodies, and no else necessary if the
preceding
> branch returns.

Done.

[Bernhard Bauer, 2016-03-18 13:33:08]

Also, +Marc FYI

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
File
components/supervised_user_error_page/supervised_user_error_page_unittest.cc
(right):

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:32:
<< "reason = " << param.reason
Nit: Shouldn't the test framework print the parameter when it fails? Or is this
because the test framework can't stringify the struct?

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:74:
//  base::string16 custodian(base::UTF8ToUTF16("custodian"));
Remove this?

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:164:
IDS_CHILD_BLOCK_INTERSTITIAL_REQUEST_SENT_MESSAGE_SINGLE_PARENT))));
I think this doesn't fit into 80 columns anymore :-/

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:226:

Nit: remove this empty line?

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:232:
}  //  namespace supervised_user_error_page
Add an empty line before this one?

[Marc Treib, 2016-03-18 13:46:57]

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

[Marc Treib, 2016-03-18 13:46:58]

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
File components/supervised_user_error_page/OWNERS (right):

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/OWNERS:2: bauerb@chromium.org
Add the other supervised_user owners too?

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
File components/supervised_user_error_page/supervised_user_error_page.h (right):

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.h:40: #endif /*
COMPONENTS_SUPERVISED_USER_ERROR_PAGE_SUPERVISED_USER_ERROR_PAGE_H_ \
nit: // comment

[Nico, 2016-03-18 13:56:06]

ui/ dep lgtm

[aberent, 2016-03-18 14:03:28]

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
File components/supervised_user_error_page/OWNERS (right):

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/OWNERS:2: bauerb@chromium.org
On 2016/03/18 13:46:58, Marc Treib wrote:
> Add the other supervised_user owners too?

Done. Also in //components/resources/OWNERS (//components/OWNERS delegates to
this file so will pick this up automatically).

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
File components/supervised_user_error_page/supervised_user_error_page.h (right):

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.h:40: #endif /*
COMPONENTS_SUPERVISED_USER_ERROR_PAGE_SUPERVISED_USER_ERROR_PAGE_H_ \
On 2016/03/18 13:46:58, Marc Treib wrote:
> nit: // comment

Done.

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
File
components/supervised_user_error_page/supervised_user_error_page_unittest.cc
(right):

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:32:
<< "reason = " << param.reason
On 2016/03/18 13:33:08, Bernhard Bauer wrote:
> Nit: Shouldn't the test framework print the parameter when it fails? Or is
this
> because the test framework can't stringify the struct?

It prints it as a list of hex values, which isn't particularly helpful.

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:74:
//  base::string16 custodian(base::UTF8ToUTF16("custodian"));
On 2016/03/18 13:33:08, Bernhard Bauer wrote:
> Remove this?

Done.

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:164:
IDS_CHILD_BLOCK_INTERSTITIAL_REQUEST_SENT_MESSAGE_SINGLE_PARENT))));
On 2016/03/18 13:33:08, Bernhard Bauer wrote:
> I think this doesn't fit into 80 columns anymore :-/

True, but I am not sure what to do about it, short of splitting it into multiple
statements. This has been through git cl format, and passes the presubmit
checks. I haven't changed the resource names.

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:226:

On 2016/03/18 13:33:08, Bernhard Bauer wrote:
> Nit: remove this empty line?

Done.

https://codereview.chromium.org/1808653003/diff/60001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page_unittest.cc:232:
}  //  namespace supervised_user_error_page
On 2016/03/18 13:33:08, Bernhard Bauer wrote:
> Add an empty line before this one?

Done.

[aberent, 2016-03-18 14:03:58]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-18 14:04:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808653003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808653003/80001

[commit-bot: I haz the power, 2016-03-18 14:07:57]

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

[commit-bot: I haz the power, 2016-03-18 14:07:58]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aberent, 2016-03-18 14:18:44]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-18 14:18:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808653003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808653003/100001

[commit-bot: I haz the power, 2016-03-18 14:31:52]

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

[commit-bot: I haz the power, 2016-03-18 14:31:53]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[jochen (gone - plz use gerrit), 2016-03-18 15:31:11]

lgtm

[aberent, 2016-03-18 16:10:27]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-03-18 16:10:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
thakis@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1808653003/#ps120001
(title: "Rebase to fix compile problem")

[commit-bot: I haz the power, 2016-03-18 16:10:53]

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

[commit-bot: I haz the power, 2016-03-18 18:17:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-18 18:19:03]

Description was changed from

==========
Move the supervised user error page to a component

This is a refactoring to allow WebView to use the supervised user error
page.

BUG=594973
==========

to

==========
Move the supervised user error page to a component

This is a refactoring to allow WebView to use the supervised user error
page.

BUG=594973

Committed: https://crrev.com/c987fed5acb7cad3d489ebe7c31fd5a6bd265823
Cr-Commit-Position: refs/heads/master@{#382015}
==========

[commit-bot: I haz the power, 2016-03-18 18:19:05]

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

[jam, 2016-07-11 20:35:11]

Are there plans to make webview use this component? because I see that only
chrome/ depends on it. If there are no plans, should this change be reverted?

[aberent, 2016-07-12 09:41:13]

Yes, work in progress, see https://codereview.chromium.org/1890203002/

On Mon, 11 Jul 2016 at 21:35 <jam@chromium.org> wrote:

> Are there plans to make webview use this component? because I see that only
> chrome/ depends on it. If there are no plans, should this change be
> reverted?
>
> https://codereview.chromium.org/1808653003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2016-07-12 19:16:03]

On 2016/07/12 09:41:13, aberent wrote:
> Yes, work in progress, see https://codereview.chromium.org/1890203002/

Thanks

[mmenke, 2017-03-30 15:07:29]

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

[mmenke, 2017-03-30 15:07:30]

https://codereview.chromium.org/1808653003/diff/120001/components/resources/O...
File components/resources/OWNERS (right):

https://codereview.chromium.org/1808653003/diff/120001/components/resources/O...
components/resources/OWNERS:28: per-file
supervised_user_error_page.grpd=reib@chromium.org
I'm a bit late to the party, but should this be treib?

[Bernhard Bauer, 2017-03-30 15:08:17]

On 2017/03/30 15:07:30, mmenke wrote:
>
https://codereview.chromium.org/1808653003/diff/120001/components/resources/O...
> File components/resources/OWNERS (right):
> 
>
https://codereview.chromium.org/1808653003/diff/120001/components/resources/O...
> components/resources/OWNERS:28: per-file
> mailto:supervised_user_error_page.grpd=reib@chromium.org
> I'm a bit late to the party, but should this be treib?

Oops... yes, it should :) Thanks for catching it!

[aberent, 2017-03-30 15:14:34]

https://codereview.chromium.org/1808653003/diff/120001/components/resources/O...
File components/resources/OWNERS (right):

https://codereview.chromium.org/1808653003/diff/120001/components/resources/O...
components/resources/OWNERS:28: per-file
supervised_user_error_page.grpd=reib@chromium.org
On 2017/03/30 15:07:30, mmenke wrote:
> I'm a bit late to the party, but should this be treib?
Yes, very late (this landed over a year ago), but I think it probably should be.
Thanks. I will create a trivial CL to fix it.