Upstream iOS specific design for omnibox

Upstream iOS specific design for omnibox

On iOS the omnibox display a different icon for result coming from "history",
"search" and others sources, including different colors when in Incognito
mode.

Add iOS specific resources and introduce a variation in TypeToIcon() method
to allow distinguishing result coming from "history" from the rest.

BUG=446930
Committed: https://crrev.com/6401246730331a555a3a04f4aff7ae86117ca060
Cr-Commit-Position: refs/heads/master@{#311559}

[sdefresne, 2015-01-12 17:48:25]

sdefresne@chromium.org changed reviewers:
+ erikwright@chromium.org

[sdefresne, 2015-01-12 17:48:25]

erikwright: please take a look

bdibello: FYI

[sdefresne, 2015-01-12 17:49:44]

sdefresne@chromium.org changed reviewers:
+ mpearson@chromium.org

[sdefresne, 2015-01-12 17:49:44]

erikwright: please take a look at components/resources
mpearson: please take a look at components/omnibox

[Mark P, 2015-01-12 18:44:29]

components/omnibox/autocomplete_match.cc lgtm

That said, I find the history distinctions odd.  Why is a suggest that the user
has visited before that's coming from the local device marked history but the
same suggestion if provided by the server (which knows the user has visited
before) (NAVSUGGEST_PERSONALIZED) not history?  And why are bookmarks not
history?  Presumably the user must've visited them at some point (on some
device)?  And so on (there are few others that I'm surprised by).

--mark

[sdefresne, 2015-01-12 18:46:35]

bdibello: as the original author of the downstream change, can you answer the
question from mpearson?

[bdibello, 2015-01-12 19:23:51]

> That said, I find the history distinctions odd.  Why is a suggest that the
user
> has visited before that's coming from the local device marked history but the
> same suggestion if provided by the server (which knows the user has visited
> before) (NAVSUGGEST_PERSONALIZED) not history?

The description of NAVSUGGEST_PERSONALIZED is "A personalized suggestion URL",
which doesn't imply it's in the user's history. I'd assume that any suggestion
that was also in the user's history would be returned as one of the HISTORY
types instead.

> And why are bookmarks not
> history?  Presumably the user must've visited them at some point (on some
> device)?  And so on (there are few others that I'm surprised by).

This is what UX spec'd for the omnibox autocomplete dropdown on iOS.

[erikwright (departed), 2015-01-12 19:43:31]

[+other components reviewers who can either answer my question or else probably
ought to learn what I'm hoping to learn]

Can anyone explain to me why there is this "common" subdirectory? Is it common
to all platforms? To official and unofficial? ...?

I'm just wondering, given that we are introducing this new ios subdirectory,
whether we are introducing it at the right level.

[jochen (gone - plz use gerrit), 2015-01-12 19:48:26]

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

[jochen (gone - plz use gerrit), 2015-01-12 19:48:26]

i don't see a common subdir?

usually, if a component spans several processes, we encourage to have subdirs
similar to what chrome/ and content/ have

common/ would mean code that is ok to use from both browser and child processes.

is that what you mean?

if there's ios specific code that is to be used from both browser and renderer
processes, I'd expect it to go into common/ios for example

[erikwright (departed), 2015-01-12 19:51:02]

The hierarchy looks like this:

//src/components/resources/default_100_percent/common/omnibox/ios/omnibox_history.png

There is no other subdirectory in "default_100_percent" besides "common".
The 200/300 % folders are similar.

On Mon, Jan 12, 2015 at 2:48 PM, <jochen@chromium.org> wrote:

> i don't see a common subdir?
>
> usually, if a component spans several processes, we encourage to have
> subdirs
> similar to what chrome/ and content/ have
>
> common/ would mean code that is ok to use from both browser and child
> processes.
>
> is that what you mean?
>
> if there's ios specific code that is to be used from both browser and
> renderer
> processes, I'd expect it to go into common/ios for example
>
> https://codereview.chromium.org/847833002/
>

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

[jochen (gone - plz use gerrit), 2015-01-12 19:52:19]

ah, I wasn't looking that deep down in the hierarchy.

no idea about why resources are structured like this

[erikwright (departed), 2015-01-12 19:59:49]

[+oshima for his thoughts]

The common directory was created in http://crrev.com/180943003 as a byproduct of
http://crrev.com/165673007 .

In a comment on the latter (http://crrev.com/165673007#msg21) oshima said:

"Looks like components/resources is new directory right? Could you please create
common subdirectory (assuming they're used on all platforms)?"

So it would appear that the only purpose of "common" is to live alongside "ios"
etc. If that's the case, we should either respect that structure or remove the
"common" directory altogether.

[sdefresne, 2015-01-12 20:02:07]

On 2015/01/12 at 19:59:49, erikwright wrote:
> [+oshima for his thoughts]
> 
> The common directory was created in http://crrev.com/180943003 as a byproduct
of http://crrev.com/165673007 .
> 
> In a comment on the latter (http://crrev.com/165673007#msg21) oshima said:
> 
> "Looks like components/resources is new directory right? Could you please
create common subdirectory (assuming they're used on all platforms)?"
> 
> So it would appear that the only purpose of "common" is to live alongside
"ios" etc. If that's the case, we should either respect that structure or remove
the "common" directory altogether.

So, IIUC, the structure should be:

components/resources/default_100_percent/common/omnibox/omnibox_extension_app.png
components/resources/default_100_percent/ios/omnibox/omnibox_history_incognito.png

And so on, right?

[erikwright (departed), 2015-01-12 20:05:20]

That is my interpretation, yes. Ultimately, it's up to us, of course. But
it seems silly to keep this "common" directory around for no purpose, so we
should either go with the (apparently) intended structure or decide on
something we like more and "make it so". I suspect the former is easier.

On Mon, Jan 12, 2015 at 3:02 PM, <sdefresne@chromium.org> wrote:

> On 2015/01/12 at 19:59:49, erikwright wrote:
>
>> [+oshima for his thoughts]
>>
>
>  The common directory was created in http://crrev.com/180943003 as a
>> byproduct
>>
> of http://crrev.com/165673007 .
>
>  In a comment on the latter (http://crrev.com/165673007#msg21) oshima
>> said:
>>
>
>  "Looks like components/resources is new directory right? Could you please
>>
> create common subdirectory (assuming they're used on all platforms)?"
>
>  So it would appear that the only purpose of "common" is to live alongside
>>
> "ios" etc. If that's the case, we should either respect that structure or
> remove
> the "common" directory altogether.
>
> So, IIUC, the structure should be:
>
> components/resources/default_100_percent/common/omnibox/
> omnibox_extension_app.png
> components/resources/default_100_percent/ios/omnibox/
> omnibox_history_incognito.png
>
> And so on, right?
>
> https://codereview.chromium.org/847833002/
>

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

[oshima, 2015-01-12 20:23:55]

On 2015/01/12 20:05:20, erikwright wrote:
> That is my interpretation, yes. Ultimately, it's up to us, of course. But
> it seems silly to keep this "common" directory around for no purpose, so we
> should either go with the (apparently) intended structure or decide on
> something we like more and "make it so". I suspect the former is easier.

I don't mind having more flat structure if it's for specific purpose (
ui/chromeos/resources, remoting/resources for example). The reason why I
suggested to have "common", is that I expected that components/resources will
be used for wide range of components. (like c/b/theme, ui/resources etc)

We used to have very unorganized file names/structure, (some images had platform
suffix like xxx_mac.png, or yyy_win.png, and some others didn't). We introduced
these directory structure to make it cleaner

Of course, I'm not the owner, so you're right that it's up to you guys
ultimately.

> 
> On Mon, Jan 12, 2015 at 3:02 PM, <mailto:sdefresne@chromium.org> wrote:
> 
> > On 2015/01/12 at 19:59:49, erikwright wrote:
> >
> >> [+oshima for his thoughts]
> >>
> >
> >  The common directory was created in http://crrev.com/180943003 as a
> >> byproduct
> >>
> > of http://crrev.com/165673007 .
> >
> >  In a comment on the latter (http://crrev.com/165673007#msg21) oshima
> >> said:
> >>
> >
> >  "Looks like components/resources is new directory right? Could you please
> >>
> > create common subdirectory (assuming they're used on all platforms)?"
> >
> >  So it would appear that the only purpose of "common" is to live alongside
> >>
> > "ios" etc. If that's the case, we should either respect that structure or
> > remove
> > the "common" directory altogether.
> >
> > So, IIUC, the structure should be:
> >
> > components/resources/default_100_percent/common/omnibox/
> > omnibox_extension_app.png
> > components/resources/default_100_percent/ios/omnibox/
> > omnibox_history_incognito.png
> >
> > And so on, right?
> >
> > https://codereview.chromium.org/847833002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[erikwright (departed), 2015-01-12 20:33:29]

On 2015/01/12 20:23:55, oshima wrote:
> On 2015/01/12 20:05:20, erikwright wrote:
> > That is my interpretation, yes. Ultimately, it's up to us, of course. But
> > it seems silly to keep this "common" directory around for no purpose, so we
> > should either go with the (apparently) intended structure or decide on
> > something we like more and "make it so". I suspect the former is easier.
> 
> I don't mind having more flat structure if it's for specific purpose (
> ui/chromeos/resources, remoting/resources for example). The reason why I
> suggested to have "common", is that I expected that components/resources will
> be used for wide range of components. (like c/b/theme, ui/resources etc)
> 
> We used to have very unorganized file names/structure, (some images had
platform
> suffix like xxx_mac.png, or yyy_win.png, and some others didn't). We
introduced
> these directory structure to make it cleaner
> 
> Of course, I'm not the owner, so you're right that it's up to you guys
> ultimately.
> 
> > 
> > On Mon, Jan 12, 2015 at 3:02 PM, <mailto:sdefresne@chromium.org> wrote:
> > 
> > > On 2015/01/12 at 19:59:49, erikwright wrote:
> > >
> > >> [+oshima for his thoughts]
> > >>
> > >
> > >  The common directory was created in http://crrev.com/180943003 as a
> > >> byproduct
> > >>
> > > of http://crrev.com/165673007 .
> > >
> > >  In a comment on the latter (http://crrev.com/165673007#msg21) oshima
> > >> said:
> > >>
> > >
> > >  "Looks like components/resources is new directory right? Could you please
> > >>
> > > create common subdirectory (assuming they're used on all platforms)?"
> > >
> > >  So it would appear that the only purpose of "common" is to live alongside
> > >>
> > > "ios" etc. If that's the case, we should either respect that structure or
> > > remove
> > > the "common" directory altogether.
> > >
> > > So, IIUC, the structure should be:
> > >
> > > components/resources/default_100_percent/common/omnibox/
> > > omnibox_extension_app.png
> > > components/resources/default_100_percent/ios/omnibox/
> > > omnibox_history_incognito.png
> > >
> > > And so on, right?
> > >
> > > https://codereview.chromium.org/847833002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

Frankly, I don't know anything about how resources are organized elsewhere, or
how they might potentially be organized here.

If you do, your opinion would be valuable.

I don't understand what you meant by:

"The reason why I suggested to have "common", is that I expected that
components/resources will be used for wide range of components. (like c/b/theme,
ui/resources etc)"

According to your expectations, what did you think would be the peers of
"common"?

[blundell, 2015-01-13 02:00:57]

On 2015/01/12 20:33:29, erikwright wrote:
> On 2015/01/12 20:23:55, oshima wrote:
> > On 2015/01/12 20:05:20, erikwright wrote:
> > > That is my interpretation, yes. Ultimately, it's up to us, of course. But
> > > it seems silly to keep this "common" directory around for no purpose, so
we
> > > should either go with the (apparently) intended structure or decide on
> > > something we like more and "make it so". I suspect the former is easier.
> > 
> > I don't mind having more flat structure if it's for specific purpose (
> > ui/chromeos/resources, remoting/resources for example). The reason why I
> > suggested to have "common", is that I expected that components/resources
will
> > be used for wide range of components. (like c/b/theme, ui/resources etc)
> > 
> > We used to have very unorganized file names/structure, (some images had
> platform
> > suffix like xxx_mac.png, or yyy_win.png, and some others didn't). We
> introduced
> > these directory structure to make it cleaner
> > 
> > Of course, I'm not the owner, so you're right that it's up to you guys
> > ultimately.
> > 
> > > 
> > > On Mon, Jan 12, 2015 at 3:02 PM, <mailto:sdefresne@chromium.org> wrote:
> > > 
> > > > On 2015/01/12 at 19:59:49, erikwright wrote:
> > > >
> > > >> [+oshima for his thoughts]
> > > >>
> > > >
> > > >  The common directory was created in http://crrev.com/180943003 as a
> > > >> byproduct
> > > >>
> > > > of http://crrev.com/165673007 .
> > > >
> > > >  In a comment on the latter (http://crrev.com/165673007#msg21) oshima
> > > >> said:
> > > >>
> > > >
> > > >  "Looks like components/resources is new directory right? Could you
please
> > > >>
> > > > create common subdirectory (assuming they're used on all platforms)?"
> > > >
> > > >  So it would appear that the only purpose of "common" is to live
alongside
> > > >>
> > > > "ios" etc. If that's the case, we should either respect that structure
or
> > > > remove
> > > > the "common" directory altogether.
> > > >
> > > > So, IIUC, the structure should be:
> > > >
> > > > components/resources/default_100_percent/common/omnibox/
> > > > omnibox_extension_app.png
> > > > components/resources/default_100_percent/ios/omnibox/
> > > > omnibox_history_incognito.png
> > > >
> > > > And so on, right?
> > > >
> > > > https://codereview.chromium.org/847833002/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Frankly, I don't know anything about how resources are organized elsewhere, or
> how they might potentially be organized here.
> 
> If you do, your opinion would be valuable.
> 
> I don't understand what you meant by:
> 
> "The reason why I suggested to have "common", is that I expected that
> components/resources will be used for wide range of components. (like
c/b/theme,
> ui/resources etc)"
> 
> According to your expectations, what did you think would be the peers of
> "common"?

I don't see a reason for the presence of common/.

[sdefresne, 2015-01-13 18:19:34]

I've reuploaded another patchset that remove the common/ subdirectory.

Please take another look.

[erikwright (departed), 2015-01-13 18:38:07]

LGTM. Thanks!

[sdefresne, 2015-01-14 20:56:10]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-01-14 20:57:23]

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

[commit-bot: I haz the power, 2015-01-14 22:38:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-14 22:40:11]

Patchset 2 (id:??) landed as
https://crrev.com/6401246730331a555a3a04f4aff7ae86117ca060
Cr-Commit-Position: refs/heads/master@{#311559}