Moves code from chromeos/ime to ui/base/ime/chromeos.

Moves code from chromeos/ime to ui/base/ime/chromeos.

This is because the code under chromeos/ime has inappropriate dependencies to src/ui.
This cl doesn't include any logic changes in code, and only contains changes of moving files, changing includes, and necessary changes in GYPs and build rules.

Note:
1) the input_methods.txt & gen_input_methods.py remain unchanged because auto-test requires input_methods.txt file to be available, and input_method_whitelist needs the generated input_methods.h for compile. It could be moved in later CLs.
2) We've been discussing the naming for ui/chromeos/ime, which could be renamed to something for "views". It can be done in a separated cl.

TBR=sky@chromium.org
BUG=431863
TEST=Verified on linux_chromeos.

Committed: https://crrev.com/9917a3671cafa9d31a071a530b9ce10ce0661bc9
Cr-Commit-Position: refs/heads/master@{#305964}

[Yuki, 2014-11-17 08:05:59]

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

[Yuki, 2014-11-17 08:06:00]

I don't see why we need to move files into ui/chromeos/ime/.

Seeing ui/chromeos/ime/, there are candidate windows, info list, menus, and mode
indicators.  All of them are UI components.

Why do we want to move keyboard layouts and/or engines (internal state managers)
into ui/?

[Shu Chen, 2014-11-17 08:12:36]

On 2014/11/17 08:06:00, Yuki wrote:
> I don't see why we need to move files into ui/chromeos/ime/.
> 
> Seeing ui/chromeos/ime/, there are candidate windows, info list, menus, and
mode
> indicators.  All of them are UI components.
> 
> Why do we want to move keyboard layouts and/or engines (internal state
managers)
> into ui/?

Because ime_keyboard_x11/ozone need to depend something under src/ui.
 - ime_keyboard_x11 depends on src/ui/gfx
 - ime_keyboard_ozone will depends on src/ui/ozone

IMO, managing XKB key mappings & modifier states (caps/numlock stuff) is UI
related and shouldn't be under src/chromeos/ime.

[Yuki, 2014-11-17 08:23:37]

On 2014/11/17 08:12:36, Shu Chen wrote:
> On 2014/11/17 08:06:00, Yuki wrote:
> > I don't see why we need to move files into ui/chromeos/ime/.
> > 
> > Seeing ui/chromeos/ime/, there are candidate windows, info list, menus, and
> mode
> > indicators.  All of them are UI components.
> > 
> > Why do we want to move keyboard layouts and/or engines (internal state
> managers)
> > into ui/?
> 
> Because ime_keyboard_x11/ozone need to depend something under src/ui.
>  - ime_keyboard_x11 depends on src/ui/gfx
>  - ime_keyboard_ozone will depends on src/ui/ozone
> 
> IMO, managing XKB key mappings & modifier states (caps/numlock stuff) is UI
> related and shouldn't be under src/chromeos/ime.

Please read my comment in the bug.  For x11, we can solve the issue in another
way.  For ozone, I don't have any info about you guys' plan.

In terms of MVC, it seems that you're moving(or merging) models into views.  I
think that ui/chromeos/ime/ was designed as views while chromeos/ime/ was
designed as models.  Managing key mappings and modifier states is a task of
models, and drawing them is a task of views.  In general, views and controls
should depend on models, and models shouldn't depend on views or controls. 
Models should provide appropriate APIs including callbacks to views and
controls.

Since "input" is natively a user interface, we can call everything related to
input "UI-related" if we don't think about model/view separation.  We need to
consider how to separate models from views.

[Shu Chen, 2014-11-17 09:11:43]

On 2014/11/17 08:23:37, Yuki wrote:
> On 2014/11/17 08:12:36, Shu Chen wrote:
> > On 2014/11/17 08:06:00, Yuki wrote:
> > > I don't see why we need to move files into ui/chromeos/ime/.
> > > 
> > > Seeing ui/chromeos/ime/, there are candidate windows, info list, menus,
and
> > mode
> > > indicators.  All of them are UI components.
> > > 
> > > Why do we want to move keyboard layouts and/or engines (internal state
> > managers)
> > > into ui/?
> > 
> > Because ime_keyboard_x11/ozone need to depend something under src/ui.
> >  - ime_keyboard_x11 depends on src/ui/gfx
> >  - ime_keyboard_ozone will depends on src/ui/ozone
> > 
> > IMO, managing XKB key mappings & modifier states (caps/numlock stuff) is UI
> > related and shouldn't be under src/chromeos/ime.
> 
> Please read my comment in the bug.  For x11, we can solve the issue in another
> way.  For ozone, I don't have any info about you guys' plan.
> 
> In terms of MVC, it seems that you're moving(or merging) models into views.  I
> think that ui/chromeos/ime/ was designed as views while chromeos/ime/ was
> designed as models.  Managing key mappings and modifier states is a task of
> models, and drawing them is a task of views.  In general, views and controls
> should depend on models, and models shouldn't depend on views or controls. 
> Models should provide appropriate APIs including callbacks to views and
> controls.
> 
> Since "input" is natively a user interface, we can call everything related to
> input "UI-related" if we don't think about model/view separation.  We need to
> consider how to separate models from views.

Do you think it's appropriate to move ime_keyboard_x11/ozone to
chrome/browser/chromeos/input_method?
InputMethodManagerImpl instantiates ime_keyboard_x11/ozone, so I think it may
make sense to put ime_keyboard_x11/ozone classes close to their client.

[Yuki, 2014-11-17 09:22:00]

On 2014/11/17 09:11:43, Shu Chen wrote:
> On 2014/11/17 08:23:37, Yuki wrote:
> > On 2014/11/17 08:12:36, Shu Chen wrote:
> > > On 2014/11/17 08:06:00, Yuki wrote:
> > > > I don't see why we need to move files into ui/chromeos/ime/.
> > > > 
> > > > Seeing ui/chromeos/ime/, there are candidate windows, info list, menus,
> and
> > > mode
> > > > indicators.  All of them are UI components.
> > > > 
> > > > Why do we want to move keyboard layouts and/or engines (internal state
> > > managers)
> > > > into ui/?
> > > 
> > > Because ime_keyboard_x11/ozone need to depend something under src/ui.
> > >  - ime_keyboard_x11 depends on src/ui/gfx
> > >  - ime_keyboard_ozone will depends on src/ui/ozone
> > > 
> > > IMO, managing XKB key mappings & modifier states (caps/numlock stuff) is
UI
> > > related and shouldn't be under src/chromeos/ime.
> > 
> > Please read my comment in the bug.  For x11, we can solve the issue in
another
> > way.  For ozone, I don't have any info about you guys' plan.
> > 
> > In terms of MVC, it seems that you're moving(or merging) models into views. 
I
> > think that ui/chromeos/ime/ was designed as views while chromeos/ime/ was
> > designed as models.  Managing key mappings and modifier states is a task of
> > models, and drawing them is a task of views.  In general, views and controls
> > should depend on models, and models shouldn't depend on views or controls. 
> > Models should provide appropriate APIs including callbacks to views and
> > controls.
> > 
> > Since "input" is natively a user interface, we can call everything related
to
> > input "UI-related" if we don't think about model/view separation.  We need
to
> > consider how to separate models from views.
> 
> Do you think it's appropriate to move ime_keyboard_x11/ozone to
> chrome/browser/chromeos/input_method?
> InputMethodManagerImpl instantiates ime_keyboard_x11/ozone, so I think it may
> make sense to put ime_keyboard_x11/ozone classes close to their client.

Thinking client/server model and consumer/producer model, in general, it doesn't
make sense to put them close.  We should rather separate them clearly.

Please consider that DEPS (dependency problem) helps us make better design.  It
helps design more visible.  We need cleaner design, not cleaner DEPS.  DEPS will
be cleaner just as a result following cleaner design.  DEPS is NOT a goal.

Putting everything in one place is easy.  We need to rather think how to
separate things.

[Yuki, 2014-11-17 09:33:43]

On 2014/11/17 09:22:00, Yuki wrote:
> On 2014/11/17 09:11:43, Shu Chen wrote:
> > On 2014/11/17 08:23:37, Yuki wrote:
> > > On 2014/11/17 08:12:36, Shu Chen wrote:
> > > > On 2014/11/17 08:06:00, Yuki wrote:
> > > > > I don't see why we need to move files into ui/chromeos/ime/.
> > > > > 
> > > > > Seeing ui/chromeos/ime/, there are candidate windows, info list,
menus,
> > and
> > > > mode
> > > > > indicators.  All of them are UI components.
> > > > > 
> > > > > Why do we want to move keyboard layouts and/or engines (internal state
> > > > managers)
> > > > > into ui/?
> > > > 
> > > > Because ime_keyboard_x11/ozone need to depend something under src/ui.
> > > >  - ime_keyboard_x11 depends on src/ui/gfx
> > > >  - ime_keyboard_ozone will depends on src/ui/ozone
> > > > 
> > > > IMO, managing XKB key mappings & modifier states (caps/numlock stuff) is
> UI
> > > > related and shouldn't be under src/chromeos/ime.
> > > 
> > > Please read my comment in the bug.  For x11, we can solve the issue in
> another
> > > way.  For ozone, I don't have any info about you guys' plan.
> > > 
> > > In terms of MVC, it seems that you're moving(or merging) models into
views. 
> I
> > > think that ui/chromeos/ime/ was designed as views while chromeos/ime/ was
> > > designed as models.  Managing key mappings and modifier states is a task
of
> > > models, and drawing them is a task of views.  In general, views and
controls
> > > should depend on models, and models shouldn't depend on views or controls.

> > > Models should provide appropriate APIs including callbacks to views and
> > > controls.
> > > 
> > > Since "input" is natively a user interface, we can call everything related
> to
> > > input "UI-related" if we don't think about model/view separation.  We need
> to
> > > consider how to separate models from views.
> > 
> > Do you think it's appropriate to move ime_keyboard_x11/ozone to
> > chrome/browser/chromeos/input_method?
> > InputMethodManagerImpl instantiates ime_keyboard_x11/ozone, so I think it
may
> > make sense to put ime_keyboard_x11/ozone classes close to their client.
> 
> Thinking client/server model and consumer/producer model, in general, it
doesn't
> make sense to put them close.  We should rather separate them clearly.
> 
> Please consider that DEPS (dependency problem) helps us make better design. 
It
> helps design more visible.  We need cleaner design, not cleaner DEPS.  DEPS
will
> be cleaner just as a result following cleaner design.  DEPS is NOT a goal.
> 
> Putting everything in one place is easy.  We need to rather think how to
> separate things.

Node that "chrome/browser/..." is a kind of the last resort where you can depend
on everything, so each component should live in another place as much as
possible.  We'd like to minimize the code in chrome/browser/.

[stevenjb, 2014-11-17 18:29:22]

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

[stevenjb, 2014-11-17 18:29:24]

I don't think it makes sense to split ime_keyboard across src/chromeos and
src/ui/chromeos, I think it will just confuse developers and make it harder to
read the code. Given that it doesn't appear to depend on anything else in
src/chromeos/ime, I think we should move all ime_keyboard classes to
src/ui/chromeos.

https://codereview.chromium.org/727143002/diff/60001/chromeos/ime/fake_ime_ke...
File chromeos/ime/fake_ime_keyboard.h (right):

https://codereview.chromium.org/727143002/diff/60001/chromeos/ime/fake_ime_ke...
chromeos/ime/fake_ime_keyboard.h:17: class CHROMEOS_EXPORT FakeImeKeyboard :
public ImeKeyboard {
We need to either keep the #include or remove CHROMEOS_EXPORT

[Shu Chen, 2014-11-18 02:50:14]

Steven is suggesting to move all related classes including ImeKeyboard,
ImeKeyboardX11, ImeKeyboardOzone & InputMethodManager to ui/chromeos/ime.

Yuki is suggesting (if I understand correctly) to pass certain delegate as
parameter to ImeKeyboard::Create(). I assume the delegate is instantiated from
chrome/browser/chromeos/input_method.

Steven's approach is easier to do but conflicts with Yuki's justification of MVC
pattern.

And I think Yuki's solution is kind of over-designing.
Suppose ImeKeyboard is a delegate to having different implementation for
different platforms (X11/Ozone). For the dependency issue to be solved, I think
it doesn't worth to design delegate-holds-delegate thing.

As Yuki's point, chromeos/ime is a shared component and should be Model.
However, that Model is only used in chrome/browser which is at higher level of
UI.
IMO, as chromeos/ime is really not "shared" to non-UI part of ChromeOS, a
natural solution is to move it to ui/chromeos/ime/model or something like that
under src/ui.

Thoughts?

[Yuki, 2014-11-18 03:24:08]

On 2014/11/18 02:50:14, Shu Chen wrote:
> Steven is suggesting to move all related classes including ImeKeyboard,
> ImeKeyboardX11, ImeKeyboardOzone & InputMethodManager to ui/chromeos/ime.
> 
> Yuki is suggesting (if I understand correctly) to pass certain delegate as
> parameter to ImeKeyboard::Create(). I assume the delegate is instantiated from
> chrome/browser/chromeos/input_method.
> 
> Steven's approach is easier to do but conflicts with Yuki's justification of
MVC
> pattern.
> 
> And I think Yuki's solution is kind of over-designing.
> Suppose ImeKeyboard is a delegate to having different implementation for
> different platforms (X11/Ozone). For the dependency issue to be solved, I
think
> it doesn't worth to design delegate-holds-delegate thing.
> 
> As Yuki's point, chromeos/ime is a shared component and should be Model.
> However, that Model is only used in chrome/browser which is at higher level of
> UI.
> IMO, as chromeos/ime is really not "shared" to non-UI part of ChromeOS, a
> natural solution is to move it to ui/chromeos/ime/model or something like that
> under src/ui.
> 
> Thoughts?

ImeKeyboardX11::Create is used only at input_method_manager_impl.cc:842, so
I thought you can pass a XDisplay directly there.  Like;

    #if defined(USE_X11)
      keyboard_.reset(ImeKeyboardX11::Create(GetXDisplay()));
    #endif

So the change will be so small so far.

Having said that, Shu's plan of moving to ui/chromeos/ime/model sounds good
to me.  I also agree with Steven's point that we have too many places to have
IME-related code.  Shu's idea will solve all the problems, I think.

If the name of the new directory will be "model", we might want to have "view",
though.  I don't stick to "model", any better name is welcome.  "model" is okay,
too.

[Shu Chen, 2014-11-18 03:32:31]

On 2014/11/18 03:24:08, Yuki wrote:
> On 2014/11/18 02:50:14, Shu Chen wrote:
> > Steven is suggesting to move all related classes including ImeKeyboard,
> > ImeKeyboardX11, ImeKeyboardOzone & InputMethodManager to ui/chromeos/ime.
> > 
> > Yuki is suggesting (if I understand correctly) to pass certain delegate as
> > parameter to ImeKeyboard::Create(). I assume the delegate is instantiated
from
> > chrome/browser/chromeos/input_method.
> > 
> > Steven's approach is easier to do but conflicts with Yuki's justification of
> MVC
> > pattern.
> > 
> > And I think Yuki's solution is kind of over-designing.
> > Suppose ImeKeyboard is a delegate to having different implementation for
> > different platforms (X11/Ozone). For the dependency issue to be solved, I
> think
> > it doesn't worth to design delegate-holds-delegate thing.
> > 
> > As Yuki's point, chromeos/ime is a shared component and should be Model.
> > However, that Model is only used in chrome/browser which is at higher level
of
> > UI.
> > IMO, as chromeos/ime is really not "shared" to non-UI part of ChromeOS, a
> > natural solution is to move it to ui/chromeos/ime/model or something like
that
> > under src/ui.
> > 
> > Thoughts?
> 
> ImeKeyboardX11::Create is used only at input_method_manager_impl.cc:842, so
> I thought you can pass a XDisplay directly there.  Like;
> 
>     #if defined(USE_X11)
>       keyboard_.reset(ImeKeyboardX11::Create(GetXDisplay()));
>     #endif
For Ozone, the interface of ImeKeyboardX11::Create() may take different type of
parameters.
> 
> So the change will be so small so far.
> 
> Having said that, Shu's plan of moving to ui/chromeos/ime/model sounds good
> to me.  I also agree with Steven's point that we have too many places to have
> IME-related code.  Shu's idea will solve all the problems, I think.
> 
> If the name of the new directory will be "model", we might want to have
"view",
> though.  I don't stick to "model", any better name is welcome.  "model" is
okay,
> too.

Yuki, can you please confirm whether you agree with Steven's comment to move all
related classes (especially InputMethodManager which holds instance of
ImeKeyboard) to src/ui?
For the naming, if InputMethodManager is moving, I would suggest to use
"ui/chromeos/ime/controller" instead of "ui/chromeos/ime/model".
Or to avoid confusion, move all to "ui/chromeos/ime" and move existing stuff to
"ui/chromeos/ime/views". SG?

[Yuki, 2014-11-18 03:54:17]

On 2014/11/18 03:32:31, Shu Chen wrote:
> On 2014/11/18 03:24:08, Yuki wrote:
> > On 2014/11/18 02:50:14, Shu Chen wrote:
> > > Steven is suggesting to move all related classes including ImeKeyboard,
> > > ImeKeyboardX11, ImeKeyboardOzone & InputMethodManager to ui/chromeos/ime.
> > > 
> > > Yuki is suggesting (if I understand correctly) to pass certain delegate as
> > > parameter to ImeKeyboard::Create(). I assume the delegate is instantiated
> from
> > > chrome/browser/chromeos/input_method.
> > > 
> > > Steven's approach is easier to do but conflicts with Yuki's justification
of
> > MVC
> > > pattern.
> > > 
> > > And I think Yuki's solution is kind of over-designing.
> > > Suppose ImeKeyboard is a delegate to having different implementation for
> > > different platforms (X11/Ozone). For the dependency issue to be solved, I
> > think
> > > it doesn't worth to design delegate-holds-delegate thing.
> > > 
> > > As Yuki's point, chromeos/ime is a shared component and should be Model.
> > > However, that Model is only used in chrome/browser which is at higher
level
> of
> > > UI.
> > > IMO, as chromeos/ime is really not "shared" to non-UI part of ChromeOS, a
> > > natural solution is to move it to ui/chromeos/ime/model or something like
> that
> > > under src/ui.
> > > 
> > > Thoughts?
> > 
> > ImeKeyboardX11::Create is used only at input_method_manager_impl.cc:842, so
> > I thought you can pass a XDisplay directly there.  Like;
> > 
> >     #if defined(USE_X11)
> >       keyboard_.reset(ImeKeyboardX11::Create(GetXDisplay()));
> >     #endif
> For Ozone, the interface of ImeKeyboardX11::Create() may take different type
of
> parameters.
> > 
> > So the change will be so small so far.
> > 
> > Having said that, Shu's plan of moving to ui/chromeos/ime/model sounds good
> > to me.  I also agree with Steven's point that we have too many places to
have
> > IME-related code.  Shu's idea will solve all the problems, I think.
> > 
> > If the name of the new directory will be "model", we might want to have
> "view",
> > though.  I don't stick to "model", any better name is welcome.  "model" is
> okay,
> > too.
> 
> Yuki, can you please confirm whether you agree with Steven's comment to move
all
> related classes (especially InputMethodManager which holds instance of
> ImeKeyboard) to src/ui?
> For the naming, if InputMethodManager is moving, I would suggest to use
> "ui/chromeos/ime/controller" instead of "ui/chromeos/ime/model".
> Or to avoid confusion, move all to "ui/chromeos/ime" and move existing stuff
to
> "ui/chromeos/ime/views". SG?

I support to move all the code in chromeos/ime/ to ui/chromeos/ime/TBD/.  For
the name,
"views" is used for Views framework, so please avoid it.  "view" (without s) is
probably
okay.  Seeing contents of chromeos/ime/, "controller" sounds a little weird to
me.
It holds a variety of things, not limited to controls.

I'd prefer "ui/chromeos/ime" and "ui/chromeos/ime/view", because the old
contents of
"ui/chromeos/ime" really focus to implement UI views.

In future, if we'd feel having too many things in ui/chromeos/ime/, then we can
create
more subdirectories and separate the code into components.

[Shu Chen, 2014-11-18 03:55:46]

On 2014/11/18 03:54:17, Yuki wrote:
> On 2014/11/18 03:32:31, Shu Chen wrote:
> > On 2014/11/18 03:24:08, Yuki wrote:
> > > On 2014/11/18 02:50:14, Shu Chen wrote:
> > > > Steven is suggesting to move all related classes including ImeKeyboard,
> > > > ImeKeyboardX11, ImeKeyboardOzone & InputMethodManager to
ui/chromeos/ime.
> > > > 
> > > > Yuki is suggesting (if I understand correctly) to pass certain delegate
as
> > > > parameter to ImeKeyboard::Create(). I assume the delegate is
instantiated
> > from
> > > > chrome/browser/chromeos/input_method.
> > > > 
> > > > Steven's approach is easier to do but conflicts with Yuki's
justification
> of
> > > MVC
> > > > pattern.
> > > > 
> > > > And I think Yuki's solution is kind of over-designing.
> > > > Suppose ImeKeyboard is a delegate to having different implementation for
> > > > different platforms (X11/Ozone). For the dependency issue to be solved,
I
> > > think
> > > > it doesn't worth to design delegate-holds-delegate thing.
> > > > 
> > > > As Yuki's point, chromeos/ime is a shared component and should be Model.
> > > > However, that Model is only used in chrome/browser which is at higher
> level
> > of
> > > > UI.
> > > > IMO, as chromeos/ime is really not "shared" to non-UI part of ChromeOS,
a
> > > > natural solution is to move it to ui/chromeos/ime/model or something
like
> > that
> > > > under src/ui.
> > > > 
> > > > Thoughts?
> > > 
> > > ImeKeyboardX11::Create is used only at input_method_manager_impl.cc:842,
so
> > > I thought you can pass a XDisplay directly there.  Like;
> > > 
> > >     #if defined(USE_X11)
> > >       keyboard_.reset(ImeKeyboardX11::Create(GetXDisplay()));
> > >     #endif
> > For Ozone, the interface of ImeKeyboardX11::Create() may take different type
> of
> > parameters.
> > > 
> > > So the change will be so small so far.
> > > 
> > > Having said that, Shu's plan of moving to ui/chromeos/ime/model sounds
good
> > > to me.  I also agree with Steven's point that we have too many places to
> have
> > > IME-related code.  Shu's idea will solve all the problems, I think.
> > > 
> > > If the name of the new directory will be "model", we might want to have
> > "view",
> > > though.  I don't stick to "model", any better name is welcome.  "model" is
> > okay,
> > > too.
> > 
> > Yuki, can you please confirm whether you agree with Steven's comment to move
> all
> > related classes (especially InputMethodManager which holds instance of
> > ImeKeyboard) to src/ui?
> > For the naming, if InputMethodManager is moving, I would suggest to use
> > "ui/chromeos/ime/controller" instead of "ui/chromeos/ime/model".
> > Or to avoid confusion, move all to "ui/chromeos/ime" and move existing stuff
> to
> > "ui/chromeos/ime/views". SG?
> 
> I support to move all the code in chromeos/ime/ to ui/chromeos/ime/TBD/.  For
> the name,
> "views" is used for Views framework, so please avoid it.  "view" (without s)
is
> probably
> okay.  Seeing contents of chromeos/ime/, "controller" sounds a little weird to
> me.
> It holds a variety of things, not limited to controls.
> 
> I'd prefer "ui/chromeos/ime" and "ui/chromeos/ime/view", because the old
> contents of
> "ui/chromeos/ime" really focus to implement UI views.
> 
> In future, if we'd feel having too many things in ui/chromeos/ime/, then we
can
> create
> more subdirectories and separate the code into components.

Cool, I will revise the CL soon.

[Yuki, 2014-11-18 04:01:00]

Thanks for your patience.
Could you update the header guards, too?

    #ifndef UI_CHROMEOS_IME_...
    #define UI_CHROMEOS_IME_...
    ...
    #endif  // UI_CHROMEOS_IME_...

[Shu Chen, 2014-11-19 08:47:40]

Done the refactoring. Please take another look.

[Shu Chen, 2014-11-19 08:48:54]

shuchen@chromium.org changed reviewers:
+ satorux@chromium.org, sky@chromium.org

[Shu Chen, 2014-11-19 08:48:55]

+satorux@, sky@ for necessary approvals.

[Yuki, 2014-11-19 14:16:43]

Can we add a new DEPS in ui/chromeos/ime/?
We want at least a line "-ui/chromeos/ime/view".
It's great if you can explicitly list up other dependencies, too.

https://codereview.chromium.org/727143002/diff/160001/chromeos/DEPS
File chromeos/DEPS (right):

https://codereview.chromium.org/727143002/diff/160001/chromeos/DEPS#newcode14
chromeos/DEPS:14: # TODO(stevenjb/flackr/shuchen): Remove these dependencies.
Remove your name from the TODO comment.

https://codereview.chromium.org/727143002/diff/160001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/160001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:484: '../ui/gfx/x/gfx_x11.gyp:gfx_x11',
Do we still need these lines?

https://codereview.chromium.org/727143002/diff/160001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:616: '../ui/gfx/x/gfx_x11.gyp:gfx_x11',
Can we remove these lines?

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/DEPS
File ui/chromeos/DEPS (right):

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/DEPS#newcode3
ui/chromeos/DEPS:3: "+chromeos",
Why do we need to rewrite this part?

The old code was more specific and better.  If you happened to need to add more
dependencies, you should list them explicitly.

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:31: 
No need to separate by an empty line.

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:44: 
No need to separate by an empty line.

[sky, 2014-11-19 16:36:42]

On 2014/11/19 08:48:55, Shu Chen wrote:
> +satorux@, sky@ for necessary approvals.

Please be specific, what files do you need me to review?

[Shu Chen, 2014-11-19 16:40:53]

https://codereview.chromium.org/727143002/diff/160001/chromeos/DEPS
File chromeos/DEPS (right):

https://codereview.chromium.org/727143002/diff/160001/chromeos/DEPS#newcode14
chromeos/DEPS:14: # TODO(stevenjb/flackr/shuchen): Remove these dependencies.
On 2014/11/19 14:16:43, Yuki wrote:
> Remove your name from the TODO comment.

Done.

https://codereview.chromium.org/727143002/diff/160001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/160001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:484: '../ui/gfx/x/gfx_x11.gyp:gfx_x11',
On 2014/11/19 14:16:43, Yuki wrote:
> Do we still need these lines?

Done.

https://codereview.chromium.org/727143002/diff/160001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:616: '../ui/gfx/x/gfx_x11.gyp:gfx_x11',
On 2014/11/19 14:16:43, Yuki wrote:
> Can we remove these lines?

Done.

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/DEPS
File ui/chromeos/DEPS (right):

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/DEPS#newcode3
ui/chromeos/DEPS:3: "+chromeos",
On 2014/11/19 14:16:43, Yuki wrote:
> Why do we need to rewrite this part?
> 
> The old code was more specific and better.  If you happened to need to add
more
> dependencies, you should list them explicitly.

Done.

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:31: 
On 2014/11/19 14:16:43, Yuki wrote:
> No need to separate by an empty line.

Done.

https://codereview.chromium.org/727143002/diff/160001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:44: 
On 2014/11/19 14:16:43, Yuki wrote:
> No need to separate by an empty line.

Done.

[Shu Chen, 2014-11-19 16:43:55]

On 2014/11/19 16:36:42, sky wrote:
> On 2014/11/19 08:48:55, Shu Chen wrote:
> > +satorux@, sky@ for necessary approvals.
> 
> Please be specific, what files do you need me to review?

Please review:

ash/...
ui/...
chrome/browser/...

which is almost everything. Thanks!

[sky, 2014-11-19 18:25:31]

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/DEPS
File ui/base/ime/DEPS (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/DEPS#newcode4
ui/base/ime/DEPS:4: "+ui/chromeos/ime",
Doesn't this mean we now have a cycle? ui/chromeos depends upon ui/base and
ui/base/ime depends upon ui/chromeos?

[stevenjb, 2014-11-19 18:57:38]

Thanks a bunch for doing this, I am really much happier with all of the ime code
in src/ui, and I like the model/view separation. We just need to figure out the
ui/base vs. ui/chromeos dependencies.

https://codereview.chromium.org/727143002/diff/200001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/200001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:473: 'ime/input_method.gyp:gencode',
We should document in gen_input_methods.py what it is used for since it does not
appear to be used by anything in the chrome repository?

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_ime_input_context_handler.cc (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_ime_input_context_handler.cc:7: #include
"ui/chromeos/ime/composition_text.h"
composition_text doesn't appear to have any dependencies, maybe it can be moved
here to avoid a circular dependency (since ui/chromeos depends on ui/base)?

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:23: #include
"ui/chromeos/ime/input_method_manager.h"
This is now a circular dependency since ui/chromeos depends on ui/base. I think
you may need to create a Delegate so that the parts that depend on these systems
can live in ui/chromeos.

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/DEPS
File ui/chromeos/DEPS (right):

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/DEPS#newcode4
ui/chromeos/DEPS:4: "+chromeos/ime",
Not needed?

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:110: '../../chromeos/chromeos.gyp:chromeos',
Does ui_chromeos_ime actually depend on chromeos/ now?

[Shu Chen, 2014-11-20 01:04:44]

https://codereview.chromium.org/727143002/diff/200001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/200001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:473: 'ime/input_method.gyp:gencode',
On 2014/11/19 18:57:37, stevenjb wrote:
> We should document in gen_input_methods.py what it is used for since it does
not
> appear to be used by anything in the chrome repository?

Done.
It generates input_methods.h which is used in
ui/chromeos/ime/input_method_whitelist.cc.
input_method_whitelist is used for tests.

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/DEPS
File ui/base/ime/DEPS (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/DEPS#newcode4
ui/base/ime/DEPS:4: "+ui/chromeos/ime",
On 2014/11/19 18:25:30, sky wrote:
> Doesn't this mean we now have a cycle? ui/chromeos depends upon ui/base and
> ui/base/ime depends upon ui/chromeos?

I met the cycle problem and that was why I separated ui_chromeos target to
multiple targets like ui_chromeos_ime, etc.

I think maybe it'd better to move ui/base/ime/input_method_chromeos.* &
ui/base/ime/chromeos/... to ui/chromeos/ime, so that this DEP can be removed.

WDYT?

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_ime_input_context_handler.cc (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_ime_input_context_handler.cc:7: #include
"ui/chromeos/ime/composition_text.h"
On 2014/11/19 18:57:37, stevenjb wrote:
> composition_text doesn't appear to have any dependencies, maybe it can be
moved
> here to avoid a circular dependency (since ui/chromeos depends on ui/base)?

Moving composition_text to here will cause confusion because where is another
composition_text under ui/base/ime.

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:23: #include
"ui/chromeos/ime/input_method_manager.h"
On 2014/11/19 18:57:37, stevenjb wrote:
> This is now a circular dependency since ui/chromeos depends on ui/base. I
think
> you may need to create a Delegate so that the parts that depend on these
systems
> can live in ui/chromeos.

A feasible solution is to moving ui/base/ime/chromeos/... &
ui/base/ime/input_method_chromeos.* to ui/chromeos/ime.

WDYT?

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/DEPS
File ui/chromeos/DEPS (right):

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/DEPS#newcode4
ui/chromeos/DEPS:4: "+chromeos/ime",
On 2014/11/19 18:57:37, stevenjb wrote:
> Not needed?

Need: chromeos/ime/input_methods.h

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/200001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:110: '../../chromeos/chromeos.gyp:chromeos',
On 2014/11/19 18:57:38, stevenjb wrote:
> Does ui_chromeos_ime actually depend on chromeos/ now?

Yes, it depends on the generated header: chromeos/ime/input_methods.h.

[Shu Chen, 2014-11-20 04:36:26]

shuchen@chromium.org changed reviewers:
+ mukai@chromium.org

[Shu Chen, 2014-11-20 04:36:28]

+mukai@ who is the owner for ui/chromeos/ime.

Offline discussed with yukishiino@/nona@ about moving ui/base/ime/chromeos &
ui/base/ime/input_method_chromeos to ui/chromeos/ime, and no concern pops up.

mukai@, do you see any problems for the moving?

Thanks

[Yuki, 2014-11-20 05:16:18]

On 2014/11/20 04:36:28, Shu Chen wrote:
> +mukai@ who is the owner for ui/chromeos/ime.
> 
> Offline discussed with yukishiino@/nona@ about moving ui/base/ime/chromeos &
> ui/base/ime/input_method_chromeos to ui/chromeos/ime, and no concern pops up.
> 
> mukai@, do you see any problems for the moving?
> 
> Thanks

IIUC, nona@'s idea is move the old contents of chromeos/ime/ to
ui/base/ime/chromeos/.  In this case, it's unlikely we will face to DEPS issues.
 Since ui/base/ime/ has stricter restrictions on dependency, I think it'd be
better to put the code into ui/base/ime/chromeos/, not in ui/chromeos/ime/.

As I heard from nona@, chromeos/ime/ was used to support IBus-based IMEs, and
this is the reason why we have had chromeos/ime/ and ui/base/ime/chromeos/
separated.  Now we no longer support IBus-based IMEs, we no longer need
chromeos/ime/, and it should be okay to merge chromeos/ime/ into
ui/base/ime/chromeos/.

My understanding is:
plan A) move chromeos/ime/ into ui/base/ime/chromeos/
ui/base/ already depends on ui/gfx/, so the original dependency issue will be
solved.

plan B) move chromeos/ime/ and ui/base/ime/chromeos/ into ui/chromeos/ime/
ui/base/ime/input_method_chromeos.{h,cc} cannot depend on ui/chromeos/ime/.
We have non-CrOS IME code in ui/base/ime/, especially ui::InputMethod and
ui::TextInputClient, and moving input_method_chromeos.{h,cc} into
ui/chromeos/ime/ doesn't make sense to me.
ui::InputMethodChromeOS has a strong dependency to
ui/base/ime/chromeos/character_composer.h, etc., so it'd be hard to go with this
plan.

I'd recommend plan A, as long as a dependency from ui/base/ to ui/gfx/ is okay.

[Shu Chen, 2014-11-20 06:16:46]

On 2014/11/20 05:16:18, Yuki wrote:
> On 2014/11/20 04:36:28, Shu Chen wrote:
> > +mukai@ who is the owner for ui/chromeos/ime.
> > 
> > Offline discussed with yukishiino@/nona@ about moving ui/base/ime/chromeos &
> > ui/base/ime/input_method_chromeos to ui/chromeos/ime, and no concern pops
up.
> > 
> > mukai@, do you see any problems for the moving?
> > 
> > Thanks
> 
> IIUC, nona@'s idea is move the old contents of chromeos/ime/ to
> ui/base/ime/chromeos/.  In this case, it's unlikely we will face to DEPS
issues.
>  Since ui/base/ime/ has stricter restrictions on dependency, I think it'd be
> better to put the code into ui/base/ime/chromeos/, not in ui/chromeos/ime/.
> 
> As I heard from nona@, chromeos/ime/ was used to support IBus-based IMEs, and
> this is the reason why we have had chromeos/ime/ and ui/base/ime/chromeos/
> separated.  Now we no longer support IBus-based IMEs, we no longer need
> chromeos/ime/, and it should be okay to merge chromeos/ime/ into
> ui/base/ime/chromeos/.
> 
> My understanding is:
> plan A) move chromeos/ime/ into ui/base/ime/chromeos/
> ui/base/ already depends on ui/gfx/, so the original dependency issue will be
> solved.
> 
> plan B) move chromeos/ime/ and ui/base/ime/chromeos/ into ui/chromeos/ime/
> ui/base/ime/input_method_chromeos.{h,cc} cannot depend on ui/chromeos/ime/.
> We have non-CrOS IME code in ui/base/ime/, especially ui::InputMethod and
> ui::TextInputClient, and moving input_method_chromeos.{h,cc} into
> ui/chromeos/ime/ doesn't make sense to me.
> ui::InputMethodChromeOS has a strong dependency to
> ui/base/ime/chromeos/character_composer.h, etc., so it'd be hard to go with
this
> plan.
> 
> I'd recommend plan A, as long as a dependency from ui/base/ to ui/gfx/ is
okay.

I think plan A) is the best solution so far because it aligns the original
design intention.

Will do a quick revise to shorten the review cycle cross PEK/MTV.

Thanks!

[Jun Mukai, 2014-11-20 07:34:14]

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/BUILD.gn
File ui/chromeos/BUILD.gn (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/BUILD.gn#ne...
ui/chromeos/BUILD.gn:16: "//ui/chromeos:ui_chromeos_ime_view",
can't find this target.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/BUILD.gn#ne...
ui/chromeos/BUILD.gn:27: component("ui_chromeos_ime") {
If target is split and all files are under ime/, you could rather create
ui/chromeos/ime/BUILD.gn and name the target as just 'ime'.
Then, dependencies would simply become "//ui/chromeos/ime"

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
File ui/chromeos/ime/ime_keyboard_ozone.cc (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
ui/chromeos/ime/ime_keyboard_ozone.cc:56: UI_CHROMEOS_EXPORT ImeKeyboard*
ImeKeyboard::Create() {
I don't think you have to specify _EXPORT macro in cc file. It's already
declared as exporting in ime_keyboard.h

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
File ui/chromeos/ime/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
ui/chromeos/ime/ime_keyboard_x11.cc:281: UI_CHROMEOS_EXPORT bool
ImeKeyboard::GetAutoRepeatEnabledForTesting() {
same

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/view/ca...
File ui/chromeos/ime/view/candidate_view.h (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/view/ca...
ui/chromeos/ime/view/candidate_view.h:5: #ifndef ASH_IME_CANDIDATE_VIEW_H_
Update the macro name to match to the path name (oops, this is way too old by
the way...)

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/view/ca...
ui/chromeos/ime/view/candidate_view.h:16: namespace ime {
Just wondering: is it possibly in ::chromeos::ime namespace instead?

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:32: 'ime/view/candidate_view.cc',
"views" (prural) is the name for our UI framework, so I prefer that for the
directory name. That's also widely used (like "ui/views", "ui/app_list/views",
"chrome/browser/ui/views" etc)

[Yuki, 2014-11-20 07:41:13]

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:32: 'ime/view/candidate_view.cc',
On 2014/11/20 07:34:14, Jun Mukai wrote:
> "views" (prural) is the name for our UI framework, so I prefer that for the
> directory name. That's also widely used (like "ui/views", "ui/app_list/views",
> "chrome/browser/ui/views" etc)

We know Views, but candidate_view.cc come from the class name
ui::ime::CandidateView, so it's natural to be in singular form.

For the directory name, "views" makes sense.

To Shu, I've now reallized all these files implement views::View.  Then, it's
okay to rename the directory to "views", "ime_views" or something like that.

[Shu Chen, 2014-11-20 07:49:59]

On 2014/11/20 07:41:13, Yuki wrote:
>
https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
> File ui/chromeos/ui_chromeos.gyp (right):
> 
>
https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
> ui/chromeos/ui_chromeos.gyp:32: 'ime/view/candidate_view.cc',
> On 2014/11/20 07:34:14, Jun Mukai wrote:
> > "views" (prural) is the name for our UI framework, so I prefer that for the
> > directory name. That's also widely used (like "ui/views",
"ui/app_list/views",
> > "chrome/browser/ui/views" etc)
> 
> We know Views, but candidate_view.cc come from the class name
> ui::ime::CandidateView, so it's natural to be in singular form.
> 
> For the directory name, "views" makes sense.
> 
> To Shu, I've now reallized all these files implement views::View.  Then, it's
> okay to rename the directory to "views", "ime_views" or something like that.

Isn't moving chromeos/ime to ui/base/ime/chromeos a better solution?

[Jun Mukai, 2014-11-20 07:50:33]

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:32: 'ime/view/candidate_view.cc',
On 2014/11/20 07:41:13, Yuki wrote:
> On 2014/11/20 07:34:14, Jun Mukai wrote:
> > "views" (prural) is the name for our UI framework, so I prefer that for the
> > directory name. That's also widely used (like "ui/views",
"ui/app_list/views",
> > "chrome/browser/ui/views" etc)
> 
> We know Views, but candidate_view.cc come from the class name
> ui::ime::CandidateView, so it's natural to be in singular form.
> 
> For the directory name, "views" makes sense.
> 
> To Shu, I've now reallized all these files implement views::View.  Then, it's
> okay to rename the directory to "views", "ime_views" or something like that.

Yeah, of course each class file should be suffixed by "_view.h" in that case.
That follows the convention like "ui/views/view.h" . I just cared about the
directory name.

[Shu Chen, 2014-11-20 08:07:45]

Guys, can you please take another look at the latest patchset?

I've changed the solution to move chromeos/ime to ui/base/ime/chromeos.
Some of your comments may be invalid for such new change.

As for naming issue of ui/chromeos/ime/view, it can be done by a separated cl.
This cl no longer such ui/chromeos/ime directory names and GYPs.

Thanks!

[Jun Mukai, 2014-11-20 08:25:07]

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/co...
File ui/base/ime/chromeos/component_extension_ime_manager.h (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/co...
ui/base/ime/chromeos/component_extension_ime_manager.h:6: #define
UI_CHROMEOS_IME_COMPONENT_EXTENSION_IME_MANAGER_H_
Please update the macro name as UI_BASE_IME_CHROMEOS_...
same for other files.

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_ozone.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_ozone.cc:56: UI_BASE_EXPORT ImeKeyboard*
ImeKeyboard::Create() {
copy from my comment in another patchset: I think UI_BASE_EXPORT is not
necessary here. Is it?

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_x11.cc:281: UI_BASE_EXPORT bool
ImeKeyboard::GetAutoRepeatEnabledForTesting() {
Similarly: see my comment on ime_keyboard_ozone.cc

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
File ui/chromeos/ime/candidate_view_unittest.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_view_unittest.cc:5: #include
"ui/chromeos/ime//candidate_view.h"
nit: remove this

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
File ui/chromeos/ime/candidate_window_view.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view.cc:5: #include
"ui/chromeos/ime//candidate_window_view.h"
and this

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
File ui/chromeos/ime/candidate_window_view_unittest.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view_unittest.cc:5: #include
"ui/chromeos/ime//candidate_window_view.h"
same

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view_unittest.cc:12: #include
"ui/chromeos/ime//candidate_view.h"
same

[Shu Chen, 2014-11-20 08:34:39]

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/BUILD.gn
File ui/chromeos/BUILD.gn (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/BUILD.gn#ne...
ui/chromeos/BUILD.gn:16: "//ui/chromeos:ui_chromeos_ime_view",
On 2014/11/20 07:34:14, Jun Mukai wrote:
> can't find this target.

Done. New solution no longer such this file.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/BUILD.gn#ne...
ui/chromeos/BUILD.gn:27: component("ui_chromeos_ime") {
On 2014/11/20 07:34:14, Jun Mukai wrote:
> If target is split and all files are under ime/, you could rather create
> ui/chromeos/ime/BUILD.gn and name the target as just 'ime'.
> Then, dependencies would simply become "//ui/chromeos/ime"

Done. New solution no longer such this file.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
File ui/chromeos/ime/ime_keyboard_ozone.cc (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
ui/chromeos/ime/ime_keyboard_ozone.cc:56: UI_CHROMEOS_EXPORT ImeKeyboard*
ImeKeyboard::Create() {
On 2014/11/20 07:34:14, Jun Mukai wrote:
> I don't think you have to specify _EXPORT macro in cc file. It's already
> declared as exporting in ime_keyboard.h

Done.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
File ui/chromeos/ime/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/ime_key...
ui/chromeos/ime/ime_keyboard_x11.cc:281: UI_CHROMEOS_EXPORT bool
ImeKeyboard::GetAutoRepeatEnabledForTesting() {
On 2014/11/20 07:34:14, Jun Mukai wrote:
> same

Done.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/view/ca...
File ui/chromeos/ime/view/candidate_view.h (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/view/ca...
ui/chromeos/ime/view/candidate_view.h:5: #ifndef ASH_IME_CANDIDATE_VIEW_H_
On 2014/11/20 07:34:14, Jun Mukai wrote:
> Update the macro name to match to the path name (oops, this is way too old by
> the way...)

Done.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ime/view/ca...
ui/chromeos/ime/view/candidate_view.h:16: namespace ime {
On 2014/11/20 07:34:14, Jun Mukai wrote:
> Just wondering: is it possibly in ::chromeos::ime namespace instead?

Let's do this in a separated cl.

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
File ui/chromeos/ui_chromeos.gyp (right):

https://codereview.chromium.org/727143002/diff/220001/ui/chromeos/ui_chromeos...
ui/chromeos/ui_chromeos.gyp:32: 'ime/view/candidate_view.cc',
On 2014/11/20 07:50:33, Jun Mukai wrote:
> On 2014/11/20 07:41:13, Yuki wrote:
> > On 2014/11/20 07:34:14, Jun Mukai wrote:
> > > "views" (prural) is the name for our UI framework, so I prefer that for
the
> > > directory name. That's also widely used (like "ui/views",
> "ui/app_list/views",
> > > "chrome/browser/ui/views" etc)
> > 
> > We know Views, but candidate_view.cc come from the class name
> > ui::ime::CandidateView, so it's natural to be in singular form.
> > 
> > For the directory name, "views" makes sense.
> > 
> > To Shu, I've now reallized all these files implement views::View.  Then,
it's
> > okay to rename the directory to "views", "ime_views" or something like that.
> 
> Yeah, of course each class file should be suffixed by "_view.h" in that case.
> That follows the convention like "ui/views/view.h" . I just cared about the
> directory name.

The new change no longer touch this file.

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/co...
File ui/base/ime/chromeos/component_extension_ime_manager.h (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/co...
ui/base/ime/chromeos/component_extension_ime_manager.h:6: #define
UI_CHROMEOS_IME_COMPONENT_EXTENSION_IME_MANAGER_H_
On 2014/11/20 08:25:06, Jun Mukai wrote:
> Please update the macro name as UI_BASE_IME_CHROMEOS_...
> same for other files.

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_ozone.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_ozone.cc:56: UI_BASE_EXPORT ImeKeyboard*
ImeKeyboard::Create() {
On 2014/11/20 08:25:06, Jun Mukai wrote:
> copy from my comment in another patchset: I think UI_BASE_EXPORT is not
> necessary here. Is it?

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_x11.cc:281: UI_BASE_EXPORT bool
ImeKeyboard::GetAutoRepeatEnabledForTesting() {
On 2014/11/20 08:25:06, Jun Mukai wrote:
> Similarly: see my comment on ime_keyboard_ozone.cc

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
File ui/chromeos/ime/candidate_view_unittest.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_view_unittest.cc:5: #include
"ui/chromeos/ime//candidate_view.h"
On 2014/11/20 08:25:06, Jun Mukai wrote:
> nit: remove this

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
File ui/chromeos/ime/candidate_window_view.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view.cc:5: #include
"ui/chromeos/ime//candidate_window_view.h"
On 2014/11/20 08:25:07, Jun Mukai wrote:
> and this

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
File ui/chromeos/ime/candidate_window_view_unittest.cc (right):

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view_unittest.cc:5: #include
"ui/chromeos/ime//candidate_window_view.h"
On 2014/11/20 08:25:07, Jun Mukai wrote:
> same

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view_unittest.cc:5: #include
"ui/chromeos/ime//candidate_window_view.h"
On 2014/11/20 08:25:07, Jun Mukai wrote:
> same

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view_unittest.cc:12: #include
"ui/chromeos/ime//candidate_view.h"
On 2014/11/20 08:25:07, Jun Mukai wrote:
> same

Done.

https://codereview.chromium.org/727143002/diff/240001/ui/chromeos/ime/candida...
ui/chromeos/ime/candidate_window_view_unittest.cc:12: #include
"ui/chromeos/ime//candidate_view.h"
On 2014/11/20 08:25:07, Jun Mukai wrote:
> same

Done.

[Jun Mukai, 2014-11-20 08:40:06]

lgtm: ui/base/ime/chromeos, ui/chromeos/ime

[Yuki, 2014-11-20 09:23:03]

lgtm.  Thanks for managing this movement.

https://codereview.chromium.org/727143002/diff/240001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/BUILD.gn#newcod...
ui/base/BUILD.gn:327: "ime/chromeos/component_extension_ime_manager.cc",
nit: Could you sort files in alphabetical order?

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_x11.cc:5: #include
"ui/base/ime/chromeos/ime_keyboard_x11.h"
#include ime_keyboard_x11.h  // The corresponding header

#include <X11/foo.h>  // system headers

#include "ui/gfx/x/x11_types.h"  // our other headers

Note that there is an empty line to separate blocks.

and the comment seems not applying for now.
At a glance, we have nothing to conflict in base/tracked_obejcts.h.
Probably it's obsolete?

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.cc
(right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.cc:8:
#include "ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.h"
Put the corresponding header first with an empty line separator.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:204: 'ime/chromeos/component_extension_ime_manager.cc',
Could you sort the files?

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:681: 'ime/chromeos/character_composer.cc',
filename_rules.gypi automatically excludes chromeos/ directory, so you don't
need to list these files.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:714: 'ime/input_method_chromeos.cc',
Actually the files with the suffix "_chromeos.{h,cc}" are also automatically
excluded by filename_rules.gypi.

We don't need this whole section.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:792: 'ime/chromeos/fake_input_method_delegate.cc',
Since chromeos/* are automatically excluded, you can list these files directly
in 'sources' section.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base_tests.gypi
File ui/base/ui_base_tests.gypi (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base_tests.g...
ui/base/ui_base_tests.gypi:203:
'ime/chromeos/component_extension_ime_manager_unittest.cc',
The TODO comment doesn't apply to your new files.

And you can put these files in 'all_sources', which already includes 'cocoa/...'
for example.

I'm on the fence if we should move the old ../chromeos/... files to which the
TODO comment applies.

[Shu Chen, 2014-11-20 14:32:27]

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_x11.cc:5: #include
"ui/base/ime/chromeos/ime_keyboard_x11.h"
On 2014/11/20 09:23:03, Yuki wrote:
> #include ime_keyboard_x11.h  // The corresponding header
> 
> #include <X11/foo.h>  // system headers
> 
> #include "ui/gfx/x/x11_types.h"  // our other headers
> 
> Note that there is an empty line to separate blocks.
> 
> and the comment seems not applying for now.
> At a glance, we have nothing to conflict in base/tracked_obejcts.h.
> Probably it's obsolete?

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.cc
(right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.cc:8:
#include "ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.h"
On 2014/11/20 09:23:03, Yuki wrote:
> Put the corresponding header first with an empty line separator.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:204: 'ime/chromeos/component_extension_ime_manager.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> Could you sort the files?

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:681: 'ime/chromeos/character_composer.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> filename_rules.gypi automatically excludes chromeos/ directory, so you don't
> need to list these files.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:714: 'ime/input_method_chromeos.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> Actually the files with the suffix "_chromeos.{h,cc}" are also automatically
> excluded by filename_rules.gypi.
> 
> We don't need this whole section.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:792: 'ime/chromeos/fake_input_method_delegate.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> Since chromeos/* are automatically excluded, you can list these files directly
> in 'sources' section.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base_tests.gypi
File ui/base/ui_base_tests.gypi (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base_tests.g...
ui/base/ui_base_tests.gypi:203:
'ime/chromeos/component_extension_ime_manager_unittest.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> The TODO comment doesn't apply to your new files.
> 
> And you can put these files in 'all_sources', which already includes
'cocoa/...'
> for example.
> 
> I'm on the fence if we should move the old ../chromeos/... files to which the
> TODO comment applies.

Done.

[Shu Chen, 2014-11-20 14:32:27]

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_keyboard_x11.cc (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_keyboard_x11.cc:5: #include
"ui/base/ime/chromeos/ime_keyboard_x11.h"
On 2014/11/20 09:23:03, Yuki wrote:
> #include ime_keyboard_x11.h  // The corresponding header
> 
> #include <X11/foo.h>  // system headers
> 
> #include "ui/gfx/x/x11_types.h"  // our other headers
> 
> Note that there is an empty line to separate blocks.
> 
> and the comment seems not applying for now.
> At a glance, we have nothing to conflict in base/tracked_obejcts.h.
> Probably it's obsolete?

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.cc
(right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.cc:8:
#include "ui/base/ime/chromeos/mock_component_extension_ime_manager_delegate.h"
On 2014/11/20 09:23:03, Yuki wrote:
> Put the corresponding header first with an empty line separator.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:204: 'ime/chromeos/component_extension_ime_manager.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> Could you sort the files?

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:681: 'ime/chromeos/character_composer.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> filename_rules.gypi automatically excludes chromeos/ directory, so you don't
> need to list these files.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:714: 'ime/input_method_chromeos.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> Actually the files with the suffix "_chromeos.{h,cc}" are also automatically
> excluded by filename_rules.gypi.
> 
> We don't need this whole section.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:792: 'ime/chromeos/fake_input_method_delegate.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> Since chromeos/* are automatically excluded, you can list these files directly
> in 'sources' section.

Done.

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base_tests.gypi
File ui/base/ui_base_tests.gypi (right):

https://codereview.chromium.org/727143002/diff/260001/ui/base/ui_base_tests.g...
ui/base/ui_base_tests.gypi:203:
'ime/chromeos/component_extension_ime_manager_unittest.cc',
On 2014/11/20 09:23:03, Yuki wrote:
> The TODO comment doesn't apply to your new files.
> 
> And you can put these files in 'all_sources', which already includes
'cocoa/...'
> for example.
> 
> I'm on the fence if we should move the old ../chromeos/... files to which the
> TODO comment applies.

Done.

[Shu Chen, 2014-11-20 14:46:35]

sky@/stevenjb@, can you please take another look? Thanks!

Changes in below files need your approval:

stevenjb:
  ash/system/chromeos/tray_caps_lock.cc
  ash/system/chromeos/tray_caps_lock.h
  chrome/browser/chromeos/accessibility/accessibility_manager.cc
  chrome/browser/chromeos/accessibility/accessibility_manager.h
  chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
  chrome/browser/chromeos/base/locale_util.cc
  chrome/browser/chromeos/chrome_browser_main_chromeos.cc
  chrome/browser/chromeos/events/event_rewriter.cc
  chrome/browser/chromeos/events/event_rewriter_unittest.cc
  chrome/browser/chromeos/events/system_key_event_listener.cc
  chrome/browser/chromeos/events/xinput_hierarchy_changed_event_listener.cc
  chrome/browser/chromeos/extensions/input_view_browsertest.cc
  chrome/browser/chromeos/login/lock/screen_locker.h
  chrome/browser/chromeos/login/oobe_localization_browsertest.cc
  chrome/browser/chromeos/login/session/user_session_manager.cc
  chrome/browser/chromeos/login/session/user_session_manager.h
  chrome/browser/chromeos/login/ui/login_display_host_impl.cc
  chrome/browser/chromeos/login/ui/user_adding_screen_input_methods_controller.h
  chrome/browser/chromeos/login/ui/webui_login_display.cc
  chrome/browser/chromeos/policy/device_local_account_browsertest.cc
  chrome/browser/chromeos/preferences.cc
  chrome/browser/chromeos/preferences.h
  chrome/browser/chromeos/preferences_browsertest.cc
  chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
  chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
  chrome/browser/ui/webui/options/chromeos/keyboard_handler.cc
  chromeos/BUILD.gn
  chromeos/DEPS
sky:
  ash/accelerators/accelerator_controller.cc
  chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc
  chrome/browser/ui/ash/ime_controller_chromeos.cc
  chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
  chrome/browser/ui/ash/system_tray_delegate_chromeos.h
  chrome/browser/ui/views/omnibox/omnibox_view_views.h
  chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc
  chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc
  chrome/browser/ui/webui/chromeos/login/l10n_util.cc
  chrome/browser/ui/webui/chromeos/login/l10n_util_test_util.h
  chrome/browser/ui/webui/chromeos/login/l10n_util_unittest.cc
  chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc
  chrome/browser/ui/webui/chromeos/login/network_screen_handler.h
  chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
  chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h
  chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc
  ui/base/BUILD.gn
  ui/base/ui_base.gyp
  ui/base/ui_base_tests.gypi

[Yuki, 2014-11-20 14:52:16]

https://codereview.chromium.org/727143002/diff/240001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/BUILD.gn#newcod...
ui/base/BUILD.gn:327: "ime/chromeos/component_extension_ime_manager.cc",
On 2014/11/20 09:23:03, Yuki wrote:
> nit: Could you sort files in alphabetical order?

It seems this hasn't been got done yet.

[sky, 2014-11-20 15:20:01]

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/DEPS
File ui/base/ime/DEPS (right):

https://codereview.chromium.org/727143002/diff/200001/ui/base/ime/DEPS#newcode4
ui/base/ime/DEPS:4: "+ui/chromeos/ime",
On 2014/11/20 01:04:44, Shu Chen wrote:
> On 2014/11/19 18:25:30, sky wrote:
> > Doesn't this mean we now have a cycle? ui/chromeos depends upon ui/base and
> > ui/base/ime depends upon ui/chromeos?
> 
> I met the cycle problem and that was why I separated ui_chromeos target to
> multiple targets like ui_chromeos_ime, etc.
> 
> I think maybe it'd better to move ui/base/ime/input_method_chromeos.* &
> ui/base/ime/chromeos/... to ui/chromeos/ime, so that this DEP can be removed.
> 
> WDYT?

I like it. As long as there are no circles I'm happy.

[Shu Chen, 2014-11-20 15:21:40]

https://codereview.chromium.org/727143002/diff/240001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/727143002/diff/240001/ui/base/BUILD.gn#newcod...
ui/base/BUILD.gn:327: "ime/chromeos/component_extension_ime_manager.cc",
On 2014/11/20 14:52:16, Yuki wrote:
> On 2014/11/20 09:23:03, Yuki wrote:
> > nit: Could you sort files in alphabetical order?
> 
> It seems this hasn't been got done yet.

Done.

[sky, 2014-11-20 16:20:15]

It looks like chromeos depends upon ui/gfx/x ui/gfx may depend upon ui/base,
which means a cycle. Can you remove the ui/base dependency from ui/gfx/x?

[stevenjb, 2014-11-20 19:09:52]

I like this better. lgtm.

https://codereview.chromium.org/727143002/diff/300001/ui/base/ime/chromeos/co...
File ui/base/ime/chromeos/composition_text.h (right):

https://codereview.chromium.org/727143002/diff/300001/ui/base/ime/chromeos/co...
ui/base/ime/chromeos/composition_text.h:6: #define
UI_BASE_IME_CHROMEOS_COMPOSITION_TEXT_H_
Since as you mention there is already a ui/base/ime/composition_text.h, I think
this should be renamed composition_text_chromeos.h (and .cc, _unittest.cc).
Personally I would also rename the class to ChompositionTextChromeos even though
it is in the chromeos namespace, just to help with searchability, but that is
entirely optional.

https://codereview.chromium.org/727143002/diff/300001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/727143002/diff/300001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:231: 'ime/input_method_chromeos.h',
This is no longer sorted, and these should maybe be moved to
ui/base/ime/chromeos/?

[stevenjb, 2014-11-20 19:12:10]

On 2014/11/20 16:20:15, sky wrote:
> It looks like chromeos depends upon ui/gfx/x ui/gfx may depend upon ui/base,
> which means a cycle. Can you remove the ui/base dependency from ui/gfx/x?

sky@ - it looks to me that ui/gfx depends on src/base, but not src/ui/base,
except for ui/gfx/test/run_all_unittests.cc, which should be OK for now? (I seem
to recall something about the test deps being cleaned up separately?)

[sky, 2014-11-20 20:50:52]

You're right. I misread +base as +ui/base.

On Thu, Nov 20, 2014 at 11:12 AM,  <stevenjb@chromium.org> wrote:
> On 2014/11/20 16:20:15, sky wrote:
>>
>> It looks like chromeos depends upon ui/gfx/x ui/gfx may depend upon
>> ui/base,
>> which means a cycle. Can you remove the ui/base dependency from ui/gfx/x?
>
>
> sky@ - it looks to me that ui/gfx depends on src/base, but not src/ui/base,
> except for ui/gfx/test/run_all_unittests.cc, which should be OK for now? (I
> seem
> to recall something about the test deps being cleaned up separately?)
>
>
>
> https://codereview.chromium.org/727143002/

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

[Shu Chen, 2014-11-25 03:02:24]

Pinging... sky@, may I get your approval? Thanks

[Shu Chen, 2014-11-26 02:11:39]

shuchen@chromium.org changed reviewers:
+ jamescook@chromium.org, pkasting@chromium.org
- satorux@chromium.org

[Shu Chen, 2014-11-26 02:11:42]

+jamescook@ for Ash related stuff:
  ash/accelerators/accelerator_controller.cc
  chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc
  chrome/browser/ui/ash/ime_controller_chromeos.cc
  chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
  chrome/browser/ui/ash/system_tray_delegate_chromeos.h

+pkasting@ for chrome/browser/ui/....

Thanks!

[Shu Chen, 2014-11-26 02:16:28]

shuchen@chromium.org changed reviewers:
+ derat@chromium.org
- jamescook@chromium.org

[Shu Chen, 2014-11-26 02:16:30]

Replacing jamescook@ (who is OOO) with derat@, for Ash related stuff.

[Daniel Erat, 2014-11-26 14:09:51]

i think i'm even more ooo than james, but lgtm for ash (feel free to tbr header
path updates in the future)

[Peter Kasting, 2014-11-26 22:17:03]

LGTM.  For purely-mechanical changes like those in c/b/ui, I suggest getting one
high-level codebase owner to approve the whole CL or else TBRing specific
owners.

[Shu Chen, 2014-11-27 06:05:57]

Thank you for the reviews & advices.

I will do TBR in the future for CLs like this.

[Shu Chen, 2014-11-27 06:11:18]

https://codereview.chromium.org/727143002/diff/300001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/727143002/diff/300001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:231: 'ime/input_method_chromeos.h',
On 2014/11/20 19:09:52, stevenjb wrote:
> This is no longer sorted, and these should maybe be moved to
> ui/base/ime/chromeos/?

Done for sorting. For renaming, let's do it in a separated cl.

[Shu Chen, 2014-11-27 06:12:06]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-11-27 06:13:07]

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

[commit-bot: I haz the power, 2014-11-27 06:18:38]

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

[commit-bot: I haz the power, 2014-11-27 06:18:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Shu Chen, 2014-11-27 06:53:00]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-11-27 06:53:16]

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

[commit-bot: I haz the power, 2014-11-27 07:07:29]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-11-27 07:08:17]

Patchset 18 (id:??) landed as
https://crrev.com/9917a3671cafa9d31a071a530b9ce10ce0661bc9
Cr-Commit-Position: refs/heads/master@{#305964}

[jochen (gone - plz use gerrit), 2014-11-27 07:57:33]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/750353004/ by jochen@chromium.org.

The reason for reverting is: fails to compile on Linux ChromiumOS GN

clang: error: no such file or directory:
'../../chromeos/ime/mock_component_extension_ime_manager_delegate.cc'
.