Move IconURLs method from WebFrame to WebLocalFrame

Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit and addresses https://crbug.com/416660.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer and
  addresses the left-over methods and the comment from r263486
  related to https://crbug.com/361761.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=michaelbai@chromium.org, rockot@chromium.org

Review-Url: https://codereview.chromium.org/2918903002
Cr-Commit-Position: refs/heads/master@{#477803}
Committed: https://chromium.googlesource.com/chromium/src/+/178ff09570a50662f01850fb0cdbea76de51bd98

[Łukasz Anforowicz, 2017-06-01 23:09:06]

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

[commit-bot: I haz the power, 2017-06-01 23:09:41]

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-06-02 01:25:46]

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

[commit-bot: I haz the power, 2017-06-02 01:25:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Łukasz Anforowicz, 2017-06-02 16:01:34]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-06-02 16:01:35]

dcheng@, can you PTAL?

[dcheng, 2017-06-02 16:28:16]

https://codereview.chromium.org/2918903002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2918903002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2611: if
(!webview()->MainFrame()->IsWebLocalFrame())
Maybe this logic should be on RenderFrameImpl. WDYT?

https://codereview.chromium.org/2918903002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2918903002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebLocalFrame.h:229: // The urls of the given
combination types of favicon (if any) specified by
Originally, this was located under the "Basic Properties" subsection of
WebFrame. Do we want to add a section like that here?

[Łukasz Anforowicz, 2017-06-02 18:44:42]

Description was changed from

==========
Move IconURLs method from WebFrame to WebLocalFrame

All callers already either call through WebLocalFrame, or verify
before the call that the frame is local.

No intended behavior change.

BUG=416660
==========

to

==========
Move IconURLs method from WebFrame to WebLocalFrame

All callers already either call through WebLocalFrame, or verify
before the call that the frame is local.

No intended behavior change.

BUG=361761, 416660
==========

[Łukasz Anforowicz, 2017-06-02 20:42:47]

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

[commit-bot: I haz the power, 2017-06-02 20:43:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-02 20:46:15]

Description was changed from

==========
Move IconURLs method from WebFrame to WebLocalFrame

All callers already either call through WebLocalFrame, or verify
before the call that the frame is local.

No intended behavior change.

BUG=361761, 416660
==========

to

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

BUG=361761, 416660
==========

[commit-bot: I haz the power, 2017-06-02 20:46:52]

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

[commit-bot: I haz the power, 2017-06-02 20:46:54]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2017-06-02 21:06:26]

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

[commit-bot: I haz the power, 2017-06-02 21:06:58]

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-06-02 21:20:39]

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

[commit-bot: I haz the power, 2017-06-02 21:20:40]

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

[Łukasz Anforowicz, 2017-06-02 23:39:35]

Description was changed from

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

BUG=361761, 416660
==========

to

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2017-06-02 23:45:00]

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

[commit-bot: I haz the power, 2017-06-02 23:45:26]

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-06-03 00:54:21]

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

[commit-bot: I haz the power, 2017-06-03 00:54:22]

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_...)

[Łukasz Anforowicz, 2017-06-05 20:27:02]

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

[commit-bot: I haz the power, 2017-06-05 20:27:51]

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-06-05 22:32:44]

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

[commit-bot: I haz the power, 2017-06-05 22:32:45]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-05 22:38:46]

Daniel - this CL is (finally) ready for another look please.

https://codereview.chromium.org/2918903002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2918903002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2611: if
(!webview()->MainFrame()->IsWebLocalFrame())
On 2017/06/02 16:28:15, dcheng wrote:
> Maybe this logic should be on RenderFrameImpl. WDYT?

Yes - thanks for pointing this out.  Done.

https://codereview.chromium.org/2918903002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2918903002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebLocalFrame.h:229: // The urls of the given
combination types of favicon (if any) specified by
On 2017/06/02 16:28:16, dcheng wrote:
> Originally, this was located under the "Basic Properties" subsection of
> WebFrame. Do we want to add a section like that here?

Done.

[dcheng, 2017-06-06 20:01:30]

LGTM

https://codereview.chromium.org/2918903002/diff/100001/components/favicon/con...
File components/favicon/content/favicon_url_util.cc (right):

https://codereview.chromium.org/2918903002/diff/100001/components/favicon/con...
components/favicon/content/favicon_url_util.cc:28: default:
Nit: prefer to keep this out of the switch statement, so the compiler can warn
on new (unhandled) values.

https://codereview.chromium.org/2918903002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/2918903002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:321: void
NavigateActiveAndCommit(const GURL& url, bool dont_swap_out = false) {
defaulted bool parameters are a little sad. I guess it's just a test though...

https://codereview.chromium.org/2918903002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2918903002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3809: if (!urls.empty())
Maybe we can short-circuit this at line 3801? I guess I could see an argument
for doing it this way, but structuring it to return earlier makes it clear that
there's no filtering logic later on.

[Łukasz Anforowicz, 2017-06-06 20:52:33]

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

[commit-bot: I haz the power, 2017-06-06 20:53:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-06 20:55:00]

Thanks for the review.  I've responded to one of your comments with a mid-size
change, so you might want to take one last look (I'll go ahead and send this CL
out to avi@ for //content review in the meantime).

https://codereview.chromium.org/2918903002/diff/100001/components/favicon/con...
File components/favicon/content/favicon_url_util.cc (right):

https://codereview.chromium.org/2918903002/diff/100001/components/favicon/con...
components/favicon/content/favicon_url_util.cc:28: default:
On 2017/06/06 20:01:30, dcheng wrote:
> Nit: prefer to keep this out of the switch statement, so the compiler can warn
> on new (unhandled) values.

Thanks for bringing this up.  I moved this in, because the compiler started
complaining about MAX_BIT_MASK field being unhandled.  Hmmm... I think I got
confused here - I think that content::FaviconURL::IconType is NOT a bitmask and
should be rewritten to avoid using the shift operator when defining enum values
(OTOH blink::WebIconURL::Type is indeed a bitmask).

So - in the next patchset I changed content::FaviconURL::IconType so that:
- It uses consecutive values (0,1,2,3,4... rather than 0,1,2,4,8,...)
- It is a class enum that uses kModernChromiumStyleNaming (this is not
technically required, but let's me find and inspect all uses to make sure nobody
uses this enum as a bitmask).

Done (I think).

https://codereview.chromium.org/2918903002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/2918903002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:321: void
NavigateActiveAndCommit(const GURL& url, bool dont_swap_out = false) {
On 2017/06/06 20:01:30, dcheng wrote:
> defaulted bool parameters are a little sad. I guess it's just a test though...

Acknowledged.

https://codereview.chromium.org/2918903002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2918903002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3809: if (!urls.empty())
On 2017/06/06 20:01:30, dcheng wrote:
> Maybe we can short-circuit this at line 3801? I guess I could see an argument
> for doing it this way, but structuring it to return earlier makes it clear
that
> there's no filtering logic later on.

Done.

[Łukasz Anforowicz, 2017-06-06 20:57:33]

lukasza@chromium.org changed reviewers:
+ avi@chromium.org

[Łukasz Anforowicz, 2017-06-06 20:57:34]

avi@, can you PTAL?  I am tagging you because in r263486 you've left a comment
that this CL partially addresses:

  // RenderViewImpl used to be a WebFrameClient, but now RenderFrameImpl is the
  // WebFrameClient. However, many implementations of WebFrameClient methods
  // still live here and are called from RenderFrameImpl. These implementations
  // are to be moved to RenderFrameImpl <http://crbug.com/361761>.

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

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

[commit-bot: I haz the power, 2017-06-06 21:12:13]

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

[dcheng, 2017-06-06 21:12:56]

still lgtm

[Łukasz Anforowicz, 2017-06-06 21:24:00]

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

[commit-bot: I haz the power, 2017-06-06 21:24:28]

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-06-06 21:42:13]

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

[commit-bot: I haz the power, 2017-06-06 21:42:14]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2017-06-06 22:11:16]

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

[commit-bot: I haz the power, 2017-06-06 22:11:46]

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-06-06 22:28:22]

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

[commit-bot: I haz the power, 2017-06-06 22:28:23]

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

[Łukasz Anforowicz, 2017-06-06 22:45:09]

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

[commit-bot: I haz the power, 2017-06-06 22:45:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-06-06 23:10:04]

lgtm

https://codereview.chromium.org/2918903002/diff/200001/content/public/common/...
File content/public/common/favicon_url.h (left):

https://codereview.chromium.org/2918903002/diff/200001/content/public/common/...
content/public/common/favicon_url.h:24: TOUCH_PRECOMPOSED_ICON = 1 << 2
They were using bitmasks, but never combining them? :\

https://codereview.chromium.org/2918903002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2918903002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:700: for (size_t i = 0; i <
web_sizes.size(); ++i)
for (:)

https://codereview.chromium.org/2918903002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3807: for (size_t i = 0; i <
icon_urls.size(); i++) {
for (:)

[Łukasz Anforowicz, 2017-06-06 23:24:12]

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

[commit-bot: I haz the power, 2017-06-06 23:24:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-06 23:25:36]

Thanks for the review avi@ (and sorry about not compiling everything and having
to fix silly compile errors in the previous 3 patchsets... :-/ ).

https://codereview.chromium.org/2918903002/diff/200001/content/public/common/...
File content/public/common/favicon_url.h (left):

https://codereview.chromium.org/2918903002/diff/200001/content/public/common/...
content/public/common/favicon_url.h:24: TOUCH_PRECOMPOSED_ICON = 1 << 2
On 2017/06/06 23:10:04, Avi (ping after 24h) wrote:
> They were using bitmasks, but never combining them? :\

Right - I was rather confused by this: initially I had a weird MAX_BIT_MASK =
FAVICON | TOUCH_ICON | TOUCH_PRECOMPOSED_ICON in patchset 6:
https://codereview.chromium.org/2918903002/patch/100001/110007.

To be fair, blink::WebIconURL::Type *is* used as a bitmask and the values were
most likely mechanically copied from the blink enum.

https://codereview.chromium.org/2918903002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2918903002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:700: for (size_t i = 0; i <
web_sizes.size(); ++i)
On 2017/06/06 23:10:04, Avi (ping after 24h) wrote:
> for (:)

Done + also returning the vector (instead of using an out parameter).

https://codereview.chromium.org/2918903002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3807: for (size_t i = 0; i <
icon_urls.size(); i++) {
On 2017/06/06 23:10:04, Avi (ping after 24h) wrote:
> for (:)

Done.

[Łukasz Anforowicz, 2017-06-06 23:32:41]

Description was changed from

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2017-06-06 23:41:45]

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

[Łukasz Anforowicz, 2017-06-06 23:41:46]

pkotwicz@, could you PTAL?  I mostly need an owners review for
components/favicon, but as somebody quite familiar with favicons you might be
interested in reviewing the whole CL.

FWIW, the motivation for this changes to content/public/common/favicon_url.h
comes from the CR comments here:
https://codereview.chromium.org/2918903002/diff/100001/components/favicon/con...
and here:
https://codereview.chromium.org/2918903002/diff/200001/content/public/common/...

[commit-bot: I haz the power, 2017-06-07 01:27:59]

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

[commit-bot: I haz the power, 2017-06-07 01:28:00]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-07 16:16:25]

Description was changed from

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit and addresses https://crbug.com/416660.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer and
  addresses the left-over methods and the comment from r263486
  related to https://crbug.com/361761.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[pkotwicz, 2017-06-07 18:47:10]

LGTM with nit

https://codereview.chromium.org/2918903002/diff/220001/content/public/common/...
File content/public/common/favicon_url.h (right):

https://codereview.chromium.org/2918903002/diff/220001/content/public/common/...
content/public/common/favicon_url.h:22: kFavIcon,
Nit: Rename this to kFavicon (lowercase 'i')

to match the names  of other classes in Chrome e.g. FaviconURL

[Łukasz Anforowicz, 2017-06-07 19:15:21]

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

[commit-bot: I haz the power, 2017-06-07 19:15:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-07 19:16:30]

Thanks.

https://codereview.chromium.org/2918903002/diff/220001/content/public/common/...
File content/public/common/favicon_url.h (right):

https://codereview.chromium.org/2918903002/diff/220001/content/public/common/...
content/public/common/favicon_url.h:22: kFavIcon,
On 2017/06/07 18:47:10, pkotwicz wrote:
> Nit: Rename this to kFavicon (lowercase 'i')
> 
> to match the names  of other classes in Chrome e.g. FaviconURL

Done.

[Łukasz Anforowicz, 2017-06-07 19:18:32]

lukasza@chromium.org changed reviewers:
+ michaelbai@chromium.org, rockot@chromium.org

[Łukasz Anforowicz, 2017-06-07 19:18:33]

+michaelbai@ for TBR of android_webview/browser/icon_helper.cc

+rockot@ for TBR of chrome/browser/extensions/favicon_downloader*

[Łukasz Anforowicz, 2017-06-07 19:19:01]

Description was changed from

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit and addresses https://crbug.com/416660.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer and
  addresses the left-over methods and the comment from r263486
  related to https://crbug.com/361761.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit and addresses https://crbug.com/416660.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer and
  addresses the left-over methods and the comment from r263486
  related to https://crbug.com/361761.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=michaelbai@chromium.org, rockot@chromium.org
==========

[commit-bot: I haz the power, 2017-06-07 21:45:59]

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

[commit-bot: I haz the power, 2017-06-07 21:46:01]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-07 21:47:46]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-06-07 21:47:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
avi@chromium.org, pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2918903002/#ps260001
(title: "s/kFavIcon/kFavicon/g (as suggested in CR feedback from pkotwicz@).")

[commit-bot: I haz the power, 2017-06-07 21:48:26]

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-06-07 23:39:53]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496872066916110,
"parent_rev": "af7865e96d623add283d783f9cb832ba82583ae0", "commit_rev":
"178ff09570a50662f01850fb0cdbea76de51bd98"}

[commit-bot: I haz the power, 2017-06-07 23:40:05]

Description was changed from

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit and addresses https://crbug.com/416660.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer and
  addresses the left-over methods and the comment from r263486
  related to https://crbug.com/361761.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=michaelbai@chromium.org, rockot@chromium.org
==========

to

==========
Make UpdateFaviconURL IPC target a frame, rather than a view.

This CL has 3 main parts:

- Removing no-op implementation of IconURLs method from
  WebRemoteFrameImpl and moving declaration of this virtual method from
  WebFrame to WebLocalFrame.  This change is contained within
  third_party/WebKit and addresses https://crbug.com/416660.

- Moving handling of icon updates from RenderView to RenderFrame
  (this includes moving the IPC from view_messages.h to
  frame_messages.h).  This change is in the //content layer and
  addresses the left-over methods and the comment from r263486
  related to https://crbug.com/361761.

- Refactoring of content::FaviconURL::IconType to highlight the fact
  that the enum values should NOT be used as bit masks.

BUG=361761, 416660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=michaelbai@chromium.org, rockot@chromium.org

Review-Url: https://codereview.chromium.org/2918903002
Cr-Commit-Position: refs/heads/master@{#477803}
Committed:
https://chromium.googlesource.com/chromium/src/+/178ff09570a50662f01850fb0cdb...
==========

[commit-bot: I haz the power, 2017-06-07 23:40:07]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/178ff09570a50662f01850fb0cdb...