TopSites: filter out non-WebSafe URLs

TopSites: filter out non-WebSafe URLs

BUG=605909
Committed: https://crrev.com/f1305c06e94b347c0245d8e14f055f4c0df5c015
Cr-Commit-Position: refs/heads/master@{#391234}

[Marc Treib, 2016-04-27 13:45:50]

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

[commit-bot: I haz the power, 2016-04-27 13:46:06]

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

[commit-bot: I haz the power, 2016-04-27 13:58:07]

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

[commit-bot: I haz the power, 2016-04-27 13:58:07]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Marc Treib, 2016-05-02 12:41:26]

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

[commit-bot: I haz the power, 2016-05-02 12:41:40]

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

[commit-bot: I haz the power, 2016-05-02 13:28:45]

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

[commit-bot: I haz the power, 2016-05-02 13:28:45]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-05-02 14:53:25]

Patchset #4 (id:60001) has been deleted

[Marc Treib, 2016-05-02 15:29:43]

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

[Marc Treib, 2016-05-02 15:29:44]

...and now for the thing I actually wanted to do :)
PTAL!

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
File ios/chrome/browser/history/history_backend_client_impl.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
ios/chrome/browser/history/history_backend_client_impl.cc:53: return true;
On non-iOS, we check this via
content::ChildProcessSecurityPolicy::IsWebSafeScheme, but I couldn't find
anything equivalent on iOS. Do you know of something?

[sdefresne, 2016-05-03 11:18:51]

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
File components/history/core/browser/visitsegment_database.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
components/history/core/browser/visitsegment_database.cc:277: if (results.size()
>= static_cast<size_t>(max_result_count))
I guess it is okay to jump over a call to sql statement reset, right?

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
File components/history/core/browser/visitsegment_database.h (right):

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
components/history/core/browser/visitsegment_database.h:10: #include
"base/callback.h"
nit: I think you can include "base/callback_forward.h" here and
"base/callback.h" in the implementation.

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
File ios/chrome/browser/history/history_backend_client_impl.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
ios/chrome/browser/history/history_backend_client_impl.cc:53: return true;
On 2016/05/02 15:29:44, Marc Treib wrote:
> On non-iOS, we check this via
> content::ChildProcessSecurityPolicy::IsWebSafeScheme, but I couldn't find
> anything equivalent on iOS. Do you know of something?

I think we do not have a dedicated method to do that. We usually just check
whether scheme is http or https (which are the two safe scheme among the few iOS
supports), i.e.:

return url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme);

[Marc Treib, 2016-05-03 11:51:06]

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

[Marc Treib, 2016-05-03 11:51:17]

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
File components/history/core/browser/visitsegment_database.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
components/history/core/browser/visitsegment_database.cc:277: if (results.size()
>= static_cast<size_t>(max_result_count))
On 2016/05/03 11:18:50, sdefresne wrote:
> I guess it is okay to jump over a call to sql statement reset, right?

Yup: We don't use it again afterwards, so it doesn't matter.

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
File components/history/core/browser/visitsegment_database.h (right):

https://codereview.chromium.org/1924773002/diff/80001/components/history/core...
components/history/core/browser/visitsegment_database.h:10: #include
"base/callback.h"
On 2016/05/03 11:18:50, sdefresne wrote:
> nit: I think you can include "base/callback_forward.h" here and
> "base/callback.h" in the implementation.

Done.

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
File ios/chrome/browser/history/history_backend_client_impl.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
ios/chrome/browser/history/history_backend_client_impl.cc:53: return true;
On 2016/05/03 11:18:50, sdefresne wrote:
> On 2016/05/02 15:29:44, Marc Treib wrote:
> > On non-iOS, we check this via
> > content::ChildProcessSecurityPolicy::IsWebSafeScheme, but I couldn't find
> > anything equivalent on iOS. Do you know of something?
> 
> I think we do not have a dedicated method to do that. We usually just check
> whether scheme is http or https (which are the two safe scheme among the few
iOS
> supports), i.e.:
> 
> return url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme);

GURL even has a SchemeIsHTTPOrHTTPS() method :)
Are these really the only supported schemes though? I'd have expected at least
FTP too?

[commit-bot: I haz the power, 2016-05-03 11:51:25]

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

[sdefresne, 2016-05-03 12:03:40]

lgtm

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
File ios/chrome/browser/history/history_backend_client_impl.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
ios/chrome/browser/history/history_backend_client_impl.cc:53: return true;
On 2016/05/03 11:51:17, Marc Treib wrote:
> On 2016/05/03 11:18:50, sdefresne wrote:
> > On 2016/05/02 15:29:44, Marc Treib wrote:
> > > On non-iOS, we check this via
> > > content::ChildProcessSecurityPolicy::IsWebSafeScheme, but I couldn't find
> > > anything equivalent on iOS. Do you know of something?
> > 
> > I think we do not have a dedicated method to do that. We usually just check
> > whether scheme is http or https (which are the two safe scheme among the few
> iOS
> > supports), i.e.:
> > 
> > return url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme);
> 
> GURL even has a SchemeIsHTTPOrHTTPS() method :)
> Are these really the only supported schemes though? I'd have expected at least
> FTP too?

No, Chrome on iOS does not support ftp:, nor file: schemes. It supports chrome:,
http:, https:, data: and javascript: IIRC.

https://codereview.chromium.org/1924773002/diff/100001/components/history/cor...
File components/history/core/browser/history_backend_client.h (right):

https://codereview.chromium.org/1924773002/diff/100001/components/history/cor...
components/history/core/browser/history_backend_client.h:47: virtual bool
IsWebSafe(const GURL& url) = 0;
Please document this method.

[Marc Treib, 2016-05-03 12:15:10]

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
File ios/chrome/browser/history/history_backend_client_impl.cc (right):

https://codereview.chromium.org/1924773002/diff/80001/ios/chrome/browser/hist...
ios/chrome/browser/history/history_backend_client_impl.cc:53: return true;
On 2016/05/03 12:03:39, sdefresne wrote:
> On 2016/05/03 11:51:17, Marc Treib wrote:
> > On 2016/05/03 11:18:50, sdefresne wrote:
> > > On 2016/05/02 15:29:44, Marc Treib wrote:
> > > > On non-iOS, we check this via
> > > > content::ChildProcessSecurityPolicy::IsWebSafeScheme, but I couldn't
find
> > > > anything equivalent on iOS. Do you know of something?
> > > 
> > > I think we do not have a dedicated method to do that. We usually just
check
> > > whether scheme is http or https (which are the two safe scheme among the
few
> > iOS
> > > supports), i.e.:
> > > 
> > > return url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme);
> > 
> > GURL even has a SchemeIsHTTPOrHTTPS() method :)
> > Are these really the only supported schemes though? I'd have expected at
least
> > FTP too?
> 
> No, Chrome on iOS does not support ftp:, nor file: schemes. It supports
chrome:,
> http:, https:, data: and javascript: IIRC.

Alright, thanks for the explanation!
I suppose "data:" could also be considered web-safe, but I don't see that ever
coming up (and even if it did, it doesn't seem useful for TopSites).

https://codereview.chromium.org/1924773002/diff/100001/components/history/cor...
File components/history/core/browser/history_backend_client.h (right):

https://codereview.chromium.org/1924773002/diff/100001/components/history/cor...
components/history/core/browser/history_backend_client.h:47: virtual bool
IsWebSafe(const GURL& url) = 0;
On 2016/05/03 12:03:40, sdefresne wrote:
> Please document this method.

Done.

[Marc Treib, 2016-05-03 13:00:53]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-05-03 13:00:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1924773002/#ps120001
(title: "comment")

[commit-bot: I haz the power, 2016-05-03 13:00:59]

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

[commit-bot: I haz the power, 2016-05-03 14:53:51]

Committed patchset #6 (id:120001)

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

Description was changed from

==========
TopSites: filter out non-WebSafe URLs

BUG=605909
==========

to

==========
TopSites: filter out non-WebSafe URLs

BUG=605909

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

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

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