Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl

Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl

This patch reduces the exposed public API and introduces a delegate
instead of a hardwired dependency such that FaviconHandler is more
testable and has clear semantics.

BUG=694312
Review-Url: https://codereview.chromium.org/2691933004
Cr-Commit-Position: refs/heads/master@{#453882}
Committed: https://chromium.googlesource.com/chromium/src/+/16aeb9f7c312208faf699a2aaa1c9792546b59e8

[pkotwicz, 2017-02-13 21:38:55]

Description was changed from

==========
WIP: Refactor FaviconHandler.

- Avoid coupling and cyclic dependency to FaviconDriverImpl.
- Reduce the exposed public API.
- Improve test coverage via TestWebContents.

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

to

==========
WIP: Refactor FaviconHandler.

- Avoid coupling and cyclic dependency to FaviconDriverImpl.
- Reduce the exposed public API.
- Improve test coverage via TestWebContents.

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

[pkotwicz, 2017-02-13 21:38:55]

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

[pkotwicz, 2017-02-13 23:35:48]

I don't have any objections to your CL. It is unclear that your CL will make the
code easier to understand in the long term.

In terms of making the code easier to understand, it is great when CLs can
remove or simplify if() statements. I did quite a bit of that for the
FaviconHandler class a year ago so I am not sure how much of this can still be
done.

In terms of making the code more testable, it is great when less lines are
required to write a new test. It is unclear whether your CL will enable this in
the future. It might, but I am not convinced

https://codereview.chromium.org/2691933004/diff/1/components/favicon/core/fav...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:556: // TODO(mastiz): Fix.
It is possible to bookmark a page while in incognito. This code is there so that
the new bookmark has an icon on the bookmarks bar and the bookmarks page.

[mastiz, 2017-02-14 11:21:40]

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

[commit-bot: I haz the power, 2017-02-14 11:21:54]

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-02-14 11:24:19]

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

[commit-bot: I haz the power, 2017-02-14 11:24:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-02-14 14:48:21]

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

[commit-bot: I haz the power, 2017-02-14 14:48:45]

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-02-14 14:51:31]

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

[commit-bot: I haz the power, 2017-02-14 14:51:34]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-02-15 14:58:43]

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

[commit-bot: I haz the power, 2017-02-15 14:58:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2017-02-15 15:10:28]

Description was changed from

==========
WIP: Refactor FaviconHandler.

- Avoid coupling and cyclic dependency to FaviconDriverImpl.
- Reduce the exposed public API.
- Improve test coverage via TestWebContents.

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

to

==========
WIP: Refactor FaviconHandler.

- Avoid coupling and cyclic dependency to FaviconDriverImpl.
- Reduce the exposed public API.

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

[commit-bot: I haz the power, 2017-02-15 15:10:54]

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

[commit-bot: I haz the power, 2017-02-15 15:10:55]

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

[mastiz, 2017-02-17 09:50:06]

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

[commit-bot: I haz the power, 2017-02-17 09:50:23]

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-02-17 10:03:19]

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

[commit-bot: I haz the power, 2017-02-17 10:03:20]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[mastiz, 2017-02-21 13:55:22]

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

[commit-bot: I haz the power, 2017-02-21 13:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2017-02-21 13:56:38]

Description was changed from

==========
WIP: Refactor FaviconHandler.

- Avoid coupling and cyclic dependency to FaviconDriverImpl.
- Reduce the exposed public API.

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

to

==========
Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl

This patch reduces the exposed public API and introduces a delegate
instead of a hardwired dependency such that FaviconHandler is more
testable and has clear semantics.

BUG=694312
==========

[commit-bot: I haz the power, 2017-02-21 13:57:32]

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

[commit-bot: I haz the power, 2017-02-21 13:57:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-02-21 14:56:31]

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

[commit-bot: I haz the power, 2017-02-21 14:57:11]

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-02-21 15:49:07]

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

[commit-bot: I haz the power, 2017-02-21 15:49:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2017-02-22 02:37:54]

Just nits!

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
File chrome/browser/sessions/session_restore_delegate.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
chrome/browser/sessions/session_restore_delegate.cc:80: // is unlikely to do any
good.
This seems like this should be a crbug not a comment in the code

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
File chrome/browser/sessions/tab_loader.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
chrome/browser/sessions/tab_loader.cc:118: // is unlikely to do any good.
This seems like this should be a crbug not a comment in the code

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/con...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/con...
components/favicon/content/content_favicon_driver.cc:93: ImageDownloadCallback
callback) {
Nit: Reorder this function to match order in .h file

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
To keep the code the same as before, shouldn't we follow the same handling as
when if(download_id == 0) {}

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.h (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:101: ImageDownloadCallback callback) =
0;
Might as well use WebContents::ImageDownloadCallback

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:111: // see its documentation for
details.
Nit: "Replace the ',' with '.'

"Notifies that the favicon image has been updated. Most delegates propagate the
notification to FaviconDriverObserver::OnFaviconUpdated(). See its documentation
for details."

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:208:
FaviconDriverObserver::NotificationIconType handler_type);
Nit: Reorder the functions in the .cc file to match order in .h file

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:238: void DidDownloadFavicon(int id,
Nit: Keep the function name as OnDidDownloadFavicon()

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler_unittest.cc:196: // emulates the
behavior of FaviconDriver::StartDownload()
Nit: Please update comment. FaviconDriver::StartDownload() no longer exists :)

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler_unittest.cc:259: :
FaviconHandler(service, delegate, handler_type) {}
Why not keep first parameter as nullptr?

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.h (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.h:41: ImageDownloadCallback callback)
override;
Nit: Can you please reorder the functions in the .cc file to match the .h file

[mastiz, 2017-02-23 21:55:54]

Thanks, PTAL!

https://codereview.chromium.org/2691933004/diff/1/components/favicon/core/fav...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:556: // TODO(mastiz): Fix.
On 2017/02/13 23:35:48, pkotwicz wrote:
> It is possible to bookmark a page while in incognito. This code is there so
that
> the new bookmark has an icon on the bookmarks bar and the bookmarks page.

I believe this is a bug that needs to be fixed but is orthogonal to this patch.
We're leaking incognito-mode activity and in fact it's something that users
could exploit by artificially creating a bookmark and clearing favicon cache, to
later know if another person visited that site.

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
File chrome/browser/sessions/session_restore_delegate.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
chrome/browser/sessions/session_restore_delegate.cc:80: // is unlikely to do any
good.
On 2017/02/22 02:37:53, pkotwicz wrote:
> This seems like this should be a crbug not a comment in the code

Reverted, this was simply to raise awareness or get your thoughts on whether my
concern was legit.

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
File chrome/browser/sessions/tab_loader.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/chrome/browser/session...
chrome/browser/sessions/tab_loader.cc:118: // is unlikely to do any good.
On 2017/02/22 02:37:53, pkotwicz wrote:
> This seems like this should be a crbug not a comment in the code

Ditto.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/con...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/con...
components/favicon/content/content_favicon_driver.cc:93: ImageDownloadCallback
callback) {
On 2017/02/22 02:37:53, pkotwicz wrote:
> Nit: Reorder this function to match order in .h file

Done. This however makes the diff and blame layer harder to digest.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
On 2017/02/22 02:37:54, pkotwicz wrote:
> To keep the code the same as before, shouldn't we follow the same handling as
> when if(download_id == 0) {}

From what I read the code in OnDidDownloadFavicon, the lookup for download_id ==
0 will always fail and is hence a no-op, except for the 404 case which cannot be
possible here.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.h (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:101: ImageDownloadCallback callback) =
0;
On 2017/02/22 02:37:54, pkotwicz wrote:
> Might as well use WebContents::ImageDownloadCallback

I can't add a dependency to WebContents in shared code, can I?

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:111: // see its documentation for
details.
On 2017/02/22 02:37:54, pkotwicz wrote:
> Nit: "Replace the ',' with '.'
> 
> "Notifies that the favicon image has been updated. Most delegates propagate
the
> notification to FaviconDriverObserver::OnFaviconUpdated(). See its
documentation
> for details."

Done.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:208:
FaviconDriverObserver::NotificationIconType handler_type);
On 2017/02/22 02:37:54, pkotwicz wrote:
> Nit: Reorder the functions in the .cc file to match order in .h file

I started to do this when I realized that many of the already existing functions
are not sorted. I would rather not do it here, I prefer keeping the diff
smaller, unless you insist.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:238: void DidDownloadFavicon(int id,
On 2017/02/22 02:37:54, pkotwicz wrote:
> Nit: Keep the function name as OnDidDownloadFavicon()

Done.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler_unittest.cc:196: // emulates the
behavior of FaviconDriver::StartDownload()
On 2017/02/22 02:37:54, pkotwicz wrote:
> Nit: Please update comment. FaviconDriver::StartDownload() no longer exists :)

Done.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler_unittest.cc:259: :
FaviconHandler(service, delegate, handler_type) {}
On 2017/02/22 02:37:54, pkotwicz wrote:
> Why not keep first parameter as nullptr?

Reverted, this was a leftover after attempting to require that FaviconService is
not null.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/ios...
File components/favicon/ios/web_favicon_driver.h (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/ios...
components/favicon/ios/web_favicon_driver.h:41: ImageDownloadCallback callback)
override;
On 2017/02/22 02:37:54, pkotwicz wrote:
> Nit: Can you please reorder the functions in the .cc file to match the .h file

Done.

[mastiz, 2017-02-23 21:56:02]

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

[commit-bot: I haz the power, 2017-02-23 21:56:37]

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-02-23 23:12:20]

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

[commit-bot: I haz the power, 2017-02-23 23:12:21]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-02-24 16:04:47]

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
There might be multiple icon URLs. We want to continue and download the second
icon URL even if the first icon URL is not valid.

You could imagine writing a test similar to
FaviconHandlerTest.MultipleFavicons404

I am unsure if it is possible for the icon URL to be invalid (if it can't
perhaps we should remove the check altogether)

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.h (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:101: ImageDownloadCallback callback) =
0;
You're right. I forgot about iOS

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.h:208:
FaviconDriverObserver::NotificationIconType handler_type);
Sorting the functions in a follow up CL is fine by me

https://codereview.chromium.org/2691933004/diff/230001/components/favicon/cor...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2691933004/diff/230001/components/favicon/cor...
components/favicon/core/favicon_handler_unittest.cc:237: int download_id_;
Nit: Can you please add a new line?

[mastiz, 2017-02-24 20:29:30]

PTAL.

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
On 2017/02/24 16:04:46, pkotwicz wrote:
> There might be multiple icon URLs. We want to continue and download the second
> icon URL even if the first icon URL is not valid.
> 
> You could imagine writing a test similar to
> FaviconHandlerTest.MultipleFavicons404
> 
> I am unsure if it is possible for the icon URL to be invalid (if it can't
> perhaps we should remove the check altogether)

NOTREACHED() definitely means this is impossible, only a bug in the code could
cause it to happen. This is obvious after this patch because ScheduleDownloads
does the check.

I think it's a bad idea to have this block here but I was trying to keep the
logic unchanged. Since you don't seem to like it much, I replaced it with a
DCHECK.

https://codereview.chromium.org/2691933004/diff/230001/components/favicon/cor...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2691933004/diff/230001/components/favicon/cor...
components/favicon/core/favicon_handler_unittest.cc:237: int download_id_;
On 2017/02/24 16:04:46, pkotwicz wrote:
> Nit: Can you please add a new line?

Done.

[pkotwicz, 2017-02-27 16:53:09]

LGTM

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
The DCHECK is ok. We can probably just remove the DCHECK(). We filter out
invalid URLs in FaviconHandler::OnUpdateFaviconURL()

[mastiz, 2017-02-27 21:08:59]

Thanks for the review!

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
On 2017/02/27 16:53:09, pkotwicz wrote:
> The DCHECK is ok. We can probably just remove the DCHECK(). We filter out
> invalid URLs in FaviconHandler::OnUpdateFaviconURL()

If the DCHECK is ok, I'll leave it there (I hope I haven't misinterpreted the
suggestion above).

[pkotwicz, 2017-02-27 23:43:34]

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2691933004/diff/140001/components/favicon/cor...
components/favicon/core/favicon_handler.cc:663: if (!image_url.is_valid()) {
Leaving it is ok.

[mastiz, 2017-03-01 07:47:22]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-03-01 07:47:42]

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-03-01 08:33:26]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1488354442466800,
"parent_rev": "cb2e1fe56fc33bde8b128817988e7b374ea7d1c3", "commit_rev":
"16aeb9f7c312208faf699a2aaa1c9792546b59e8"}

[commit-bot: I haz the power, 2017-03-01 08:34:09]

Description was changed from

==========
Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl

This patch reduces the exposed public API and introduces a delegate
instead of a hardwired dependency such that FaviconHandler is more
testable and has clear semantics.

BUG=694312
==========

to

==========
Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl

This patch reduces the exposed public API and introduces a delegate
instead of a hardwired dependency such that FaviconHandler is more
testable and has clear semantics.

BUG=694312

Review-Url: https://codereview.chromium.org/2691933004
Cr-Commit-Position: refs/heads/master@{#453882}
Committed:
https://chromium.googlesource.com/chromium/src/+/16aeb9f7c312208faf699a2aaa1c...
==========

[commit-bot: I haz the power, 2017-03-01 08:34:12]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/16aeb9f7c312208faf699a2aaa1c...