Add build dependency: ui/app_list uses ui/base/ime

Add build dependency: ui/app_list uses ui/base/ime

The code in ui/app_list/views/search_box_view.cc was importing
ui/base/ime/text_input_flags.h, which was violating a dependency,
resulting in a compile problem for a downstream project. Fixed by
allowing this dependency.

R=sadrul@chromium.org,mgiuca@chromium.org

Committed: https://crrev.com/d239049c3533c38961166a820fd5cc4762e33555
Cr-Commit-Position: refs/heads/master@{#328506}

[Daniel Bratell, 2015-05-04 15:47:22]

bratell@opera.com changed reviewers:
+ sadrul@chromium.org

[Daniel Bratell, 2015-05-04 15:47:22]

sadrul, can you please take a look at this compile fix?

[sadrul, 2015-05-04 17:01:41]

lgtm

[Matt Giuca, 2015-05-05 00:41:52]

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

[Matt Giuca, 2015-05-05 00:41:53]

What compile is broken?

I can see that search_box_view.cc includes "ui/base/ime/text_input_flags.h", so
I agree that this is the correct change, but I'd like to document exactly what
was broken and what is being fixed.

Please file a bug describing the problem, and link to it from the CL
description, before submitting.

[Daniel Bratell, 2015-05-05 08:51:02]

On 2015/05/05 00:41:53, Matt Giuca wrote:
> What compile is broken?
> 
> I can see that search_box_view.cc includes "ui/base/ime/text_input_flags.h",
so
> I agree that this is the correct change, but I'd like to document exactly what
> was broken and what is being fixed.
> 
> Please file a bug describing the problem, and link to it from the CL
> description, before submitting.

It was an Opera build that broke during a regular sync with the upstream code so
I am afraid that I do not have anything to link. Typically (this is not an
uncommon problem) it's because we've disabled or replaced a subsystem that would
have indirectly added the necessary dependencies or would have caused the files
to compile in the right order just by "luck". I tried to reproduce it locally
now by reverting the patch but since I don't know if it was a timing issue or
exactly what product it was I've not been successful. I will do a few more
experiments but if you can take this on "it's the right thing to do" I would
really appreciate it.

[tfarina, 2015-05-05 13:27:11]

Although the dependency seems right, it seems this just compiles fine without it
because the type used from ui/base/ime is just an enum, and thus when you pass
it to SearchBox it is just an int, and you don't need any dependency for that
int to work or compile. But I'm happy to be proven wrong though.

[Daniel Bratell, 2015-05-05 13:32:11]

On 2015/05/05 13:27:11, tfarina wrote:
> Although the dependency seems right, it seems this just compiles fine without
it
> because the type used from ui/base/ime is just an enum, and thus when you pass
> it to SearchBox it is just an int, and you don't need any dependency for that
> int to work or compile. But I'm happy to be proven wrong though.

I have no more information really. This patch fixed a problem that I can no
longer reproduce in any way. If you want to I can drop it.

[Matt Giuca, 2015-05-06 00:15:54]

Hi Daniel (sorry for the time delay; I'm in Sydney which is presumably a
different time zone to you).

Sorry if I came off as stand-offish. I wasn't asking for proof, just to put some
details in the change description (or a linked bug) so that if someone stumbles
upon this, the rationale is explained and they don't have to dig into the text
of this conversation. If you can no longer reproduce it, but it's just "the
right thing to do", then I don't think we need to file a bug. Just add some
extra text to the CL description, like this:

"The code in ui/app_list/views/search_box_view.cc was importing
ui/base/ime/text_input_flags.h, which was violating a dependency, resulting in a
compile failure for a downstream project. Fixed by allowing this dependency."

Feel free to reword, then land. lgtm. Thanks!

[tapted, 2015-05-06 00:34:35]

Note that ui/base/ime/text_input_flags.h doesn't actually appear in any gyp or
gn file... which is probably a separate bug, and may be behind a flaky
dependency issue.

Perhaps gn would complain about a bad header dependency in app_list if it was
added, but I can't seem to figure out the incantation to make it upset.

[tfarina, 2015-05-06 01:00:50]

On Tue, May 5, 2015 at 9:34 PM, <tapted@chromium.org> wrote:

> Note that ui/base/ime/text_input_flags.h doesn't actually appear in any
> gyp or
> gn file... which is probably a separate bug, and may be behind a flaky
> dependency issue.
>
> Perhaps gn would complain about a bad header dependency in app_list if it
> was
> added, but I can't seem to figure out the incantation to make it upset.
>
> It would appear if ui/base/ime/text_input_flags.h were included from
ui/app_list.


> https://codereview.chromium.org/1127603002/
>



-- 
Thiago Farina

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

[Daniel Bratell, 2015-05-06 07:48:06]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2015-05-06 07:48:24]

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

[Daniel Bratell, 2015-05-06 07:49:50]

Oh, no worries. It was reasonable to ask for details to learn more. If I came
across as frustrated it was probably true but I was frustrated at not being able
to provide the data, not at you for asking for it.

If I had to review this patch I would most likely have asked the exact same
question, to learn more about what had gone wrong.

Thanks for the reviews!

[commit-bot: I haz the power, 2015-05-06 08:47:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-06 08:48:00]

Patchset 1 (id:??) landed as
https://crrev.com/d239049c3533c38961166a820fd5cc4762e33555
Cr-Commit-Position: refs/heads/master@{#328506}