clean up partition check in url_data_manager_backend.cc

clean up partition check in url_data_manager_backend.cc

BUG=

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

[guohui, 2014-03-05 19:30:00]

Hey John,

could you please a look at the CL? It claens up the storage partition check in
url_data_manager_backend.cc, which i promised a while ago...

Thanks,

Hui

[jam, 2014-03-05 22:29:46]

lgtm with comments

https://codereview.chromium.org/183803023/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/183803023/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1077:
content::ContentBrowserClient::GetWebUIHostsToIgnoreParititionCheck(hosts);
nit: then remove this line

https://codereview.chromium.org/183803023/diff/20001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/183803023/diff/20001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:360: std::vector<std::string>
hosts;
i.e. do hosts->push_back(kChromeUIResourcesHost) here and don't change
ContentBrowserClient's impl file

https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.cc:42:
hosts->push_back(kChromeUIResourcesHost);
this file should just be empty methods. move this to the calling code which
should just add kChromeUIResourcesHost to the vector before calling this method

after that, no need to change this cc file

[guohui, 2014-03-05 23:04:24]

https://codereview.chromium.org/183803023/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/183803023/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1077:
content::ContentBrowserClient::GetWebUIHostsToIgnoreParititionCheck(hosts);
On 2014/03/05 22:29:46, jam wrote:
> nit: then remove this line

Done.

https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.cc:42:
hosts->push_back(kChromeUIResourcesHost);
On 2014/03/05 22:29:46, jam wrote:
> this file should just be empty methods. move this to the calling code which
> should just add kChromeUIResourcesHost to the vector before calling this
method
> 
> after that, no need to change this cc file

then is it better to move this line to the subclass ContentBrowserClient instead
of the calling code? Otherwise every caller has to know the logic and dup the
code.

[guohui, 2014-03-05 23:05:00]

On 2014/03/05 23:04:24, guohui wrote:
>
https://codereview.chromium.org/183803023/diff/20001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/183803023/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:1077:
> content::ContentBrowserClient::GetWebUIHostsToIgnoreParititionCheck(hosts);
> On 2014/03/05 22:29:46, jam wrote:
> > nit: then remove this line
> 
> Done.
Sorry not done yet ...

> 
>
https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
> File content/public/browser/content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
> content/public/browser/content_browser_client.cc:42:
> hosts->push_back(kChromeUIResourcesHost);
> On 2014/03/05 22:29:46, jam wrote:
> > this file should just be empty methods. move this to the calling code which
> > should just add kChromeUIResourcesHost to the vector before calling this
> method
> > 
> > after that, no need to change this cc file
> 
> then is it better to move this line to the subclass ContentBrowserClient
instead
> of the calling code? Otherwise every caller has to know the logic and dup the
> code.

[jam, 2014-03-06 01:54:47]

https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/183803023/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.cc:42:
hosts->push_back(kChromeUIResourcesHost);
On 2014/03/05 23:04:24, guohui wrote:
> On 2014/03/05 22:29:46, jam wrote:
> > this file should just be empty methods. move this to the calling code which
> > should just add kChromeUIResourcesHost to the vector before calling this
> method
> > 
> > after that, no need to change this cc file
> 
> then is it better to move this line to the subclass ContentBrowserClient
instead
> of the calling code? Otherwise every caller has to know the logic and dup the
> code.

if you do my comment in url_data_manager_backend.cc, then you don't ned to have
this line anywhere other than url_data_manager_backend

[guohui, 2014-03-17 21:17:00]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:19:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/183803023/60001

[commit-bot: I haz the power, 2014-03-18 00:33:34]

Change committed as 257556

[Dan Beam, 2016-05-17 02:28:55]

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

[Dan Beam, 2016-05-17 02:28:55]

https://codereview.chromium.org/183803023/diff/60001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/183803023/diff/60001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:365: std::find(hosts.begin(),
hosts.end(), url.host()) != hosts.end())) {
did this code ever work?  it seems like any chrome:// scheme URL would be
allowed and |hosts| was effectively never used.

[Dan Beam, 2016-05-17 03:43:48]

https://codereview.chromium.org/183803023/diff/60001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (left):

https://codereview.chromium.org/183803023/diff/60001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:364: (url.host() ==
kChromeUIResourcesHost ||
it seems as if this host got dropped during this refactor as well :(

[Charlie Reis, 2016-05-17 21:08:24]

https://codereview.chromium.org/183803023/diff/60001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/183803023/diff/60001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:365: std::find(hosts.begin(),
hosts.end(), url.host()) != hosts.end())) {
On 2016/05/17 02:28:55, Dan Beam wrote:
> did this code ever work?  it seems like any chrome:// scheme URL would be
> allowed and |hosts| was effectively never used.

Yeah, this doesn't match the change that jam@ requested (i.e., putting
kChromeUIResourcesHost into hosts), and it broke the outcome.  Apparently there
weren't any tests to catch it either.