Move ZoomObserver, ZoomController and ZoomEventManager to components/.

Move ZoomObserver, ZoomController and ZoomEventManager to components/.

This CL moves the three classes into components, namespace ui_zoom, and moves
their associated source files into components/ui/zoom so that
they can be used in non-chrome configurations.

BUG=none

TBR=willchan@chromium.org  chrome/browser/profiles
TBR=fsamuel@chromium.org   */web_view/*
TBR=estade@chromium.org chrome/browser/ui/autofill
TBR=pkasting@chromium.org chrome/browser/ui not covered above
TBR=dgozman@chromium.org chrome/browser/devtools
TBR=rsesek@chromium.org chrome/browser/browser_commands_unittest.cc
TBR=sky@chromium.org chrome/browser/renderer_preferences_util.cc

Committed: https://crrev.com/7f63c6b94ee1d470791c243c4941c55315416171
Cr-Commit-Position: refs/heads/master@{#307470}

[wjmaclean, 2014-12-02 18:23:26]

Patchset #3 (id:40001) has been deleted

[wjmaclean, 2014-12-02 18:23:39]

Patchset #3 (id:60001) has been deleted

[wjmaclean, 2014-12-02 18:47:00]

Patchset #3 (id:80001) has been deleted

[wjmaclean, 2014-12-02 20:43:15]

wjmaclean@chromium.org changed reviewers:
+ rockot@chromium.org

[wjmaclean, 2014-12-02 20:43:16]

Please review changes in:
chrome/browser/extensions/*
extensions/*

[Ken Rockot(use gerrit already), 2014-12-02 20:49:33]

On 2014/12/02 20:43:16, wjmaclean wrote:
> Please review changes in:
> chrome/browser/extensions/*
> extensions/*

This seems like a non-extensions-specific concept that is used by non-extensions
chrome code as well as extensions code. Could it instead be a
//components/zoom_controller or something like that?

[wjmaclean, 2014-12-02 20:50:56]

wjmaclean@chromium.org changed reviewers:
+ asvitkine@chromium.org, fsamuel@chromium.org

[wjmaclean, 2014-12-02 20:50:57]

fsamuel@chromium.org: Please review changes in 
zoom_controller_unittest.cc

asvitkine@chromium.org: Please review changes in 
chrome/browser/ui/cocoa - minor changes to load resource image ids

[wjmaclean, 2014-12-02 20:53:04]

wjmaclean@chromium.org changed reviewers:
+ pkasting@chromium.org

[wjmaclean, 2014-12-02 20:53:04]

pkasting@chromium.org: Please review changes in 

zoom_view.cc - minor changes re loading resource image ids.

[Fady Samuel, 2014-12-02 20:58:40]

On 2014/12/02 20:49:33, Ken Rockot wrote:
> On 2014/12/02 20:43:16, wjmaclean wrote:
> > Please review changes in:
> > chrome/browser/extensions/*
> > extensions/*
> 
> This seems like a non-extensions-specific concept that is used by
non-extensions
> chrome code as well as extensions code. Could it instead be a
> //components/zoom_controller or something like that?

Do we have any targets that rely on ZoomController and friends without relying
on extensions?

[Peter Kasting, 2014-12-02 21:04:54]

c/b/ui/views/location_bar LGTM

https://codereview.chromium.org/769593003/diff/40002/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://codereview.chromium.org/769593003/diff/40002/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_view.cc:48: }
Nit: Shorter:

  int image_name = IDR_ZOOM_NORMAL;
  extensions::ZoomController::RelativeZoom relative_zoom =
      zoom_controller->GetZoomRelativeToDefault();
  if (relative_zoom == extensions::ZoomController::ZOOM_BELOW_DEFAULT_ZOOM)
    image_name = IDR_ZOOM_MINUS;
  else if (relative_zoom == extensions::ZoomController::ZOOM_ABOVE_DEFAULT_ZOOM)
    image_name = IDR_ZOOM_PLUS;

[Ken Rockot(use gerrit already), 2014-12-02 21:07:05]

Maybe not, but I don't think that justifies putting it in extensions. In
general I'd like to see fewer features being provided directly by
//extensions where possible, and in this case - unless I totally
misunderstand - this isn't even an extensions feature - it's just something
which is consumed by //chrome and [soon] //extensions.

Moving this here seems like it's only justifiable as a matter of
convenience for a refactor; I think if it's going to move, it might as well
move once, and to the right place.


On Tue, Dec 2, 2014 at 12:58 PM, <fsamuel@chromium.org> wrote:

> On 2014/12/02 20:49:33, Ken Rockot wrote:
>
>> On 2014/12/02 20:43:16, wjmaclean wrote:
>> > Please review changes in:
>> > chrome/browser/extensions/*
>> > extensions/*
>>
>
>  This seems like a non-extensions-specific concept that is used by
>>
> non-extensions
>
>> chrome code as well as extensions code. Could it instead be a
>> //components/zoom_controller or something like that?
>>
>
> Do we have any targets that rely on ZoomController and friends without
> relying
> on extensions?
>
> https://codereview.chromium.org/769593003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2014-12-02 21:07:58]

On 2014/12/02 20:49:33, Ken Rockot wrote:
> On 2014/12/02 20:43:16, wjmaclean wrote:
> > Please review changes in:
> > chrome/browser/extensions/*
> > extensions/*
> 
> This seems like a non-extensions-specific concept that is used by
non-extensions
> chrome code as well as extensions code. Could it instead be a
> //components/zoom_controller or something like that?

I agree with this.

[wjmaclean, 2014-12-02 21:09:23]

On 2014/12/02 21:04:54, Peter Kasting wrote:
> c/b/ui/views/location_bar LGTM
> 
>
https://codereview.chromium.org/769593003/diff/40002/chrome/browser/ui/views/...
> File chrome/browser/ui/views/location_bar/zoom_view.cc (right):
> 
>
https://codereview.chromium.org/769593003/diff/40002/chrome/browser/ui/views/...
> chrome/browser/ui/views/location_bar/zoom_view.cc:48: }
> Nit: Shorter:
> 
>   int image_name = IDR_ZOOM_NORMAL;
>   extensions::ZoomController::RelativeZoom relative_zoom =
>       zoom_controller->GetZoomRelativeToDefault();
>   if (relative_zoom == extensions::ZoomController::ZOOM_BELOW_DEFAULT_ZOOM)
>     image_name = IDR_ZOOM_MINUS;
>   else if (relative_zoom ==
extensions::ZoomController::ZOOM_ABOVE_DEFAULT_ZOOM)
>     image_name = IDR_ZOOM_PLUS;

Nice! Will add this :-)

[wjmaclean, 2014-12-02 21:19:45]

On 2014/12/02 21:07:05, Ken Rockot wrote:
> Maybe not, but I don't think that justifies putting it in extensions. In
> general I'd like to see fewer features being provided directly by
> //extensions where possible, and in this case - unless I totally
> misunderstand - this isn't even an extensions feature - it's just something
> which is consumed by //chrome and [soon] //extensions.
> 
> Moving this here seems like it's only justifiable as a matter of
> convenience for a refactor; I think if it's going to move, it might as well
> move once, and to the right place.
> 
> 
> On Tue, Dec 2, 2014 at 12:58 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2014/12/02 20:49:33, Ken Rockot wrote:
> >
> >> On 2014/12/02 20:43:16, wjmaclean wrote:
> >> > Please review changes in:
> >> > chrome/browser/extensions/*
> >> > extensions/*
> >>
> >
> >  This seems like a non-extensions-specific concept that is used by
> >>
> > non-extensions
> >
> >> chrome code as well as extensions code. Could it instead be a
> >> //components/zoom_controller or something like that?
> >>
> >
> > Do we have any targets that rely on ZoomController and friends without
> > relying
> > on extensions?
> >
> > https://codereview.chromium.org/769593003/

The main impetus for putting it here is ZoomController's dependence on
Extension*, but this can presumably be handled by a delegate. I can revise it on
that basis., but will need to add a delegate implementation to extensions/ in
the process.

[wjmaclean, 2014-12-03 21:32:12]

wjmaclean@chromium.org changed reviewers:
+ jochen@chromium.org

[wjmaclean, 2014-12-03 21:32:13]

jochen@chromium.org: Please review changes in 

components/

PTAL as this CL now moves the zoom code to components instead.

I'm a bit unsure of all the gypi/DEPS wiring required to add this to components,
so please let me know if I've made mistakes or broken any conventions.

[Ken Rockot(use gerrit already), 2014-12-03 21:42:48]

Thank you for doing this in components. All extensions code LGTM.

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
components/ui/zoom/zoom_controller.cc:77: // An extension did not initiate this
zoom change.
nit: extension -> client?

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
components/ui/zoom/zoom_controller.cc:298: }  // namespace extensions
nit: extensions -> components

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
components/ui/zoom/zoom_controller.h:135: const scoped_refptr<const
ZoomRequestClient>& extension);
nit: client -> extension

[wjmaclean, 2014-12-04 15:04:29]

wjmaclean@chromium.org changed reviewers:
+ jamescook@chromium.org

[wjmaclean, 2014-12-04 15:04:30]

jamescook@chromium.org: Please review changes in 

extensions/browser/DEPS

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
components/ui/zoom/zoom_controller.cc:77: // An extension did not initiate this
zoom change.
On 2014/12/03 21:42:48, Ken Rockot wrote:
> nit: extension -> client?

Done.

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
components/ui/zoom/zoom_controller.cc:298: }  // namespace extensions
On 2014/12/03 21:42:48, Ken Rockot wrote:
> nit: extensions -> components

Done. (thanks for spotting these!)

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/769593003/diff/110001/components/ui/zoom/zoom...
components/ui/zoom/zoom_controller.h:135: const scoped_refptr<const
ZoomRequestClient>& extension);
On 2014/12/03 21:42:48, Ken Rockot wrote:
> nit: client -> extension

Done.

[Alexei Svitkine (slow), 2014-12-04 15:52:02]

cocoa lgtm with a nit

https://codereview.chromium.org/769593003/diff/110001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm (right):

https://codereview.chromium.org/769593003/diff/110001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm:9: using
components::ZoomController;
Nit: Just use the namespace where this is referenced - only a couple of places.

[wjmaclean, 2014-12-04 15:57:45]

https://codereview.chromium.org/769593003/diff/110001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm (right):

https://codereview.chromium.org/769593003/diff/110001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/zoom_decoration_unittest.mm:9: using
components::ZoomController;
On 2014/12/04 15:52:02, Alexei Svitkine wrote:
> Nit: Just use the namespace where this is referenced - only a couple of
places.

Done.

[James Cook, 2014-12-04 17:17:16]

LGTM with nits

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
components/ui/zoom/zoom_event_manager.cc:40: }  // namespace extensions
ditto

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
components/ui/zoom/zoom_event_manager.h:55: }  // namespace extensions
nit: namespace components (elsewhere too I think)

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_observer.h (right):

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
components/ui/zoom/zoom_observer.h:23: }  // namespace extensions
ditto

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/DEPS...
extensions/browser/DEPS:9: "+components/ui/zoom",
nit: alphabetize

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
File extensions/browser/extension_zoom_request_client.cc (right):

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
extensions/browser/extension_zoom_request_client.cc:17: }  // namespace
Extensions
nit: *e*xtensions

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
File extensions/browser/extension_zoom_request_client.h (right):

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
extensions/browser/extension_zoom_request_client.h:16: class
ExtensionZoomRequestClient : public components::ZoomRequestClient {
nit: Class comment explaining why this must exist and/or what it does.

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
extensions/browser/extension_zoom_request_client.h:23: virtual
~ExtensionZoomRequestClient();
nit: no virtual, "override" instead

[wjmaclean, 2014-12-04 18:27:17]

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
components/ui/zoom/zoom_event_manager.cc:40: }  // namespace extensions
On 2014/12/04 17:17:16, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
components/ui/zoom/zoom_event_manager.h:55: }  // namespace extensions
On 2014/12/04 17:17:16, James Cook wrote:
> nit: namespace components (elsewhere too I think)

Done.

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
File components/ui/zoom/zoom_observer.h (right):

https://codereview.chromium.org/769593003/diff/150001/components/ui/zoom/zoom...
components/ui/zoom/zoom_observer.h:23: }  // namespace extensions
On 2014/12/04 17:17:16, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/DEPS...
extensions/browser/DEPS:9: "+components/ui/zoom",
On 2014/12/04 17:17:16, James Cook wrote:
> nit: alphabetize

Done.

(Big Bird silently weeps for me ... ;-) )

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
File extensions/browser/extension_zoom_request_client.h (right):

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
extensions/browser/extension_zoom_request_client.h:16: class
ExtensionZoomRequestClient : public components::ZoomRequestClient {
On 2014/12/04 17:17:16, James Cook wrote:
> nit: Class comment explaining why this must exist and/or what it does.

Done.

https://codereview.chromium.org/769593003/diff/150001/extensions/browser/exte...
extensions/browser/extension_zoom_request_client.h:23: virtual
~ExtensionZoomRequestClient();
On 2014/12/04 17:17:16, James Cook wrote:
> nit: no virtual, "override" instead

Done.

Interesting, was unaware of the 'override' use with destructors.

[wjmaclean, 2014-12-04 22:41:48]

jochen@ - Will you be able to look at this?

If so, I'd appreciate your thoughts on the win_dbg compile error
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg/...).

I'm currently experiment with using 'static int' for the specialized version of
kLocatorKey.

[wjmaclean, 2014-12-04 22:55:40]

On 2014/12/04 22:41:48, wjmaclean wrote:
> jochen@ - Will you be able to look at this?
> 
> If so, I'd appreciate your thoughts on the win_dbg compile error
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg/...).
> 
> I'm currently experiment with using 'static int' for the specialized version
of
> kLocatorKey.

Experiment failed:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg/...

[Evan Stade, 2014-12-05 02:28:04]

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

[Evan Stade, 2014-12-05 02:28:06]

autofill lgtm

[jochen (gone - plz use gerrit), 2014-12-08 12:12:18]

have you considered to just make this a static library instead of going through
the trouble of making WebContentsUserData work with this?

The component appears to be so small that making it a shared library might be a
bit overkill.

[wjmaclean, 2014-12-08 14:04:32]

On 2014/12/08 12:12:18, jochen (slow) wrote:
> have you considered to just make this a static library instead of going
through
> the trouble of making WebContentsUserData work with this?
> 
> The component appears to be so small that making it a shared library might be
a
> bit overkill.

Based on trying to compile PS#14, just making it a static library doesn't solve
the compilation & linking issues (unless PS#14 has an error I'm unaware of).

[wjmaclean, 2014-12-08 16:51:38]

Using static_library now ... PTAL?

[jochen (gone - plz use gerrit), 2014-12-09 13:52:29]

lgtm

https://codereview.chromium.org/769593003/diff/340001/components/ui/zoom/BUIL...
File components/ui/zoom/BUILD.gn (right):

https://codereview.chromium.org/769593003/diff/340001/components/ui/zoom/BUIL...
components/ui/zoom/BUILD.gn:14: defines = [ "UI_ZOOM_IMPLEMENTATION"]
same

https://codereview.chromium.org/769593003/diff/340001/components/ui_zoom.gypi
File components/ui_zoom.gypi (right):

https://codereview.chromium.org/769593003/diff/340001/components/ui_zoom.gypi...
components/ui_zoom.gypi:21: 'defines': [
not needed anymore

[wjmaclean, 2014-12-09 13:59:23]

https://codereview.chromium.org/769593003/diff/340001/components/ui/zoom/BUIL...
File components/ui/zoom/BUILD.gn (right):

https://codereview.chromium.org/769593003/diff/340001/components/ui/zoom/BUIL...
components/ui/zoom/BUILD.gn:14: defines = [ "UI_ZOOM_IMPLEMENTATION"]
On 2014/12/09 13:52:28, jochen (slow) wrote:
> same

Done.

https://codereview.chromium.org/769593003/diff/340001/components/ui_zoom.gypi
File components/ui_zoom.gypi (right):

https://codereview.chromium.org/769593003/diff/340001/components/ui_zoom.gypi...
components/ui_zoom.gypi:21: 'defines': [
On 2014/12/09 13:52:29, jochen (slow) wrote:
> not needed anymore

Done.

[wjmaclean, 2014-12-09 13:59:35]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-09 14:00:03]

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

[commit-bot: I haz the power, 2014-12-09 15:00:06]

Committed patchset #17 (id:360001)

[commit-bot: I haz the power, 2014-12-09 15:00:55]

Patchset 17 (id:??) landed as
https://crrev.com/7f63c6b94ee1d470791c243c4941c55315416171
Cr-Commit-Position: refs/heads/master@{#307470}