Don't derive from ChromeURLDataManager::DataSource, and instead have these classes implement a dele…

Don't derive from ChromeURLDataManager::DataSource, and instead have these classes implement a delegate interface instead. Also make ChromeURLDataManager::DataSource not a nested class so that it can be forward declared.

This is in preparation for hiding the URLDataSource implementation.

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

[jam, 2013-01-14 17:40:28]

https://codereview.chromium.org/11885021/diff/1/content/public/browser/url_da...
File content/public/browser/url_data_source_delegate.h (right):

https://codereview.chromium.org/11885021/diff/1/content/public/browser/url_da...
content/public/browser/url_data_source_delegate.h:14: class
ChromeWebUIDataSource;
most of these forward declarations and friend'ing will disappear in future
changes, but are needed temporarily

[James Hawkins, 2013-01-14 18:37:33]

Is it possible to pull this part [1] into a separate CL?

[1] "Also make ChromeURLDataManager::DataSource not a nested class so that it
can be forward declared."

[jam, 2013-01-14 18:39:47]

On 2013/01/14 18:37:33, James Hawkins wrote:
> Is it possible to pull this part [1] into a separate CL?
> 
> [1] "Also make ChromeURLDataManager::DataSource not a nested class so that it
> can be forward declared."

perhaps in hindsight I should have done that, but now that I have this change,
I'd prefer not to do the extra effort. The number of changed files would be
roughly the same in each change anyways, since all the same callsites would need
to be updated.

if it makes it less work for you, you can review the core change (i.e. to
chrome_url_data_manager*) and I can get other reviewers for the rest

[James Hawkins, 2013-01-14 18:41:21]

https://codereview.chromium.org/11885021/diff/69/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_url_data_manager.h (right):

https://codereview.chromium.org/11885021/diff/69/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_url_data_manager.h:119: class URLDataSource :
public base::RefCountedThreadSafe<
Please pull this out into its own file.

[jam, 2013-01-14 18:45:28]

On Mon, Jan 14, 2013 at 10:41 AM, <jhawkins@chromium.org> wrote:

>
> https://codereview.chromium.**org/11885021/diff/69/chrome/**
>
browser/ui/webui/chrome_url_**data_manager.h<https://codereview.chromium.org/11885021/diff/69/chrome/browser/ui/webui/chrome_url_data_manager.h>
> File chrome/browser/ui/webui/**chrome_url_data_manager.h (right):
>
> https://codereview.chromium.**org/11885021/diff/69/chrome/**
>
browser/ui/webui/chrome_url_**data_manager.h#newcode119<https://codereview.chromium.org/11885021/diff/69/chrome/browser/ui/webui/chrome_url_data_manager.h#newcode119>
> chrome/browser/ui/webui/**chrome_url_data_manager.h:119: class
> URLDataSource : public base::RefCountedThreadSafe<
> Please pull this out into its own file.
>

(perhaps my plan on the bug should have been more detailed)

I am planning on doing so, but I didn't want to do it in this change. The
reason is that it seemed like a waste of effort to move this to file foo.h
now, only to move it to bar.h while splitting it into an interface and
implementation in a future change.

>
>
https://codereview.chromium.**org/11885021/<https://codereview.chromium.org/1...
>

[James Hawkins, 2013-01-14 18:53:54]

You get the point about period at the end in webui/.  I stopped marking them
about halfway or so.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.h (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_url_data_manager_backend.h:66: static void
CallStartRequest(scoped_refptr<URLDataSource>,
nit: Document method and parameters.

Rationale: my internal monologue:

"What does it do beyond calling the StartRequestMethod?"

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:85: //
content::URLDataSourceDelegate implementation:
webui/ files use:

// content::URLDataSourceDelegate implementation.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:93: std::string
source_name_;
nit: Document member variable.

Yes, none of the others are documented, and that is bad.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:35: //
content::URLDataSourceDelegate implementation:
nit: Period at the end.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/register_page_ui.cc (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/register_page_ui.cc:103: //
content::URLDataSourceDelegate implementation:
nit: Period at the end.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/sim_unlock_ui.cc (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/sim_unlock_ui.cc:78: //
content::URLDataSourceDelegate implementation:
nit: Period at the end.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/system_info_ui.cc:50: //
content::URLDataSourceDelegate implementation:
nit: Period at the end.

[jam, 2013-01-14 19:21:18]

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.h (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_url_data_manager_backend.h:66: static void
CallStartRequest(scoped_refptr<URLDataSource>,
On 2013/01/14 18:53:54, James Hawkins wrote:
> nit: Document method and parameters.
> 
> Rationale: my internal monologue:
> 
> "What does it do beyond calling the StartRequestMethod?"

Done.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:85: //
content::URLDataSourceDelegate implementation:
On 2013/01/14 18:53:54, James Hawkins wrote:
> webui/ files use:
> 
> // content::URLDataSourceDelegate implementation.

Done.

https://codereview.chromium.org/11885021/diff/74/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:93: std::string
source_name_;
On 2013/01/14 18:53:54, James Hawkins wrote:
> nit: Document member variable.
> 
> Yes, none of the others are documented, and that is bad.

Done.

[James Hawkins, 2013-01-14 19:24:33]

Linter issues.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:118: AboutMemoryHandler(const
base::WeakPtr<AboutUIHTMLSource>& source,
Need to -1 space on these lines.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_url_data_manager.cc (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_url_data_manager.cc:42:
ChromeURLDataManager::URLDataSources* ChromeURLDataManager::data_sources_ =
NULL;
80 cols.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:36: explicit
ChromeWebUIDataSource(const std::string& source_name);;
Remove double semi-colon.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/thumbnail_source.cc (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:20: : // Set ThumbnailService
now as Profile isn't thread safe.
nit: One extra space before start of comment.

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
File content/public/browser/url_data_source_delegate.cc (right):

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
content/public/browser/url_data_source_delegate.cc:31: }  // namespace content
Add missing new line.

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
File content/public/browser/url_data_source_delegate.h (right):

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
content/public/browser/url_data_source_delegate.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Fix linter issues in this file.

[jam, 2013-01-14 20:25:14]

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:118: AboutMemoryHandler(const
base::WeakPtr<AboutUIHTMLSource>& source,
On 2013/01/14 19:24:33, James Hawkins wrote:
> Need to -1 space on these lines.

Done.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_url_data_manager.cc (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_url_data_manager.cc:42:
ChromeURLDataManager::URLDataSources* ChromeURLDataManager::data_sources_ =
NULL;
On 2013/01/14 19:24:33, James Hawkins wrote:
> 80 cols.

Done.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:36: explicit
ChromeWebUIDataSource(const std::string& source_name);;
On 2013/01/14 19:24:33, James Hawkins wrote:
> Remove double semi-colon.

Done.

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/thumbnail_source.cc (right):

https://codereview.chromium.org/11885021/diff/10020/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:20: : // Set ThumbnailService
now as Profile isn't thread safe.
On 2013/01/14 19:24:33, James Hawkins wrote:
> nit: One extra space before start of comment.

Done.

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
File content/public/browser/url_data_source_delegate.cc (right):

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
content/public/browser/url_data_source_delegate.cc:31: }  // namespace content
On 2013/01/14 19:24:33, James Hawkins wrote:
> Add missing new line.

it's there but rietveld doesn't show it (might be a newline thing with windows)

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
File content/public/browser/url_data_source_delegate.h (right):

https://codereview.chromium.org/11885021/diff/10020/content/public/browser/ur...
content/public/browser/url_data_source_delegate.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/01/14 19:24:33, James Hawkins wrote:
> Fix linter issues in this file.

Done.

[James Hawkins, 2013-01-14 20:30:25]

LGTM with question.  In general I'm not a fan of adding NULL checks, if
possible.

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:207: if (!source_)
Under what circumstances would this be NULL (at a high level)?

[jam, 2013-01-14 20:33:57]

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:207: if (!source_)
On 2013/01/14 20:30:25, James Hawkins wrote:
> Under what circumstances would this be NULL (at a high level)?

Previously, DataSource derived classes got refcounting from the parent class.
Now that these classes derive from a non-refcounted base class
(URLSourceDelegate), hopping to another thread (FILE in this case) means that
the source can be gone if the user navigated away. So basically the weak pointer
factory accomplishes roughly the same thing (not crashing) as refcounting did
before.

[tfarina, 2013-01-14 20:49:11]

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/b...
File chrome/browser/ui/webui/bookmarks_ui.cc (right):

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/b...
chrome/browser/ui/webui/bookmarks_ui.cc:60: }
wait, something is crazy going on here.


Isn't:

ChromeURLDataManager::AddDataSource(Profile::FromWebUI(web_ui), new
BookmarksUIHTMLSource);

The same of:

BookmarksUIHTMLSource* html_source = new BookmarksUIHTMLSource();
Profile* profile = Profile::FromWebUI(web_ui);
ChromeURLDataManager::AddDataSource(profile, html_source);

?

If I'm not seeing this wrong, you are doing the same thing twice here.

[jam, 2013-01-14 20:52:06]

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/b...
File chrome/browser/ui/webui/bookmarks_ui.cc (right):

https://codereview.chromium.org/11885021/diff/14032/chrome/browser/ui/webui/b...
chrome/browser/ui/webui/bookmarks_ui.cc:60: }
On 2013/01/14 20:49:11, tfarina wrote:
> wait, something is crazy going on here.
> 
> 
> Isn't:
> 
> ChromeURLDataManager::AddDataSource(Profile::FromWebUI(web_ui), new
> BookmarksUIHTMLSource);
> 
> The same of:
> 
> BookmarksUIHTMLSource* html_source = new BookmarksUIHTMLSource();
> Profile* profile = Profile::FromWebUI(web_ui);
> ChromeURLDataManager::AddDataSource(profile, html_source);
> 
> ?
> 
> If I'm not seeing this wrong, you are doing the same thing twice here.

ah, thanks. i had changed it while doing another change that i later undid, and
now i went back to make the formatting the same but forgot to remove this. will
commit in a followup