Added the chrome://safe-browsing/ page as part of the chrome://about

Added the chrome://safe-browsing/ page as part of the chrome://about

Created the chrome://safe-browsing/ WebUI interface, and added it as
part of the list, in the chrome://about page.

Reference: I followed the steps in
https://www.chromium.org/developers/webui to create the interface.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2940963002
Cr-Commit-Position: refs/heads/master@{#483104}
Committed: https://chromium.googlesource.com/chromium/src/+/8c024eea933703d60dc129ad2feecd5765a2417c

[hkamila, 2017-06-14 22:29:27]

Description was changed from

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

BUG=
==========

to

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hkamila, 2017-06-14 22:31:32]

Description was changed from

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.
==========

[hkamila, 2017-06-14 22:31:32]

hkamila@google.com changed reviewers:
+ vakh@chromium.org

[hkamila, 2017-06-14 22:34:43]

The CQ bit was checked by hkamila@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 22:35:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 00:02:47]

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

[commit-bot: I haz the power, 2017-06-15 00:02:47]

Dry run: This issue passed the CQ dry run.

[hkamila, 2017-06-15 16:02:43]

hkamila@google.com changed reviewers:
+ jialiul@chromium.org

[hkamila, 2017-06-15 16:27:47]

Description was changed from

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.
==========

to

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

Reference: I followed the steps in https://www.chromium.org/developers/webui to
create the interface.
==========

[vakh (use Gerrit instead), 2017-06-15 22:51:14]

How do you feel about calling it chrome://safebrowsing i.e. drop the '-'?

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:453: <include name="IDR_SAFE_BROWSING_HTML"
file="resources\safe_browsing\safe_browsing.html" type="BINDATA" />
I'd prefer to have these files under components/safe_browsing/...

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/ui/webui/saf...
File chrome/browser/ui/webui/safe_browsing_ui.cc (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/ui/webui/saf...
chrome/browser/ui/webui/safe_browsing_ui.cc:25:
content::WebUIDataSource::Add(profile, html_source);
Does it mean that we can't open it in an Incognito window?

[Jialiu Lin, 2017-06-15 22:52:43]

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/safe_browsing.css (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/safe_browsing.css:1: p {
Please add copyright statement at the top. 

e.g.
https://cs.chromium.org/chromium/src/chrome/browser/resources/cast/cast.css?dr

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/safe_browsing.js (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/safe_browsing.js:1:
cr.define('safe_browsing', function() {
copy right.

[hkamila, 2017-06-16 01:29:07]

Description was changed from

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

Reference: I followed the steps in https://www.chromium.org/developers/webui to
create the interface.
==========

to

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

Reference: I followed the steps in https://www.chromium.org/developers/webui to
create the interface.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hkamila, 2017-06-16 16:01:45]

On 2017/06/15 22:51:14, vakh (Varun Khaneja) wrote:
> How do you feel about calling it chrome://safebrowsing i.e. drop the '-'?
> 
>
https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
> File chrome/browser/browser_resources.grd (right):
> 
>
https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
> chrome/browser/browser_resources.grd:453: <include
name="IDR_SAFE_BROWSING_HTML"
> file="resources\safe_browsing\safe_browsing.html" type="BINDATA" />
> I'd prefer to have these files under components/safe_browsing/...
> 
>
https://codereview.chromium.org/2940963002/diff/1/chrome/browser/ui/webui/saf...
> File chrome/browser/ui/webui/safe_browsing_ui.cc (right):
> 
>
https://codereview.chromium.org/2940963002/diff/1/chrome/browser/ui/webui/saf...
> chrome/browser/ui/webui/safe_browsing_ui.cc:25:
> content::WebUIDataSource::Add(profile, html_source);
> Does it mean that we can't open it in an Incognito window?
--------
> How do you feel about calling it chrome://safebrowsing i.e. drop the '-'? 

I don't think it would make much change, but it would be inconsistent with the
rest of the pages in Chrome://about/ which have two or more words in their
naming.

> I'd prefer to have these files under components/safe_browsing/...

That was my initial location of the files, but once I went through the guide,
the css, html and js files were saved in the resources\... and my reason for
going for "resources\..." was to keep it consistent. Please let me know if you
still want the files moved to "components\..."

[vakh (use Gerrit instead), 2017-06-16 16:08:55]

chrome://safe-browsing is fine too.

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:453: <include name="IDR_SAFE_BROWSING_HTML"
file="resources\safe_browsing\safe_browsing.html" type="BINDATA" />
On 2017/06/15 22:51:14, vakh (Varun Khaneja) wrote:
> I'd prefer to have these files under components/safe_browsing/...

Re: your comment about the location of the files, lots of other pages have their
UI in their page own directory:
See:
https://cs.chromium.org/search/?q=%22chrome://resources/js/cr.js%22+-f:src/ch...

[hkamila, 2017-06-16 20:53:25]

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:453: <include name="IDR_SAFE_BROWSING_HTML"
file="resources\safe_browsing\safe_browsing.html" type="BINDATA" />
On 2017/06/15 22:51:14, vakh (Varun Khaneja) wrote:
> I'd prefer to have these files under components/safe_browsing/...

That was my initial location of the files, but once I went through the guide,
the css, html and js files were saved in the resources\... and my reason for
going for "resources\..." was to keep it consistent. Please let me know if you
still want the files moved to "components\..."


---
I moved the files to components\... in patch #3.

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/safe_browsing.css (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/safe_browsing.css:1: p {
On 2017/06/15 22:52:43, Jialiu Lin wrote:
> Please add copyright statement at the top. 
> 
> e.g.
> https://cs.chromium.org/chromium/src/chrome/browser/resources/cast/cast.css?dr

I added the copyright statement in patch #2.

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/safe_browsing.js (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/safe_browsing.js:1:
cr.define('safe_browsing', function() {
On 2017/06/15 22:52:43, Jialiu Lin wrote:
> copy right. 

I added the copyright statement in patch #2.

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/ui/webui/saf...
File chrome/browser/ui/webui/safe_browsing_ui.cc (right):

https://codereview.chromium.org/2940963002/diff/1/chrome/browser/ui/webui/saf...
chrome/browser/ui/webui/safe_browsing_ui.cc:25:
content::WebUIDataSource::Add(profile, html_source);
On 2017/06/15 22:51:14, vakh (Varun Khaneja) wrote:
> Does it mean that we can't open it in an Incognito window?

Addressed this in patch #3.

[hkamila, 2017-06-19 16:30:54]

hkamila@google.com changed reviewers:
+ pkasting@chromium.org, tsergeant@chromium.org

[hkamila, 2017-06-19 16:30:56]

tsergeant@chromium.org: Please review changes in 

chrome/browser/browser_resources.grd
----
pkasting@chromium.org: Please review changes in 

chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
chrome/browser/ui/webui/safe_browsing_ui.h	
chrome/browser/ui/webui/safe_browsing_ui.cc

[vakh (use Gerrit instead), 2017-06-19 17:02:13]

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:406: "webui/safe_browsing_ui.cc",
Let's move these also into the components/safe_browsing/webui/ directory.

https://codereview.chromium.org/2940963002/diff/40001/components/safe_browsin...
File components/safe_browsing/resources/safe_browsing.css (right):

https://codereview.chromium.org/2940963002/diff/40001/components/safe_browsin...
components/safe_browsing/resources/safe_browsing.css:2: /* Copyright 2017 The
Chromium Authors. All rights reserved.
I know I said earlier that let's put this under
components/safe_browsing/resources/ directory, but I think a more suitable
location would be: components/safe_browsing/webui/resources/

See:
https://cs.chromium.org/chromium/src/components/dom_distiller/webui/?q=compon...

Sorry for not looking this up earlier.

[Peter Kasting, 2017-06-19 21:06:47]

LGTM

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/safe_browsing_ui.cc (right):

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/safe_browsing_ui.cc:24: Profile* profile =
Profile::FromWebUI(web_ui);
Nit: Optional: Inline into next statement

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/safe_browsing_ui.cc:28:
SafeBrowsingUI::~SafeBrowsingUI() {}
Nit: Can this be "= default" in the header, or does the compiler complain?

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/safe_browsing_ui.h (right):

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/safe_browsing_ui.h:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
Nit: No (c); see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
.  Also applies in .cc file.

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/safe_browsing_ui.h:11: #include "ui/base/layout.h"
Nit: It looks like at least url_data_source.h and layout.h are not used by this
header and can be removed.

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/safe_browsing_ui.h:13: // The WebUI for
chrome://safe-browsing
Nit: Trailing period on comments

[tsergeant, 2017-06-19 23:08:46]

lgtm

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2940963002/diff/40001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:453: <include name="IDR_SAFE_BROWSING_HTML"
file="..\..\components\safe_browsing\resources\safe_browsing.html"
type="BINDATA" />
Nit: Sort these alphabetically, so that the order is CSS, HTML, JS

[hkamila, 2017-06-23 18:29:36]

hkamila@google.com changed reviewers:
- pkasting@chromium.org, tsergeant@chromium.org

[hkamila, 2017-06-23 18:29:36]

hkamila@google.com changed required reviewers:
+ jialiul@chromium.org, vakh@chromium.org

[hkamila, 2017-06-23 18:29:37]

I changed the location of several files, and created new Build, .grdp etc.
files, in order to base the safe browsing page at "components/" instead of
"chrome/". Please Review the changes in PATCH 4.

[vakh (use Gerrit instead), 2017-06-23 18:52:03]

lgtm

Good work.

https://codereview.chromium.org/2940963002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/safe_browsing.js (right):

https://codereview.chromium.org/2940963002/diff/60001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/safe_browsing.js:1: /* Copyright 2017 The
Chromium Authors. All rights reserved.
This file should no longer be needed.

https://codereview.chromium.org/2940963002/diff/60001/components/safe_browsin...
File components/safe_browsing/web_ui/constants.cc (right):

https://codereview.chromium.org/2940963002/diff/60001/components/safe_browsin...
components/safe_browsing/web_ui/constants.cc:12: "The safe browsing page is
under construction.";
We probably don't need this anymore.

https://codereview.chromium.org/2940963002/diff/60001/components/safe_browsin...
File components/safe_browsing/web_ui/safe_browsing_ui.cc (right):

https://codereview.chromium.org/2940963002/diff/60001/components/safe_browsin...
components/safe_browsing/web_ui/safe_browsing_ui.cc:26:
html_source->SetDefaultResource(IDR_SAFE_BROWSING_HTML);
This line explains why the strings.js file was loading as HTML> It could not
find strings.js and tried to load the "default" resource instead.

https://codereview.chromium.org/2940963002/diff/60001/components/safe_browsin...
components/safe_browsing/web_ui/safe_browsing_ui.cc:30:
content::WebUIDataSource::Add(browser_context, html_source);
content::WebUIDataSource::Add(web_ui->GetWebContents()->GetBrowserContext(),
html_source);

[hkamila, 2017-06-23 21:02:20]

hkamila@google.com changed reviewers:
+ blundell@chromium.org, thestig@chromium.org

[hkamila, 2017-06-23 21:02:21]

thestig@chromium.org: Please review changes in 
chrome/common/DEPS


blundell@chromium.org: Please review changes in 

components/safe_browsing_strings.grdp components/components_strings.grd	
components/resources/components_resources.grd
components/resources/safe_browsing_resources.grdp

[hkamila, 2017-06-23 21:07:40]

The CQ bit was checked by hkamila@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 21:08:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 22:27:04]

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

[commit-bot: I haz the power, 2017-06-23 22:27:05]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-06-23 23:16:44]

Description was changed from

==========
Added the chrome://safe-browsing/ page as part of the chrome://about


Created the chrome://safe-browsing/ WebUI interface, and added it as part of the
list, in the chrome://about page.

Reference: I followed the steps in https://www.chromium.org/developers/webui to
create the interface.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Added the chrome://safe-browsing/ page as part of the chrome://about

Created the chrome://safe-browsing/ WebUI interface, and added it as
part of the list, in the chrome://about page.

Reference: I followed the steps in
https://www.chromium.org/developers/webui to create the interface.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Lei Zhang, 2017-06-23 23:21:02]

I reformatted the CL description with a couple small tweaks.

https://codereview.chromium.org/2940963002/diff/80001/chrome/common/DEPS
File chrome/common/DEPS (right):

https://codereview.chromium.org/2940963002/diff/80001/chrome/common/DEPS#newc...
chrome/common/DEPS:28: "+components/safe_browsing/web_ui",
In BUILD.gn, the dependency is set up as
//components/safe_browsing/web_ui:constants, which means really just
constants.h. So this should be just the one header, instead of the entire
directory.

[blundell, 2017-06-26 13:04:57]

lgtm

[Jialiu Lin, 2017-06-26 21:11:12]

LGTM

Great work!

[hkamila, 2017-06-28 01:24:22]

hkamila@google.com changed reviewers:
- blundell@chromium.org, jialiul@chromium.org, vakh@chromium.org

[hkamila, 2017-06-28 01:24:22]

hkamila@google.com changed required reviewers:
- jialiul@chromium.org, vakh@chromium.org

[hkamila, 2017-06-28 01:24:24]

https://codereview.chromium.org/2940963002/diff/80001/chrome/common/DEPS
File chrome/common/DEPS (right):

https://codereview.chromium.org/2940963002/diff/80001/chrome/common/DEPS#newc...
chrome/common/DEPS:28: "+components/safe_browsing/web_ui",
On 2017/06/23 23:21:02, Lei Zhang wrote:
> In BUILD.gn, the dependency is set up as
> //components/safe_browsing/web_ui:constants, which means really just
> constants.h. So this should be just the one header, instead of the entire
> directory.

@thestig
Thank you. I changed it for constants.h in Patch 6. Please review the file.

https://codereview.chromium.org/2940963002/diff/80001/chrome/common/DEPS
File chrome/common/DEPS (right):

https://codereview.chromium.org/2940963002/diff/80001/chrome/common/DEPS#newc...
chrome/common/DEPS:28: "+components/safe_browsing/web_ui",
On 2017/06/23 23:21:02, Lei Zhang wrote:
> In BUILD.gn, the dependency is set up as
> //components/safe_browsing/web_ui:constants, which means really just
> constants.h. So this should be just the one header, instead of the entire
> directory.

Thank you. I changed it for constants.h in Patch 6.

[Lei Zhang, 2017-06-28 18:10:53]

lgtm

[hkamila, 2017-06-28 18:20:04]

The CQ bit was checked by hkamila@google.com

[hkamila, 2017-06-28 18:20:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, pkasting@chromium.org, vakh@chromium.org,
jialiul@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2940963002/#ps100001
(title: "Added the header file in DEPS")

[commit-bot: I haz the power, 2017-06-28 18:20:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 19:47:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498674004686290,
"parent_rev": "7c24da4cdb9052839e6baf3291bd55436981b7b4", "commit_rev":
"8c024eea933703d60dc129ad2feecd5765a2417c"}

[commit-bot: I haz the power, 2017-06-28 19:47:18]

Description was changed from

==========
Added the chrome://safe-browsing/ page as part of the chrome://about

Created the chrome://safe-browsing/ WebUI interface, and added it as
part of the list, in the chrome://about page.

Reference: I followed the steps in
https://www.chromium.org/developers/webui to create the interface.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Added the chrome://safe-browsing/ page as part of the chrome://about

Created the chrome://safe-browsing/ WebUI interface, and added it as
part of the list, in the chrome://about page.

Reference: I followed the steps in
https://www.chromium.org/developers/webui to create the interface.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2940963002
Cr-Commit-Position: refs/heads/master@{#483104}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c024eea933703d60dc129ad2fee...
==========

[commit-bot: I haz the power, 2017-06-28 19:47:19]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8c024eea933703d60dc129ad2fee...