Removing HistoryService::profile() method

Removing HistoryService::profile() method

As per review comments on the issue
https://codereview.chromium.org/573553004/#msg3
removing HistoryService::profile() method.

BUG=430070
Committed: https://crrev.com/c358578aa24196c5be8bd22633517c640b13a108
Cr-Commit-Position: refs/heads/master@{#308132}

[Paritosh Kumar, 2014-12-10 15:25:47]

paritosh.in@samsung.com changed reviewers:
+ jochen@chromium.org, sdefresne@chromium.org

[Paritosh Kumar, 2014-12-10 15:25:48]

Please have a look.

[sdefresne, 2014-12-10 15:41:50]

Please do not access private state of another class. Instead see recommendation
in my comment. Once this is done, you can probably remove the "friend class
LastDownloadFinder;" in HistoryService too.

https://codereview.chromium.org/783963003/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc
(right):

https://codereview.chromium.org/783963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:342:
OnProfileHistoryLoaded(history_service->profile_, history_service);
Please do not access the instance variable profile_ either. Instead iterate over
profile_states_ and check if the HistoryService attached to the profile is the
same as the one passed to the event. Since the profile_states_ is checked into
OnProfileHistoryLoaded, I would inline the function there, something like this:

void LastDownloadFinder::OnHistoryServiceLoaded(
    HistoryService* history_service) {
  for (const auto& pair : profile_states_) {
    HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
        pair.first, Profile::EXPLICIT_ACCESS);
    if (hs == history_service) {
      // Start the query in the history service if the finder was waiting for
the
      // service to load.
      if (pair.second == WAITING_FOR_HISTORY) {
        history_service->QueryDownloads(
            base::Bind(&LastDownloadFinder::OnDownloadQuery,
                       weak_ptr_factory_.GetWeakPtr(),
                       pair.first));
    }
    break;
  }
}

[Paritosh Kumar, 2014-12-11 11:49:55]

Thanks sdefresne

Updated as per your suggestion.
Please have a look.

https://codereview.chromium.org/783963003/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc
(right):

https://codereview.chromium.org/783963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:342:
OnProfileHistoryLoaded(history_service->profile_, history_service);
Thanks Updated as per suggestion.

[sdefresne, 2014-12-11 12:43:08]

https://codereview.chromium.org/783963003/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc
(right):

https://codereview.chromium.org/783963003/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:249:
void LastDownloadFinder::OnProfileHistoryLoaded(
This method is no longer called. Remove.

[Paritosh Kumar, 2014-12-11 13:13:56]

Thanks

Updted as suggested.

PTAL.

https://codereview.chromium.org/783963003/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc
(right):

https://codereview.chromium.org/783963003/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:249:
void LastDownloadFinder::OnProfileHistoryLoaded(
On 2014/12/11 12:43:08, sdefresne wrote:
> This method is no longer called. Remove.

Done.

[sdefresne, 2014-12-11 15:14:26]

lgtm

Thank you for the patch.

[Paritosh Kumar, 2014-12-11 16:52:55]

paritosh.in@samsung.com changed reviewers:
+ avi@chromium.org, mattm@chromium.org

[Paritosh Kumar, 2014-12-11 16:52:56]

@mattm, @jochen, @avi :- Please have a look on browser/safe-browsing area.

-Paritosh

[Paritosh Kumar, 2014-12-11 17:33:40]

paritosh.in@samsung.com changed reviewers:
+ sky@chromium.org

[Paritosh Kumar, 2014-12-11 18:33:10]

paritosh.in@samsung.com changed reviewers:
- avi@chromium.org, jochen@chromium.org, mattm@chromium.org,
sdefresne@chromium.org

[Paritosh Kumar, 2014-12-11 18:33:10]

@sky-Please have a look on browser/safe-browsing parts.

-Paritosh

[Paritosh Kumar, 2014-12-11 18:33:30]

paritosh.in@samsung.com changed reviewers:
+ avi@chromium.org, jochen@chromium.org, mattm@chromium.org,
sdefresne@chromium.org

[sky, 2014-12-11 19:23:49]

https://codereview.chromium.org/783963003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc
(right):

https://codereview.chromium.org/783963003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:326:
HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
IMO the old way is cleaner. Why do you need to remove HS::profile() ?

[sdefresne, 2014-12-12 10:00:25]

On 2014/12/11 at 19:23:49, sky wrote:
>
https://codereview.chromium.org/783963003/diff/40001/chrome/browser/safe_brow...
> File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc
(right):
> 
>
https://codereview.chromium.org/783963003/diff/40001/chrome/browser/safe_brow...
> chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:326:
HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
> IMO the old way is cleaner. Why do you need to remove HS::profile() ?

The plan is that HistoryService will eventually move into //components/history
so that it can be shared by iOS. To be shareable with iOS, we have to remove the
dependencies on //content and //chrome and thus must remove the profile_ field
from the HistoryService completely, and thus the getter.

[sky, 2014-12-12 16:27:32]

I was afraid it was something like that. *SIGH* LGTM

[Paritosh Kumar, 2014-12-12 17:35:26]

The CQ bit was checked by paritosh.in@samsung.com

[Paritosh Kumar, 2014-12-12 17:36:02]

The CQ bit was checked by paritosh.in@samsung.com

[commit-bot: I haz the power, 2014-12-12 17:37:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/783963003/40001

[commit-bot: I haz the power, 2014-12-12 18:54:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-12 18:56:04]

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