Partially move history_types.{cc,h} to //components/history

Partially move history_types.{cc,h} to //components/history

Move all types from //chrome/browser/history/history_types.{cc,h} that do
not depends on content types to //components/history.

BUG=371816
TBR=msw@chromium.org
TBR=jochen@chromium.org

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

[sdefresne, 2014-08-04 09:50:21]

jif@: please take a look at the whole changelist

msw@: TBR for adding a dependency to ui/gfx in components/history

jochen@: TBR for adding a dependency to components/favicon_base in
components/history

jhawkins@: TBR for files affected by the file move (#include changes) in
//chrome/browser/ui/webui, //chrome/browser/jumplist_win.cc and change to
//chrome/chrome_browser.gypi

[James Hawkins, 2014-08-04 09:52:58]

lgtm

[jif, 2014-08-04 11:39:10]

LGTM

[sdefresne, 2014-08-04 11:55:21]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-08-04 11:55:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sdefresne@chromium.org/439713004/20001

[commit-bot: I haz the power, 2014-08-04 14:17:59]

Change committed as 287333

[msw, 2014-08-04 16:56:32]

Why didn't you get review from a components/history owner? +brettw and sky
I recommend dropping the ui/gfx dependency as I describe below.

https://codereview.chromium.org/439713004/diff/20001/components/history/core/...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/439713004/diff/20001/components/history/core/...
components/history/core/browser/history_types.h:23: #include
"ui/gfx/image/image.h"
You don't actually need this include afaict.

https://codereview.chromium.org/439713004/diff/20001/components/history/core/...
components/history/core/browser/history_types.h:382: gfx::Size pixel_size;
You can drop the new gfx dependency if you store two width/height pairs instead
of the two |pixel_size| gfx::Size values. I recommend doing this.

[brettw, 2014-08-04 17:01:34]

LGTM, although I agree that you should've gotten a history owners.

I know it already existed before this patch, but what is the point of the "core"
directory under components/history? Will there be other siblings? Otherwise, it
seems better to delete this extra level.

[sdefresne, 2014-08-04 17:06:29]

A revert of this CL has been created in
https://codereview.chromium.org/428253008/ by sdefresne@chromium.org.

The reason for reverting is: Missing OWNERS LGTM/.

[brettw, 2014-08-04 17:12:33]

On 2014/08/04 17:06:29, sdefresne wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/428253008/ by mailto:sdefresne@chromium.org.
> 
> The reason for reverting is: Missing OWNERS LGTM/.

I don't think you need to revert, I just said LGTM.

[blundell, 2014-08-07 07:23:09]

On 2014/08/04 16:56:32, msw wrote:
> Why didn't you get review from a components/history owner? +brettw and sky

There was a little bit of benign crossed wires here. Sylvain was a temporary
OWNER of //components/history and //chrome/browser/history for the purposes of
componentization, which is a practice that we employ when doing
componentizations to allow the large amount of relatively mechanical work to
proceed quickly (we coordinated with sky@ on this but I neglected to give
brettw@ the context by mistake). He has been looping in brettw@ as FYI on all
these CLs, and it was just an oversight that that wasn't the case here. Anyway,
we got it all sorted now :).

In general, if you see a componentization CL landing without an LGTM from an
OWNER that you would expect, it's because those OWNERS have given
componentization people the go-ahead on becoming temporary owners for
componentization purposes. Of course, we make sure that any work that requires a
non-trivial judgment call about e.g. the structure of the feature goes through a
permanent owner.

Thanks,

Colin

> I recommend dropping the ui/gfx dependency as I describe below.
> 
>
https://codereview.chromium.org/439713004/diff/20001/components/history/core/...
> File components/history/core/browser/history_types.h (right):
> 
>
https://codereview.chromium.org/439713004/diff/20001/components/history/core/...
> components/history/core/browser/history_types.h:23: #include
> "ui/gfx/image/image.h"
> You don't actually need this include afaict.
> 
>
https://codereview.chromium.org/439713004/diff/20001/components/history/core/...
> components/history/core/browser/history_types.h:382: gfx::Size pixel_size;
> You can drop the new gfx dependency if you store two width/height pairs
instead
> of the two |pixel_size| gfx::Size values. I recommend doing this.